Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep app dir #2062

Merged
merged 22 commits into from
Sep 1, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/compat/src/compat-app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ export default class CompatApp {
return this.preprocessJS(
buildFunnel(this.legacyEmberAppInstance.trees.app, {
exclude: ['styles/**', '*.html'],
destDir: 'app',
})
);
}
Expand Down
52 changes: 34 additions & 18 deletions packages/core/src/module-resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -466,8 +466,15 @@ export class Resolver {
} else {
pkg = requestingPkg;
}

return logTransition('entrypoint', request, request.virtualize(resolve(pkg.root, '-embroider-entrypoint.js')));
let matched = resolveExports(pkg.packageJSON, '-embroider-entrypoint.js', {
browser: true,
conditions: ['default', 'imports'],
});
return logTransition(
'entrypoint',
request,
request.virtualize(resolve(pkg.root, matched?.[0] ?? '-embroider-entrypoint.js'))
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a first attempt at fixing the way the entrypoint works, since it may need to crawl a different directory than the package root, given what's in package.json exports.

But this is not really sufficient by itself. The AppFiles class assumes that app and tests have already been smashed together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting the entrypoint inside a path that is mapped via package.json exports seems like a good idea, so this code is probably helpful. It ensures that we're resolving things that are actually supposed to be resolvable from outside the package. But we can't use only this path to identify which dirs to crawl when building the entrypoint, since there is more than one dir to crawl.

Instead, it probably makes sense to make the crawling code attempt to apply resolve.exports to the-package/tests/example and the-package/example to identify the two directories we need to crawl to build AppFiles.

}

private handleTestEntrypoint<R extends ModuleRequest>(request: R): R {
Expand All @@ -494,10 +501,14 @@ export class Resolver {
);
}

let matched = resolveExports(pkg.packageJSON, '-embroider-test-entrypoint.js', {
browser: true,
conditions: ['default', 'imports'],
});
return logTransition(
'test-entrypoint',
request,
request.virtualize(resolve(pkg.root, '-embroider-test-entrypoint.js'))
request.virtualize(resolve(pkg.root, matched?.[0] ?? '-embroider-test-entrypoint.js'))
);
}

