From 5c7a9e9cfbc723e99281ed706b9c2adee50295c5 Mon Sep 17 00:00:00 2001 From: Chris Manson Date: Thu, 19 Oct 2023 14:11:46 +0100 Subject: [PATCH 1/2] use package paths instead of relative paths for app tree resolving --- packages/compat/src/compat-app-builder.ts | 21 +++++----- packages/core/package.json | 1 + packages/core/src/app-files.ts | 6 +-- packages/core/src/module-resolver.ts | 51 ++++++++++------------- pnpm-lock.yaml | 3 ++ tests/scenarios/core-resolver-test.ts | 2 + 6 files changed, 43 insertions(+), 41 deletions(-) diff --git a/packages/compat/src/compat-app-builder.ts b/packages/compat/src/compat-app-builder.ts index e2f0364b2..15f5c2cb1 100644 --- a/packages/compat/src/compat-app-builder.ts +++ b/packages/compat/src/compat-app-builder.ts @@ -289,9 +289,10 @@ export class CompatAppBuilder { root: realpathSync(index === 0 ? this.root : appFiles.engine.package.root), fastbootFiles: appFiles.fastbootFiles, activeAddons: [...appFiles.engine.addons] - .map(a => ({ - name: a.name, - root: a.root, + .map(([addon, canResolveFromFile]) => ({ + name: addon.name, + root: addon.root, + canResolveFromFile, })) // the traditional order is the order in which addons will run, such // that the last one wins. Our resolver's order is the order to @@ -396,7 +397,7 @@ export class CompatAppBuilder { private impliedAddonAssets(type: keyof ImplicitAssetPaths, { engine }: AppFiles): string[] { let result: Array = []; - for (let addon of sortBy(Array.from(engine.addons), this.scriptPriority.bind(this))) { + for (let addon of sortBy(Array.from(engine.addons.keys()), this.scriptPriority.bind(this))) { let implicitScripts = addon.meta[type]; if (implicitScripts) { let styles = []; @@ -640,12 +641,12 @@ export class CompatAppBuilder { if (!child.isEngine()) { this.findActiveAddons(child, engine, true); } - engine.addons.add(child); + engine.addons.set(child, resolvePath(pkg.root, 'package.json')); } // ensure addons are applied in the correct order, if set (via @embroider/compat/v1-addon) if (!isChild) { - engine.addons = new Set( - [...engine.addons].sort((a, b) => { + engine.addons = new Map( + [...engine.addons].sort(([a], [b]) => { return (a.meta['order-index'] || 0) - (b.meta['order-index'] || 0); }) ); @@ -656,7 +657,7 @@ export class CompatAppBuilder { let queue: Engine[] = [ { package: this.appPackageWithMovedDeps, - addons: new Set(), + addons: new Map(), parent: undefined, sourcePath: appJSPath, modulePrefix: this.modulePrefix(), @@ -671,12 +672,12 @@ export class CompatAppBuilder { break; } this.findActiveAddons(current.package, current); - for (let addon of current.addons) { + for (let addon of current.addons.keys()) { if (addon.isEngine() && !seenEngines.has(addon)) { seenEngines.add(addon); queue.push({ package: addon, - addons: new Set(), + addons: new Map(), parent: current, sourcePath: addon.root, modulePrefix: addon.name, diff --git a/packages/core/package.json b/packages/core/package.json index 4c0e43ad8..57230a432 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -42,6 +42,7 @@ "lodash": "^4.17.21", "resolve": "^1.20.0", "resolve-package-path": "^4.0.1", + "@embroider/reverse-exports": "workspace:*", "typescript-memoize": "^1.0.1", "walk-sync": "^3.0.0" }, diff --git a/packages/core/src/app-files.ts b/packages/core/src/app-files.ts index 3294e282f..4bd825ed2 100644 --- a/packages/core/src/app-files.ts +++ b/packages/core/src/app-files.ts @@ -44,7 +44,7 @@ export class AppFiles { combinedFiles.add(f); } - for (let addon of engine.addons) { + for (let addon of engine.addons.keys()) { let appJS = addon.meta['app-js']; if (appJS) { for (let filename of Object.keys(appJS)) { @@ -202,8 +202,8 @@ export class AppFiles { export interface Engine { // the engine's own package package: Package; - // the set of active addons in the engine - addons: Set; + // the set of active addons in the engine. For each one we keep track of a file that can resolve the addon, because we'll need that later. + addons: Map; // the parent engine, if any parent: Engine | undefined; // where the engine's own V2 code comes from diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index 386feafd3..1cccf2d06 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -11,6 +11,8 @@ import { explicitRelative, RewrittenPackageCache } from '@embroider/shared-inter import makeDebug from 'debug'; import assertNever from 'assert-never'; import resolveModule from 'resolve'; +import reversePackageExports from '@embroider/reverse-exports'; + import { virtualExternalESModule, virtualExternalCJSModule, @@ -70,7 +72,7 @@ export interface Options { interface EngineConfig { packageName: string; - activeAddons: { name: string; root: string }[]; + activeAddons: { name: string; root: string; canResolveFromFile: string }[]; fastbootFiles: { [appName: string]: { localFilename: string; shadowedFilename: string | undefined } }; root: string; } @@ -79,29 +81,29 @@ type MergeEntry = | { type: 'app-only'; 'app-js': { - localPath: string; - packageRoot: string; + specifier: string; + fromFile: string; fromPackageName: string; }; } | { type: 'fastboot-only'; 'fastboot-js': { - localPath: string; - packageRoot: string; + specifier: string; + fromFile: string; fromPackageName: string; }; } | { type: 'both'; 'app-js': { - localPath: string; - packageRoot: string; + specifier: string; + fromFile: string; fromPackageName: string; }; 'fastboot-js': { - localPath: string; - packageRoot: string; + specifier: string; + fromFile: string; fromPackageName: string; }; }; @@ -408,7 +410,7 @@ export class Resolver { return logTransition( 'matched addon entry', request, - request.alias(entry[section].localPath).rehome(resolve(entry[section].packageRoot, 'package.json')) + request.alias(entry[section].specifier).rehome(entry[section].fromFile) ); } } @@ -637,8 +639,8 @@ export class Resolver { engineModules.set(inEngineName, { type: 'app-only', 'app-js': { - localPath: inAddonName, - packageRoot: addon.root, + specifier: reversePackageExports(addon.packageJSON, inAddonName), + fromFile: addonConfig.canResolveFromFile, fromPackageName: addon.name, }, }); @@ -651,8 +653,8 @@ export class Resolver { engineModules.set(inEngineName, { type: 'both', 'app-js': { - localPath: inAddonName, - packageRoot: addon.root, + specifier: reversePackageExports(addon.packageJSON, inAddonName), + fromFile: addonConfig.canResolveFromFile, fromPackageName: addon.name, }, 'fastboot-js': prevEntry['fastboot-js'], @@ -681,8 +683,8 @@ export class Resolver { engineModules.set(inEngineName, { type: 'fastboot-only', 'fastboot-js': { - localPath: inAddonName, - packageRoot: addon.root, + specifier: reversePackageExports(addon.packageJSON, inAddonName), + fromFile: addonConfig.canResolveFromFile, fromPackageName: addon.name, }, }); @@ -695,8 +697,8 @@ export class Resolver { engineModules.set(inEngineName, { type: 'both', 'fastboot-js': { - localPath: inAddonName, - packageRoot: addon.root, + specifier: reversePackageExports(addon.packageJSON, inAddonName), + fromFile: addonConfig.canResolveFromFile, fromPackageName: addon.name, }, 'app-js': prevEntry['app-js'], @@ -1163,18 +1165,11 @@ export class Resolver { case undefined: return undefined; case 'app-only': - return request - .alias(matched.entry['app-js'].localPath) - .rehome(resolve(matched.entry['app-js'].packageRoot, 'package.json')); + return request.alias(matched.entry['app-js'].specifier).rehome(matched.entry['app-js'].fromFile); case 'fastboot-only': - return request - .alias(matched.entry['fastboot-js'].localPath) - .rehome(resolve(matched.entry['fastboot-js'].packageRoot, 'package.json')); + return request.alias(matched.entry['fastboot-js'].specifier).rehome(matched.entry['fastboot-js'].fromFile); case 'both': - let foundAppJS = this.nodeResolve( - matched.entry['app-js'].localPath, - resolve(matched.entry['app-js'].packageRoot, 'package.json') - ); + let foundAppJS = this.nodeResolve(matched.entry['app-js'].specifier, matched.entry['app-js'].fromFile); if (foundAppJS.type !== 'real') { throw new Error( `${matched.entry['app-js'].fromPackageName} declared ${inEngineSpecifier} in packageJSON.ember-addon.app-js, but that module does not exist` diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4edb36992..e99a72bff 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -356,6 +356,9 @@ importers: '@embroider/macros': specifier: workspace:* version: link:../macros + '@embroider/reverse-exports': + specifier: workspace:* + version: link:../reverse-exports '@embroider/shared-internals': specifier: workspace:* version: link:../shared-internals diff --git a/tests/scenarios/core-resolver-test.ts b/tests/scenarios/core-resolver-test.ts index c9d276d65..b3df736d1 100644 --- a/tests/scenarios/core-resolver-test.ts +++ b/tests/scenarios/core-resolver-test.ts @@ -105,10 +105,12 @@ Scenarios.fromProject(() => new Project()) { name: 'my-addon', root: resolve(app.dir, 'node_modules', 'my-addon'), + canResolveFromFile: resolve(app.dir, 'package.json'), }, { name: 'a-v1-addon', root: resolve(app.dir, 'node_modules', 'a-v1-addon'), + canResolveFromFile: resolve(app.dir, 'package.json'), }, ], }, From 56b46f38133fe851cbda7d848046e35f07c7c483 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 24 Oct 2023 10:25:28 -0400 Subject: [PATCH 2/2] always use rewritten paths in canResolveFrom --- packages/compat/src/compat-app-builder.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/compat/src/compat-app-builder.ts b/packages/compat/src/compat-app-builder.ts index 15f5c2cb1..a13799f7b 100644 --- a/packages/compat/src/compat-app-builder.ts +++ b/packages/compat/src/compat-app-builder.ts @@ -641,7 +641,16 @@ export class CompatAppBuilder { if (!child.isEngine()) { this.findActiveAddons(child, engine, true); } - engine.addons.set(child, resolvePath(pkg.root, 'package.json')); + let canResolveFrom: string; + if (pkg === this.appPackageWithMovedDeps) { + // we want canResolveFrom to always be a rewritten package path, and our + // app's package is not rewritten yet here. + canResolveFrom = resolvePath(this.root, 'package.json'); + } else { + // whereas our addons are already moved + canResolveFrom = resolvePath(pkg.root, 'package.json'); + } + engine.addons.set(child, canResolveFrom); } // ensure addons are applied in the correct order, if set (via @embroider/compat/v1-addon) if (!isChild) {