Skip to content

Commit

Permalink
Merge pull request #2062 from embroider-build/keep-app-dir
Browse files Browse the repository at this point in the history
Keep app dir
  • Loading branch information
ef4 authored Sep 1, 2024
2 parents b059849 + ae11588 commit eb93cd5
Show file tree
Hide file tree
Showing 26 changed files with 213 additions and 148 deletions.
1 change: 1 addition & 0 deletions packages/compat/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
"lodash": "^4.17.21",
"pkg-up": "^3.1.0",
"resolve": "^1.20.0",
"resolve.exports": "^2.0.2",
"resolve-package-path": "^4.0.1",
"semver": "^7.3.5",
"symlink-or-copy": "^1.3.1",
Expand Down
7 changes: 7 additions & 0 deletions packages/compat/src/compat-app-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,13 @@ export class CompatAppBuilder {

if ((this.origAppPackage.packageJSON['ember-addon']?.version ?? 0) < 2) {
meta['auto-upgraded'] = true;
// our rewriting keeps app in app directory, etc.
pkgLayers.push({
exports: {
'./*': './app/*',
'./tests/*': './tests/*',
},
});
}

pkgLayers.push({ 'ember-addon': meta });
Expand Down
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
27 changes: 17 additions & 10 deletions packages/compat/src/dependency-rules.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import type { Resolver } from '@embroider/core';
import { getOrCreate } from '@embroider/core';
import { resolve } from 'path';
import { resolve as pathResolve, dirname } from 'path';
import { satisfies } from 'semver';
import { resolve as resolveExports } from 'resolve.exports';

export interface PackageRules {
// This whole set of rules will only apply when the given addon package
Expand Down Expand Up @@ -232,14 +233,20 @@ export function activePackageRules(

export function appTreeRulesDir(root: string, resolver: Resolver) {
let pkg = resolver.packageCache.ownerOfFile(root);
if (pkg?.isV2Addon()) {
// in general v2 addons can keep their app tree stuff in other places than
// "_app_" and we would need to check their package.json to see. But this code
// is only for applying packageRules to auto-upgraded v1 addons and apps, and
// those we always organize predictably.
return resolve(root, '_app_');
} else {
// auto-upgraded apps don't get an exist _app_ dir.
return root;
if (pkg) {
if (pkg.isV2Addon()) {
// in general v2 addons can keep their app tree stuff in other places than
// "_app_" and we would need to check their package.json to see. But this code
// is only for applying packageRules to auto-upgraded v1 addons and apps, and
// those we always organize predictably.
return pathResolve(root, '_app_');
} else {
// this is an app
let matched = resolveExports(pkg.packageJSON, './index.js');
if (matched) {
return dirname(pathResolve(root, matched[0]));
}
}
}
return root;
}
14 changes: 7 additions & 7 deletions packages/compat/src/resolver-transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -547,17 +547,17 @@ class TemplateResolver implements ASTPlugin {
2. Have a mustache statement like: `{{something}}`, where `something` is:
a. Not a variable in scope (for example, there's no preceeding line
a. Not a variable in scope (for example, there's no preceeding line
like `<Parent as |something|>`)
b. Does not start with `@` because that must be an argument from outside this template.
c. Does not contain a dot, like `some.thing` (because that case is classically
c. Does not contain a dot, like `some.thing` (because that case is classically
never a global component resolution that we would need to handle)
d. Does not start with `this` (this rule is mostly redundant with the previous rule,
d. Does not start with `this` (this rule is mostly redundant with the previous rule,
but even a standalone `this` is never a component invocation).
e. Does not have any arguments. If there are argument like `{{something a=b}}`,
there is still ambiguity between helper vs component, but there is no longer
e. Does not have any arguments. If there are argument like `{{something a=b}}`,
there is still ambiguity between helper vs component, but there is no longer
the possibility that this was just rendering some data.
f. Does not take a block, like `{{#something}}{{/something}}` (because that is
f. Does not take a block, like `{{#something}}{{/something}}` (because that is
always a component, no ambiguity.)
We can't tell if this problematic case is really:
Expand All @@ -571,7 +571,7 @@ class TemplateResolver implements ASTPlugin {
2. A component invocation, which you could have written `<Something />`
instead. Angle-bracket invocation has been available and easy-to-adopt
for a very long time.
for a very long time.
3. Property-this-fallback for `{{this.something}}`. Property-this-fallback
is eliminated at Ember 4.0, so people have been heavily pushed to get
Expand Down
4 changes: 4 additions & 0 deletions packages/compat/tests/audit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ describe('audit', function () {
merge(app.pkg, {
'ember-addon': appMeta,
keywords: ['ember-addon'],
exports: {
'./*': './*',
'./tests/*': './tests/*',
},
});
});

Expand Down
120 changes: 69 additions & 51 deletions packages/core/src/module-resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -465,8 +465,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'))
);
}

private handleRouteEntrypoint<R extends ModuleRequest>(request: R): R {
Expand All @@ -486,7 +493,16 @@ export class Resolver {
throw new Error(`bug: found entrypoint import in non-ember package at ${request.fromFile}`);
}

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

return logTransition(
'route entrypoint',
request,
request.virtualize(encodeRouteEntrypoint(pkg.root, matched?.[0], routeName))
);
}

private handleImplicitTestScripts<R extends ModuleRequest>(request: R): R {
Expand Down Expand Up @@ -963,6 +979,11 @@ export class Resolver {
// ember-source might provide backburner via module renaming, but if you
// have an explicit dependency on backburner you should still get that real
// copy.

// if (pkg.root === this.options.engines[0].root && request.specifier === `${pkg.name}/environment/config`) {
// return logTransition('legacy config location', request, request.alias(`${pkg.name}/app/environment/config`));
// }

if (!pkg.hasDependency(packageName)) {
for (let [candidate, replacement] of Object.entries(this.options.renameModules)) {
if (candidate === request.specifier) {
Expand Down Expand Up @@ -1000,35 +1021,33 @@ export class Resolver {
let owningEngine = this.owningEngine(pkg);
let addonConfig = owningEngine.activeAddons.find(a => a.root === pkg.root);
if (addonConfig) {
// auto-upgraded addons get special support for self-resolving here.
return logTransition(`v1 addon self-import`, request, request.rehome(addonConfig.canResolveFromFile));
} else {
let selfImportPath = request.specifier === pkg.name ? './' : request.specifier.replace(pkg.name, '.');
// auto-upgraded apps will necessarily have packageJSON.exports
// because we insert them, so for that support we can fall through to
// that support below.
}
}

// v2 packages are supposed to use package.json `exports` to enable
// self-imports, but not all build tools actually follow the spec. This
// is a workaround for badly behaved packagers.
//
// Known upstream bugs this works around:
// - https://github.com/vitejs/vite/issues/9731
if (pkg.packageJSON.exports) {
let found = resolveExports(pkg.packageJSON, request.specifier, {
browser: true,
conditions: ['default', 'imports'],
});
if (found?.[0]) {
return logTransition(
`v1 app self-import`,
`v2 self-import with package.json exports`,
request,
request.alias(selfImportPath).rehome(resolve(pkg.root, 'package.json'))
request.alias(found?.[0]).rehome(resolve(pkg.root, 'package.json'))
);
}
} else {
// v2 packages are supposed to use package.json `exports` to enable
// self-imports, but not all build tools actually follow the spec. This
// is a workaround for badly behaved packagers.
//
// Known upstream bugs this works around:
// - https://github.com/vitejs/vite/issues/9731
if (pkg.packageJSON.exports) {
let found = resolveExports(pkg.packageJSON, request.specifier, {
browser: true,
conditions: ['default', 'imports'],
});
if (found?.[0]) {
return logTransition(
`v2 self-import with package.json exports`,
request,
request.alias(found?.[0]).rehome(resolve(pkg.root, 'package.json'))
);
}
}
}
}

Expand Down Expand Up @@ -1105,21 +1124,15 @@ export class Resolver {

// if the requesting file is in an addon's app-js, the relative request
// should really be understood as a request for a module in the containing
// engine
// engine.
let logicalLocation = this.reverseSearchAppTree(pkg, request.fromFile);
if (logicalLocation) {
return logTransition(
'beforeResolve: relative import in app-js',
request,
request
.alias('./' + posix.join(dirname(logicalLocation.inAppName), request.specifier))
// it's important that we're rehoming this to the root of the engine
// (which we know really exists), and not to a subdir like
// logicalLocation.inAppName (which might not physically exist),
// because some environments (including node's require.resolve) will
// refuse to do resolution from a notional path that doesn't
// physically exist.
.rehome(resolve(logicalLocation.owningEngine.root, 'package.json'))
request.alias(
posix.join(logicalLocation.owningEngine.packageName, dirname(logicalLocation.inAppName), request.specifier)
)
);
}

Expand Down Expand Up @@ -1301,11 +1314,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 @@ -1462,16 +1479,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.
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 @@ -1523,3 +1534,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
14 changes: 7 additions & 7 deletions packages/core/src/virtual-route-entrypoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,19 @@ import { getAppFiles, getFastbootFiles, importPaths, splitRoute, staticAppPathsP

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

export function encodeRouteEntrypoint(packagePath: string, routeName: string): string {
return resolve(packagePath, `-embroider-route-entrypoint.js:route=${routeName}`);
export function encodeRouteEntrypoint(packagePath: string, matched: string | undefined, routeName: string): string {
return resolve(packagePath, `${matched}:route=${routeName}` ?? `-embroider-route-entrypoint.js:route=${routeName}`);
}

export function decodeRouteEntrypoint(filename: string): { fromFile: string; route: string } | undefined {
export function decodeRouteEntrypoint(filename: string): { fromDir: string; route: string } | undefined {
// Performance: avoid paying regex exec cost unless needed
if (!filename.includes('-embroider-route-entrypoint')) {
return;
}
let m = entrypointPattern.exec(filename);
if (m) {
return {
fromFile: m.groups!.filename,
fromDir: m.groups!.filename,
route: m.groups!.route,
};
}
Expand All @@ -42,9 +42,9 @@ export function decodePublicRouteEntrypoint(specifier: string): string | null {

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

if (!owner) {
throw new Error('Owner expected'); // ToDo: Really bad error, update message
Expand All @@ -67,7 +67,7 @@ export function renderRouteEntrypoint(
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
1 change: 1 addition & 0 deletions packages/shared-internals/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"fs-extra": "^9.1.0",
"lodash": "^4.17.21",
"minimatch": "^3.0.4",
"resolve.exports": "^2.0.2",
"semver": "^7.3.5"
},
"devDependencies": {
Expand Down
Loading

0 comments on commit eb93cd5

Please sign in to comment.