Expand Down Expand Up @@ -1333,11 +1344,15 @@ export class Resolver {
if (withinEngine) {
// it's a relative import inside an engine (which also means app), which
// means we may need to satisfy the request via app tree merging.
let appJSMatch = await this.searchAppTree(
request,
withinEngine,
explicitRelative(pkg.root, resolve(dirname(request.fromFile), request.specifier))
);

let logicalName = engineRelativeName(pkg, resolve(dirname(request.fromFile), request.specifier));
if (!logicalName) {
return logTransition(
'fallbackResolve: relative failure because this file is not externally accessible',
request
);
}
let appJSMatch = await this.searchAppTree(request, withinEngine, logicalName);
if (appJSMatch) {
return logTransition('fallbackResolve: relative appJsMatch', request, appJSMatch);
} else {
Expand Down Expand Up @@ -1494,16 +1509,10 @@ export class Resolver {
if (engineConfig) {
// we're directly inside an engine, so we're potentially resolvable as a
// global component

// this kind of mapping is not true in general for all packages, but it
// *is* true for all classical engines (which includes apps) since they
// don't support package.json `exports`. As for a future v2 engine or app:
// this whole method is only relevant for implementing packageRules, which
// should only be for classic stuff. v2 packages should do the right
// things from the beginning and not need packageRules about themselves.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was claiming we don't care about making this code work in native v2 packages, but it honestly seems easier to just make it work than guarantee that nobody will try to ever use reverseComponentLookup in a v2 package.

let inAppName = explicitRelative(engineConfig.root, filename);

return this.tryReverseComponent(engineConfig.packageName, inAppName);
let inAppName = engineRelativeName(owningPackage, filename);
if (inAppName) {
return this.tryReverseComponent(engineConfig.packageName, inAppName);
}
}

let engineInfo = this.reverseSearchAppTree(owningPackage, filename);
Expand Down Expand Up @@ -1555,3 +1564,10 @@ function reliablyResolvable(pkg: V2Package, packageName: string) {
function appImportInAppTree(inPackage: Package, inLogicalPackage: Package, importedPackageName: string): boolean {
return inPackage !== inLogicalPackage && importedPackageName === inLogicalPackage.name;
}

function engineRelativeName(pkg: Package, filename: string): string | undefined {
let outsideName = externalName(pkg.packageJSON, explicitRelative(pkg.root, filename));
if (outsideName) {
return '.' + outsideName.slice(pkg.name.length);
}
}
12 changes: 5 additions & 7 deletions packages/core/src/virtual-entrypoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ import escapeRegExp from 'escape-string-regexp';

const entrypointPattern = /(?<filename>.*)[\\/]-embroider-entrypoint.js/;

export function decodeEntrypoint(filename: string): { fromFile: string } | undefined {
export function decodeEntrypoint(filename: string): { fromDir: string } | undefined {
// Performance: avoid paying regex exec cost unless needed
if (!filename.includes('-embroider-entrypoint')) {
return;
}
let m = entrypointPattern.exec(filename);
if (m) {
return {
fromFile: m.groups!.filename,
fromDir: m.groups!.filename,
};
}
}
Expand All @@ -33,10 +33,10 @@ export function staticAppPathsPattern(staticAppPaths: string[] | undefined): Reg

export function renderEntrypoint(
resolver: Resolver,
{ fromFile }: { fromFile: string }
{ fromDir }: { fromDir: string }
): { src: string; watches: string[] } {
// this is new
const owner = resolver.packageCache.ownerOfFile(fromFile);
const owner = resolver.packageCache.ownerOfFile(fromDir);

let eagerModules: string[] = [];

Expand All @@ -61,7 +61,7 @@ export function renderEntrypoint(
modulePrefix: isApp ? resolver.options.modulePrefix : engine.packageName,
appRelativePath: 'NOT_USED_DELETE_ME',
},
getAppFiles(owner.root),
getAppFiles(fromDir),
hasFastboot ? getFastbootFiles(owner.root) : new Set(),
extensionsPattern(resolver.options.resolvableExtensions),
staticAppPathsPattern(resolver.options.staticAppPaths),
Expand Down Expand Up @@ -154,8 +154,6 @@ export function renderEntrypoint(
const entryTemplate = compile(`
import { macroCondition, getGlobalConfig } from '@embroider/macros';

import environment from './config/environment';

{{#if styles}}
if (macroCondition(!getGlobalConfig().fastboot?.isRunning)) {
{{#each styles as |stylePath| ~}}
Expand Down
12 changes: 6 additions & 6 deletions packages/core/src/virtual-test-entrypoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,27 @@ import { getAppFiles, importPaths, staticAppPathsPattern } from './virtual-entry

const entrypointPattern = /(?<filename>.*)[\\/]-embroider-test-entrypoint.js/;

export function decodeTestEntrypoint(filename: string): { fromFile: string } | undefined {
export function decodeTestEntrypoint(filename: string): { fromDir: string } | undefined {
// Performance: avoid paying regex exec cost unless needed
if (!filename.includes('-embroider-test-entrypoint.js')) {
return;
}
let m = entrypointPattern.exec(filename);
if (m) {
return {
fromFile: m.groups!.filename,
fromDir: m.groups!.filename,
};
}
}

export function renderTestEntrypoint(
resolver: Resolver,
{ fromFile }: { fromFile: string }
{ fromDir }: { fromDir: string }
): { src: string; watches: string[] } {
const owner = resolver.packageCache.ownerOfFile(fromFile);
const owner = resolver.packageCache.ownerOfFile(fromDir);

if (!owner) {
throw new Error(`Owner expected while loading test entrypoint from file: ${fromFile}`);
throw new Error(`Owner expected while loading test entrypoint from file: ${fromDir}`);
}

let engine = resolver.owningEngine(owner);
Expand All @@ -45,7 +45,7 @@ export function renderTestEntrypoint(
modulePrefix: resolver.options.modulePrefix,
appRelativePath: 'NOT_USED_DELETE_ME',
},
getAppFiles(owner.root),
getAppFiles(fromDir),
new Set(), // no fastboot files
extensionsPattern(resolver.options.resolvableExtensions),
staticAppPathsPattern(resolver.options.staticAppPaths),
Expand Down
4 changes: 2 additions & 2 deletions tests/app-template/app/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@

<script src="/@embroider/core/vendor.js"></script>
<script type="module">
import Application from './app';
import environment from './config/environment';
import Application from './app/app';
import environment from './app/config/environment';

Application.create(environment.APP);
</script>
Expand Down
3 changes: 2 additions & 1 deletion tests/app-template/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
"test": "tests"
},
"exports": {
"./*": "./*"
"./tests": "/tests/*",
"./*": "./app/*"
},
"scripts": {
"build": "vite build",
Expand Down
Loading