From 28d83173c7a69813f8b9628fab361f612d91e129 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 7 Mar 2023 18:47:09 -0500 Subject: [PATCH 01/26] app tree resolving This supersedes #1357 now that we've landed several prerequisites and can do it properly in one shot. First step here is to implement app-tree resolving module-resolver. --- packages/core/src/module-resolver.ts | 129 ++++++++++++++++++++++---- tests/scenarios/core-resolver-test.ts | 103 ++++++++++++++++---- 2 files changed, 200 insertions(+), 32 deletions(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index e9aa5227c..c369b5252 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -4,7 +4,7 @@ import { extensionsPattern, packageName as getPackageName, } from '@embroider/shared-internals'; -import { dirname, resolve } from 'path'; +import { dirname, resolve, posix } from 'path'; import { PackageCache, Package, V2Package, explicitRelative } from '@embroider/shared-internals'; import makeDebug from 'debug'; import assertNever from 'assert-never'; @@ -268,9 +268,11 @@ export class Resolver { private resolveHelper(path: string, inEngine: EngineConfig, request: R): R { let target = this.parseGlobalPath(path, inEngine); - return request - .alias(`${target.packageName}/helpers/${target.memberName}`) - .rehome(resolve(inEngine.root, 'package.json')); + return logTransition( + 'resolveHelper', + request, + request.alias(`${target.packageName}/helpers/${target.memberName}`).rehome(resolve(inEngine.root, 'package.json')) + ); } private resolveComponent(path: string, inEngine: EngineConfig, request: R): R { @@ -326,7 +328,7 @@ export class Resolver { let helperCandidate = this.resolveHelper(path, inEngine, request); let helperMatch = this.nodeResolve(helperCandidate.specifier, helperCandidate.fromFile); if (helperMatch.type === 'real') { - return helperCandidate; + return logTransition('ambiguous case matched a helper', request, helperCandidate); } // unlike resolveHelper, resolveComponent already does pre-resolution in @@ -334,20 +336,24 @@ export class Resolver { // colocation.≥ let componentMatch = this.resolveComponent(path, inEngine, request); if (componentMatch !== request) { - return componentMatch; + return logTransition('ambiguous case matched a cmoponent', request, componentMatch); } // this is the hard failure case -- we were supposed to find something and // didn't. Let the normal resolution process progress so the user gets a // normal build error. - return request; + return logTransition('ambiguous case failing', request); } private resolveModifier(path: string, inEngine: EngineConfig, request: R): R { let target = this.parseGlobalPath(path, inEngine); - return request - .alias(`${target.packageName}/modifiers/${target.memberName}`) - .rehome(resolve(inEngine.root, 'package.json')); + return logTransition( + 'resolveModifier', + request, + request + .alias(`${target.packageName}/modifiers/${target.memberName}`) + .rehome(resolve(inEngine.root, 'package.json')) + ); } private *componentTemplateCandidates(inPackageName: string) { @@ -391,6 +397,10 @@ export class Resolver { } } + private engineConfig(packageName: string): EngineConfig | undefined { + return this.options.engines.find(e => e.packageName === packageName); + } + owningEngine(pkg: Package) { if (pkg.root === this.options.appRoot) { // the app is always the first engine @@ -493,9 +503,21 @@ export class Resolver { if (isExplicitlyExternal(packageRelativeSpecifier, pkg)) { let publicSpecifier = absoluteSpecifier.replace(pkg.root, pkg.name); return external('beforeResolve', request, publicSpecifier); - } else { - return request; } + + // 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 + let logicalLocation = this.reverseSearchAppTree(pkg, request.fromFile); + if (logicalLocation) { + return logTransition( + 'beforeResolve: relative import in app-js', + request, + request.rehome(resolve(logicalLocation.owningEngine.root, logicalLocation.inAppName)) + ); + } + + return request; } // absolute package imports can also be explicitly external based on their @@ -585,13 +607,27 @@ export class Resolver { let pkg = this.owningPackage(fromFile); if (!pkg || !pkg.isV2Ember()) { - return request; + return logTransition('fallbackResolve: not in an ember package', request); } let packageName = getPackageName(specifier); if (!packageName) { - // this is a relative import, we have nothing more to for it. - return request; + // this is a relative import + + let withinEngine = this.engineConfig(pkg.name); + 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 = this.searchAppTree(request, withinEngine, unrelativize(pkg, request)); + if (appJSMatch) { + return logTransition('fallbackResolve: relative appJsMatch', request, appJSMatch); + } else { + return logTransition('fallbackResolve: relative appJs search failure', request); + } + } else { + // nothing else to do for relative imports + return logTransition('fallbackResolve: relative failure', request); + } } let originalPkg = this.originalPackage(fromFile); @@ -616,6 +652,27 @@ export class Resolver { ); } + let targetingEngine = this.engineConfig(packageName); + if (targetingEngine) { + let appJSMatch = this.searchAppTree(request, targetingEngine, specifier); + if (appJSMatch) { + return logTransition('fallbackResolve: non-relative appJsMatch', request, appJSMatch); + } + } + + let logicalLocation = this.reverseSearchAppTree(pkg, request.fromFile); + if (logicalLocation) { + // the requesting file is in an addon's appTree. We didn't succeed in + // resolving this (non-relative) request from inside the actual addon, so + // next try to resolve it from the corresponding logical location in the + // app. + return logTransition( + 'fallbackResolve: retry from original home of relocated file', + request, + request.rehome(resolve(logicalLocation.owningEngine.root, logicalLocation.inAppName)) + ); + } + if (pkg.meta['auto-upgraded']) { // auto-upgraded packages can fall back to attempting to find dependencies at // runtime. Native v2 packages can only get this behavior in the @@ -633,7 +690,32 @@ export class Resolver { // this is falling through with the original specifier which was // non-resolvable, which will presumably cause a static build error in stage3. - return request; + return logTransition('fallbackResolve final exit', request); + } + + private searchAppTree( + request: R, + engine: EngineConfig, + inEngineSpecifier: string + ): R | undefined { + let packageCache = PackageCache.shared('embroider-stage3', this.options.appRoot); + let targetModule = withoutJSExt(inEngineSpecifier); + + for (let addonConfig of engine.activeAddons) { + let addon = packageCache.get(addonConfig.root); + if (!addon.isV2Addon()) { + continue; + } + let appJS = addon.meta['app-js']; + if (!appJS) { + continue; + } + for (let [inAppName, inAddonName] of Object.entries(appJS)) { + if (targetModule === withoutJSExt(posix.join(engine.packageName, inAppName))) { + return request.alias(inAddonName).rehome(posix.join(addon.root, 'package.json')); + } + } + } } // check whether the given file with the given owningPackage is an addon's @@ -727,3 +809,18 @@ function external(label: string, request: R, specifier: let filename = virtualExternalModule(specifier); return logTransition(label, request, request.virtualize(filename)); } + +// this is specifically for app-js handling, where only .js and .hbs are legal +// extensiosn, and only .js is allowed to be an *implied* extension (.hbs must +// be explicit). So when normalizing such paths, it's only a .js suffix that we +// must remove. +function withoutJSExt(filename: string): string { + return filename.replace(/\.js$/, ''); +} + +function unrelativize(pkg: Package, request: ModuleRequest) { + if (pkg.packageJSON.exports) { + throw new Error(`unsupported: engines cannot use package.json exports`); + } + return resolve(dirname(request.fromFile), request.specifier).replace(pkg.root, pkg.name); +} diff --git a/tests/scenarios/core-resolver-test.ts b/tests/scenarios/core-resolver-test.ts index bb103e00e..6bf3989d6 100644 --- a/tests/scenarios/core-resolver-test.ts +++ b/tests/scenarios/core-resolver-test.ts @@ -31,22 +31,6 @@ Scenarios.fromProject(() => new Project()) }; app.mergeFiles({ 'index.html': '', - node_modules: { - 'my-addon': { - 'package.json': JSON.stringify( - (() => { - let meta: AddonMeta = { type: 'addon', version: 2, 'auto-upgraded': true }; - return { - name: 'my-addon', - keywords: ['ember-addon'], - 'ember-addon': meta, - }; - })(), - null, - 2 - ), - }, - }, }); }) .forEachScenario(scenario => { @@ -57,6 +41,7 @@ Scenarios.fromProject(() => new Project()) interface ConfigureOpts { podModulePrefix?: string; renamePackages?: Record; + addonMeta?: Partial; } let configure: (opts?: ConfigureOpts) => Promise; @@ -116,6 +101,18 @@ Scenarios.fromProject(() => new Project()) module.exports = function(filename) { return true } `, '.embroider/resolver.json': JSON.stringify(resolverOptions), + 'node_modules/my-addon/package.json': JSON.stringify( + (() => { + let meta: AddonMeta = { type: 'addon', version: 2, 'auto-upgraded': true, ...(opts?.addonMeta ?? {}) }; + return { + name: 'my-addon', + keywords: ['ember-addon'], + 'ember-addon': meta, + }; + })(), + null, + 2 + ), }); expectAudit = await assert.audit({ outputDir: app.dir }); @@ -426,5 +423,79 @@ Scenarios.fromProject(() => new Project()) .to('./helpers/something/hello-world.js'); }); }); + + Qmodule('engine-relative resolving', function () { + test('module in app takes precedence', async function () { + givenFiles({ + 'node_modules/my-addon/_app_/hello-world.js': '', + './hello-world.js': '', + 'app.js': `import "my-app/hello-world"`, + }); + + await configure({ + addonMeta: { + 'app-js': { './hello-world': './_app_/hello-world.js' }, + }, + }); + + expectAudit.module('./app.js').resolves('my-app/hello-world').to('./hello-world.js'); + }); + + test('module in addon is found', async function () { + givenFiles({ + 'node_modules/my-addon/_app_/hello-world.js': '', + 'app.js': `import "my-app/hello-world"`, + }); + + await configure({ + addonMeta: { + 'app-js': { './hello-world': './_app_/hello-world.js' }, + }, + }); + + expectAudit + .module('./app.js') + .resolves('my-app/hello-world') + .to('./node_modules/my-addon/_app_/hello-world.js'); + }); + + test(`relative import in addon's app tree resolves to app`, async function () { + givenFiles({ + 'node_modules/my-addon/_app_/hello-world.js': `import "./secondary"`, + 'app.js': `import "my-app/hello-world"`, + 'secondary.js': '', + }); + + await configure({ + addonMeta: { + 'app-js': { './hello-world': './_app_/hello-world.js' }, + }, + }); + + expectAudit + .module('./node_modules/my-addon/_app_/hello-world.js') + .resolves('./secondary') + .to('./secondary.js'); + }); + + test(`absolute import in addon's app tree resolves to app`, async function () { + givenFiles({ + 'node_modules/my-addon/_app_/hello-world.js': `import "my-app/secondary"`, + 'app.js': `import "my-app/hello-world"`, + 'secondary.js': '', + }); + + await configure({ + addonMeta: { + 'app-js': { './hello-world': './_app_/hello-world.js' }, + }, + }); + + expectAudit + .module('./node_modules/my-addon/_app_/hello-world.js') + .resolves('my-app/secondary') + .to('./secondary.js'); + }); + }); }); }); From b58732d20391990604162e2672cac6bbe830a550 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Wed, 8 Mar 2023 00:33:48 -0500 Subject: [PATCH 02/26] standardize specifiers on windows --- packages/core/src/module-resolver.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index c369b5252..8828ecec5 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -4,7 +4,7 @@ import { extensionsPattern, packageName as getPackageName, } from '@embroider/shared-internals'; -import { dirname, resolve, posix } from 'path'; +import { dirname, resolve, posix, sep } from 'path'; import { PackageCache, Package, V2Package, explicitRelative } from '@embroider/shared-internals'; import makeDebug from 'debug'; import assertNever from 'assert-never'; @@ -72,7 +72,7 @@ export interface ModuleRequest { } class NodeModuleRequest implements ModuleRequest { - constructor(readonly specifier: string, readonly fromFile: string, readonly isVirtual = false) {} + constructor(readonly specifier: string, readonly fromFile: string, readonly isVirtual = false) { } alias(specifier: string): this { return new NodeModuleRequest(specifier, this.fromFile) as this; } @@ -97,7 +97,7 @@ export type SyncResolverFunction Res; export class Resolver { - constructor(private options: Options) {} + constructor(private options: Options) { } beforeResolve(request: R): R { if (request.specifier === '@embroider/macros') { @@ -822,5 +822,9 @@ function unrelativize(pkg: Package, request: ModuleRequest) { if (pkg.packageJSON.exports) { throw new Error(`unsupported: engines cannot use package.json exports`); } - return resolve(dirname(request.fromFile), request.specifier).replace(pkg.root, pkg.name); + let result = resolve(dirname(request.fromFile), request.specifier).replace(pkg.root, pkg.name); + if (sep !== '/') { + result = result.split(sep).join('/'); + } + return result; } From 8b63e2895703b8a91cddad83078be3f3eb692df8 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Wed, 8 Mar 2023 00:42:05 -0500 Subject: [PATCH 03/26] previous commit on windows didn't run prettier --- packages/core/src/module-resolver.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index 8828ecec5..f2a609e62 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -72,7 +72,7 @@ export interface ModuleRequest { } class NodeModuleRequest implements ModuleRequest { - constructor(readonly specifier: string, readonly fromFile: string, readonly isVirtual = false) { } + constructor(readonly specifier: string, readonly fromFile: string, readonly isVirtual = false) {} alias(specifier: string): this { return new NodeModuleRequest(specifier, this.fromFile) as this; } @@ -97,7 +97,7 @@ export type SyncResolverFunction Res; export class Resolver { - constructor(private options: Options) { } + constructor(private options: Options) {} beforeResolve(request: R): R { if (request.specifier === '@embroider/macros') { From 0813263db44e69c1cdbecca2f1408a08e056aa87 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Wed, 8 Mar 2023 09:04:15 -0500 Subject: [PATCH 04/26] switch to shared version of this utility --- packages/core/src/module-resolver.ts | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index f2a609e62..547a00ba4 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -4,12 +4,13 @@ import { extensionsPattern, packageName as getPackageName, } from '@embroider/shared-internals'; -import { dirname, resolve, posix, sep } from 'path'; +import { dirname, resolve, posix } from 'path'; import { PackageCache, Package, V2Package, explicitRelative } from '@embroider/shared-internals'; import makeDebug from 'debug'; import assertNever from 'assert-never'; import resolveModule from 'resolve'; import { virtualExternalModule, virtualPairComponent, virtualContent } from './virtual-content'; +import { unrelativize } from '@embroider/shared-internals'; const debug = makeDebug('embroider:resolver'); function logTransition(reason: string, before: R, after: R = before): R { @@ -618,7 +619,11 @@ 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 = this.searchAppTree(request, withinEngine, unrelativize(pkg, request)); + let appJSMatch = this.searchAppTree( + request, + withinEngine, + unrelativize(pkg, request.specifier, request.fromFile) + ); if (appJSMatch) { return logTransition('fallbackResolve: relative appJsMatch', request, appJSMatch); } else { @@ -817,14 +822,3 @@ function external(label: string, request: R, specifier: function withoutJSExt(filename: string): string { return filename.replace(/\.js$/, ''); } - -function unrelativize(pkg: Package, request: ModuleRequest) { - if (pkg.packageJSON.exports) { - throw new Error(`unsupported: engines cannot use package.json exports`); - } - let result = resolve(dirname(request.fromFile), request.specifier).replace(pkg.root, pkg.name); - if (sep !== '/') { - result = result.split(sep).join('/'); - } - return result; -} From bd58c427dfbb90fd7cc406614a526715802884cb Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Wed, 8 Mar 2023 09:08:54 -0500 Subject: [PATCH 05/26] disabling on-disk app-tree merge --- packages/core/src/app-differ.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/core/src/app-differ.ts b/packages/core/src/app-differ.ts index f8b0eb657..0ad0d1215 100644 --- a/packages/core/src/app-differ.ts +++ b/packages/core/src/app-differ.ts @@ -107,7 +107,9 @@ export default class AppDiffer { this.isFastbootOnly.set(relativePath, sourceIndices[0] >= this.firstFastbootTree); let source = this.sources[sourceIndices[0]]; let sourceFile = source.locate(relativePath); - copySync(sourceFile, outputPath, { dereference: true }); + if (!source.isRelocated) { + copySync(sourceFile, outputPath, { dereference: true }); + } this.updateFiles(relativePath, source, sourceFile); } else { // we have both fastboot and non-fastboot files for this path. @@ -122,12 +124,14 @@ export default class AppDiffer { let base = basename(relativePath); let browserDest = `_browser_${base}`; let fastbootDest = `_fastboot_${base}`; - copySync(browserSourceFile, join(this.outputPath, dir, browserDest), { dereference: true }); - copySync(fastbootSourceFile, join(this.outputPath, dir, fastbootDest), { dereference: true }); - writeFileSync( - outputPath, - switcher(browserDest, fastbootDest, this.babelParserConfig!, readFileSync(browserSourceFile, 'utf8')) - ); + if (!browserSrc.isRelocated && !fastbootSrc.isRelocated) { + copySync(browserSourceFile, join(this.outputPath, dir, browserDest), { dereference: true }); + copySync(fastbootSourceFile, join(this.outputPath, dir, fastbootDest), { dereference: true }); + writeFileSync( + outputPath, + switcher(browserDest, fastbootDest, this.babelParserConfig!, readFileSync(browserSourceFile, 'utf8')) + ); + } this.updateFiles(relativePath, browserSrc, browserSourceFile); } break; From 196f4496c017e5a7f4e2e64217ddd2a08e1d3f4d Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Wed, 8 Mar 2023 11:15:57 -0500 Subject: [PATCH 06/26] simplify searchAppTree --- packages/core/src/module-resolver.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index 547a00ba4..a7ec4e461 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -4,13 +4,12 @@ import { extensionsPattern, packageName as getPackageName, } from '@embroider/shared-internals'; -import { dirname, resolve, posix } from 'path'; +import { dirname, resolve } from 'path'; import { PackageCache, Package, V2Package, explicitRelative } from '@embroider/shared-internals'; import makeDebug from 'debug'; import assertNever from 'assert-never'; import resolveModule from 'resolve'; import { virtualExternalModule, virtualPairComponent, virtualContent } from './virtual-content'; -import { unrelativize } from '@embroider/shared-internals'; const debug = makeDebug('embroider:resolver'); function logTransition(reason: string, before: R, after: R = before): R { @@ -622,7 +621,7 @@ export class Resolver { let appJSMatch = this.searchAppTree( request, withinEngine, - unrelativize(pkg, request.specifier, request.fromFile) + explicitRelative(pkg.root, resolve(dirname(fromFile), specifier)) ); if (appJSMatch) { return logTransition('fallbackResolve: relative appJsMatch', request, appJSMatch); @@ -659,7 +658,7 @@ export class Resolver { let targetingEngine = this.engineConfig(packageName); if (targetingEngine) { - let appJSMatch = this.searchAppTree(request, targetingEngine, specifier); + let appJSMatch = this.searchAppTree(request, targetingEngine, specifier.replace(packageName, '.')); if (appJSMatch) { return logTransition('fallbackResolve: non-relative appJsMatch', request, appJSMatch); } @@ -716,8 +715,8 @@ export class Resolver { continue; } for (let [inAppName, inAddonName] of Object.entries(appJS)) { - if (targetModule === withoutJSExt(posix.join(engine.packageName, inAppName))) { - return request.alias(inAddonName).rehome(posix.join(addon.root, 'package.json')); + if (targetModule === withoutJSExt(inAppName)) { + return request.alias(inAddonName).rehome(resolve(addon.root, 'package.json')); } } } From fff11ab26d20483f416667087a3ef53d11c43cdb Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Wed, 8 Mar 2023 18:05:31 -0500 Subject: [PATCH 07/26] implement fastboot switching --- packages/compat/src/audit.ts | 4 +- packages/core/src/app-differ.ts | 9 +- packages/core/src/describe-exports.ts | 21 ++- packages/core/src/module-resolver.ts | 201 +++++++++++++++++++++++--- packages/core/src/virtual-content.ts | 47 +++++- tests/scenarios/core-resolver-test.ts | 84 ++++++++++- tests/scenarios/fastboot-app-test.ts | 2 +- tests/v2-addon-template/package.json | 3 + 8 files changed, 330 insertions(+), 41 deletions(-) diff --git a/packages/compat/src/audit.ts b/packages/compat/src/audit.ts index 2a7a829f5..f83baf8ad 100644 --- a/packages/compat/src/audit.ts +++ b/packages/compat/src/audit.ts @@ -139,7 +139,9 @@ export class AuditResults { })) : [], exports: module.linked?.exports ? [...module.linked.exports] : [], - content: module.parsed?.transpiledContent ? module.parsed?.transpiledContent.toString() : '', + content: module.parsed?.transpiledContent + ? module.parsed?.transpiledContent.toString() + : 'module failed to transpile', }; results.modules[explicitRelative(baseDir, filename)] = publicModule; } diff --git a/packages/core/src/app-differ.ts b/packages/core/src/app-differ.ts index 0ad0d1215..f800b76dc 100644 --- a/packages/core/src/app-differ.ts +++ b/packages/core/src/app-differ.ts @@ -198,8 +198,13 @@ function switcher( babelParserConfig: TransformOptions, browserSource: string ): string { - let { names, hasDefaultExport } = describeExports(browserSource, babelParserConfig); - return switcherTemplate({ fastbootDest, browserDest, names: [...names], hasDefaultExport }); + let { names } = describeExports(browserSource, babelParserConfig); + return switcherTemplate({ + fastbootDest, + browserDest, + names: [...names].filter(name => name !== 'default'), + hasDefaultExport: names.has('default'), + }); } interface Source extends InputTree { diff --git a/packages/core/src/describe-exports.ts b/packages/core/src/describe-exports.ts index fb69d31c4..be19ba5a5 100644 --- a/packages/core/src/describe-exports.ts +++ b/packages/core/src/describe-exports.ts @@ -3,16 +3,12 @@ import traverse, { NodePath } from '@babel/traverse'; import { types as t } from '@babel/core'; import assertNever from 'assert-never'; -export function describeExports( - code: string, - babelParserConfig: TransformOptions -): { names: Set; hasDefaultExport: boolean } { +export function describeExports(code: string, babelParserConfig: TransformOptions): { names: Set } { let ast = parse(code, babelParserConfig); if (!ast || ast.type !== 'File') { throw new Error(`bug in embroider/core describe-exports`); } let names: Set = new Set(); - let hasDefaultExport = false; traverse(ast, { ExportNamedDeclaration(path: NodePath) { @@ -22,11 +18,7 @@ export function describeExports( case 'ExportNamespaceSpecifier': const name = spec.exported.type === 'Identifier' ? spec.exported.name : spec.exported.value; - if (name === 'default') { - hasDefaultExport = true; - } else { - names.add(name); - } + names.add(name); break; case 'ExportDefaultSpecifier': // this is in the types but was never standardized @@ -42,10 +34,15 @@ export function describeExports( } } } + if (t.isFunctionDeclaration(path.node.declaration) || t.isClassDeclaration(path.node.declaration)) { + if (t.isIdentifier(path.node.declaration.id)) { + names.add(path.node.declaration.id.name); + } + } }, ExportDefaultDeclaration(_path: NodePath) { - hasDefaultExport = true; + names.add('default'); }, }); - return { names, hasDefaultExport }; + return { names }; } diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index a7ec4e461..c82ff87f3 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -9,7 +9,16 @@ import { PackageCache, Package, V2Package, explicitRelative } from '@embroider/s import makeDebug from 'debug'; import assertNever from 'assert-never'; import resolveModule from 'resolve'; -import { virtualExternalModule, virtualPairComponent, virtualContent } from './virtual-content'; +import { + virtualExternalModule, + virtualPairComponent, + virtualContent, + fastbootSwitch, + decodeFastbootSwitch, +} from './virtual-content'; +import { Memoize } from 'typescript-memoize'; +import { describeExports } from './describe-exports'; +import { readFileSync } from 'fs'; const debug = makeDebug('embroider:resolver'); function logTransition(reason: string, before: R, after: R = before): R { @@ -60,6 +69,39 @@ interface EngineConfig { root: string; } +type MergeEntry = + | { + type: 'app-only'; + 'app-js': { + localPath: string; + packageRoot: string; + fromPackageName: string; + }; + } + | { + type: 'fastboot-only'; + 'fastboot-js': { + localPath: string; + packageRoot: string; + fromPackageName: string; + }; + } + | { + type: 'both'; + 'app-js': { + localPath: string; + packageRoot: string; + fromPackageName: string; + }; + 'fastboot-js': { + localPath: string; + packageRoot: string; + fromPackageName: string; + }; + }; + +type MergeMap = Map>; + const compatPattern = /#embroider_compat\/(?[^\/]+)\/(?.*)/; export interface ModuleRequest { @@ -108,6 +150,7 @@ export class Resolver { return logTransition('early exit', request); } + request = this.handleFastbootCompat(request); request = this.handleGlobalsCompat(request); request = this.handleRenaming(request); return this.preHandleExternal(request); @@ -240,6 +283,35 @@ export class Resolver { } } + private handleFastbootCompat(request: R): R { + let match = decodeFastbootSwitch(request.fromFile); + if (!match) { + return request; + } + + let section: 'app-js' | 'fastboot-js' | undefined; + if (request.specifier === './browser') { + section = 'app-js'; + } else if (request.specifier === './fastboot') { + section = 'fastboot-js'; + } + + if (!section) { + return request; + } + + let pkg = this.owningPackage(match.filename); + if (pkg) { + let rel = withoutJSExt(explicitRelative(pkg.root, match.filename)); + let entry = this.mergeMap.get(pkg.root)?.get(rel); + if (entry?.type === 'both') { + return request.alias(entry[section].localPath).rehome(resolve(entry[section].packageRoot, 'package.json')); + } + } + + return request; + } + private handleGlobalsCompat(request: R): R { let match = compatPattern.exec(request.specifier); if (!match) { @@ -401,6 +473,95 @@ export class Resolver { return this.options.engines.find(e => e.packageName === packageName); } + // This is where we figure out how all the classic treeForApp merging bottoms + // out. + @Memoize() + private get mergeMap(): MergeMap { + let packageCache = PackageCache.shared('embroider-stage3', this.options.appRoot); + let result: MergeMap = new Map(); + for (let engine of this.options.engines) { + let engineModules: Map = new Map(); + for (let addonConfig of engine.activeAddons) { + let addon = packageCache.get(addonConfig.root); + if (!addon.isV2Addon()) { + continue; + } + + let appJS = addon.meta['app-js']; + if (appJS) { + for (let [inEngineName, inAddonName] of Object.entries(appJS)) { + inEngineName = withoutJSExt(inEngineName); + let prevEntry = engineModules.get(inEngineName); + switch (prevEntry?.type) { + case undefined: + engineModules.set(inEngineName, { + type: 'app-only', + 'app-js': { + localPath: inAddonName, + packageRoot: addon.root, + fromPackageName: addon.name, + }, + }); + break; + case 'app-only': + case 'both': + // first match wins, so this one is shadowed + break; + case 'fastboot-only': + engineModules.set(inEngineName, { + type: 'both', + 'app-js': { + localPath: inAddonName, + packageRoot: addon.root, + fromPackageName: addon.name, + }, + 'fastboot-js': prevEntry['fastboot-js'], + }); + break; + } + } + } + + let fastbootJS = addon.meta['fastboot-js']; + if (fastbootJS) { + for (let [inEngineName, inAddonName] of Object.entries(fastbootJS)) { + inEngineName = withoutJSExt(inEngineName); + let prevEntry = engineModules.get(inEngineName); + switch (prevEntry?.type) { + case undefined: + engineModules.set(inEngineName, { + type: 'fastboot-only', + 'fastboot-js': { + localPath: inAddonName, + packageRoot: addon.root, + fromPackageName: addon.name, + }, + }); + break; + case 'fastboot-only': + case 'both': + // first match wins, so this one is shadowed + break; + case 'app-only': + engineModules.set(inEngineName, { + type: 'both', + 'fastboot-js': { + localPath: inAddonName, + packageRoot: addon.root, + fromPackageName: addon.name, + }, + 'app-js': prevEntry['app-js'], + }); + break; + } + } + } + } + result.set(engine.root, engineModules); + } + return result; + } + owningEngine(pkg: Package) { if (pkg.root === this.options.appRoot) { // the app is always the first engine @@ -702,23 +863,29 @@ export class Resolver { engine: EngineConfig, inEngineSpecifier: string ): R | undefined { - let packageCache = PackageCache.shared('embroider-stage3', this.options.appRoot); - let targetModule = withoutJSExt(inEngineSpecifier); - - for (let addonConfig of engine.activeAddons) { - let addon = packageCache.get(addonConfig.root); - if (!addon.isV2Addon()) { - continue; - } - let appJS = addon.meta['app-js']; - if (!appJS) { - continue; - } - for (let [inAppName, inAddonName] of Object.entries(appJS)) { - if (targetModule === withoutJSExt(inAppName)) { - return request.alias(inAddonName).rehome(resolve(addon.root, 'package.json')); + inEngineSpecifier = withoutJSExt(inEngineSpecifier); + let entry = this.mergeMap.get(engine.root)?.get(inEngineSpecifier); + switch (entry?.type) { + case undefined: + return undefined; + case 'app-only': + return request.alias(entry['app-js'].localPath).rehome(resolve(entry['app-js'].packageRoot, 'package.json')); + case 'fastboot-only': + return request + .alias(entry['fastboot-js'].localPath) + .rehome(resolve(entry['fastboot-js'].packageRoot, 'package.json')); + case 'both': + let foundAppJS = this.nodeResolve( + entry['app-js'].localPath, + resolve(entry['app-js'].packageRoot, 'package.json') + ); + if (foundAppJS.type !== 'real') { + throw new Error( + `${entry['app-js'].fromPackageName} declared ${inEngineSpecifier} in packageJSON.ember-addon.app-js, but that module does not exist` + ); } - } + let { names } = describeExports(readFileSync(foundAppJS.filename, 'utf8'), {}); + return request.virtualize(fastbootSwitch(request.specifier, request.fromFile, names)); } } diff --git a/packages/core/src/virtual-content.ts b/packages/core/src/virtual-content.ts index 28cc1559f..e4f0979ec 100644 --- a/packages/core/src/virtual-content.ts +++ b/packages/core/src/virtual-content.ts @@ -1,4 +1,4 @@ -import { dirname, basename } from 'path'; +import { dirname, basename, resolve } from 'path'; import { explicitRelative } from '.'; import { compile } from './js-handlebars'; @@ -16,6 +16,12 @@ export function virtualContent(filename: string): string { if (match) { return pairedComponentShim(match); } + + let fb = decodeFastbootSwitch(filename); + if (fb) { + return fastbootSwitchTemplate(fb); + } + throw new Error(`not an @embroider/core virtual file: ${filename}`); } @@ -88,3 +94,42 @@ function decodeVirtualPairComponent( debugName: basename(relativeHBSModule).replace(/\.(js|hbs)$/, ''), }; } + +const fastbootSwitchSuffix = '/embroider_fastboot_switch'; +const fastbootSwitchPattern = /(?.+)\/embroider_fastboot_switch(?:\?names=(?.+))?$/; +export function fastbootSwitch(specifier: string, fromFile: string, names: Set): string { + let filename = `${resolve(dirname(fromFile), specifier)}${fastbootSwitchSuffix}`; + if (names.size > 0) { + return `${filename}?names=${[...names].join(',')}`; + } else { + return filename; + } +} + +export function decodeFastbootSwitch(filename: string) { + let match = fastbootSwitchPattern.exec(filename); + if (match) { + let names = match.groups?.names?.split(',') ?? []; + return { + names: names.filter(name => name !== 'default'), + hasDefaultExport: names.includes('default'), + filename: match.groups!.original, + }; + } +} + +const fastbootSwitchTemplate = compile(` +import { macroCondition, getGlobalConfig, importSync } from '@embroider/macros'; +let mod; +if (macroCondition(getGlobalConfig().fastboot?.isRunning)){ + mod = importSync('./fastboot'); +} else { + mod = importSync('./browser'); +} +{{#if hasDefaultExport}} +export default mod.default; +{{/if}} +{{#each names as |name|}} +export const {{name}} = mod.{{name}}; +{{/each}} +`) as (params: { names: string[]; hasDefaultExport: boolean }) => string; diff --git a/tests/scenarios/core-resolver-test.ts b/tests/scenarios/core-resolver-test.ts index 6bf3989d6..4ffb40a39 100644 --- a/tests/scenarios/core-resolver-test.ts +++ b/tests/scenarios/core-resolver-test.ts @@ -134,9 +134,13 @@ Scenarios.fromProject(() => new Project()) .to('./components/hello-world.js'); }); + hooks.afterEach(() => { + expectAudit.hasNoProblems(); + }); + test('js-and-hbs component', async function () { givenFiles({ - 'components/hello-world.js': '', + 'components/hello-world.js': 'export default function() {}', 'templates/components/hello-world.hbs': '', 'app.js': `import "#embroider_compat/components/hello-world"`, }); @@ -305,7 +309,7 @@ Scenarios.fromProject(() => new Project()) test('podded js-and-hbs component with blank podModulePrefix', async function () { givenFiles({ - 'components/hello-world/component.js': '', + 'components/hello-world/component.js': 'export default function() {}', 'components/hello-world/template.hbs': '', 'app.js': `import "#embroider_compat/components/hello-world"`, }); @@ -330,7 +334,7 @@ Scenarios.fromProject(() => new Project()) test('podded js-and-hbs component with non-blank podModulePrefix', async function () { givenFiles({ - 'pods/components/hello-world/component.js': '', + 'pods/components/hello-world/component.js': 'export default function() {}', 'pods/components/hello-world/template.hbs': '', 'app.js': `import "#embroider_compat/components/hello-world"`, }); @@ -434,7 +438,7 @@ Scenarios.fromProject(() => new Project()) await configure({ addonMeta: { - 'app-js': { './hello-world': './_app_/hello-world.js' }, + 'app-js': { './hello-world.js': './_app_/hello-world.js' }, }, }); @@ -449,7 +453,7 @@ Scenarios.fromProject(() => new Project()) await configure({ addonMeta: { - 'app-js': { './hello-world': './_app_/hello-world.js' }, + 'app-js': { './hello-world.js': './_app_/hello-world.js' }, }, }); @@ -468,7 +472,7 @@ Scenarios.fromProject(() => new Project()) await configure({ addonMeta: { - 'app-js': { './hello-world': './_app_/hello-world.js' }, + 'app-js': { './hello-world.js': './_app_/hello-world.js' }, }, }); @@ -487,7 +491,7 @@ Scenarios.fromProject(() => new Project()) await configure({ addonMeta: { - 'app-js': { './hello-world': './_app_/hello-world.js' }, + 'app-js': { './hello-world.js': './_app_/hello-world.js' }, }, }); @@ -496,6 +500,72 @@ Scenarios.fromProject(() => new Project()) .resolves('my-app/secondary') .to('./secondary.js'); }); + + test(`resolves addon fastboot-js`, async function () { + givenFiles({ + 'node_modules/my-addon/_fastboot_/hello-world.js': ``, + 'app.js': `import "my-app/hello-world"`, + }); + + await configure({ + addonMeta: { + 'fastboot-js': { './hello-world.js': './_fastboot_/hello-world.js' }, + }, + }); + + expectAudit + .module('./app.js') + .resolves('my-app/hello-world') + .to('./node_modules/my-addon/_fastboot_/hello-world.js'); + }); + + test(`file exists in both app-js and fastboot-js`, async function () { + givenFiles({ + 'node_modules/my-addon/_fastboot_/hello-world.js': ` + export function hello() { return 'fastboot'; } + export class Bonjour {} + export default function() {} + const value = 1; + export { value }; + export const x = 2; + `, + 'node_modules/my-addon/_app_/hello-world.js': ` + export function hello() { return 'browser'; } + export class Bonjour {} + export default function() {} + const value = 1; + export { value }; + export const x = 2; + `, + 'app.js': `import "my-app/hello-world"`, + }); + + await configure({ + addonMeta: { + 'fastboot-js': { './hello-world.js': './_fastboot_/hello-world.js' }, + 'app-js': { './hello-world.js': './_app_/hello-world.js' }, + }, + }); + + let switcherModule = expectAudit.module('./app.js').resolves('my-app/hello-world').toModule(); + switcherModule.codeEquals(` + import { macroCondition, getGlobalConfig, importSync } from '@embroider/macros'; + let mod; + if (macroCondition(getGlobalConfig().fastboot?.isRunning)) { + mod = importSync("./fastboot"); + } else { + mod = importSync("./browser"); + } + export default mod.default; + export const hello = mod.hello; + export const Bonjour = mod.Bonjour; + export const value = mod.value; + export const x = mod.x; + `); + + switcherModule.resolves('./fastboot').to('./node_modules/my-addon/_fastboot_/hello-world.js'); + switcherModule.resolves('./browser').to('./node_modules/my-addon/_app_/hello-world.js'); + }); }); }); }); diff --git a/tests/scenarios/fastboot-app-test.ts b/tests/scenarios/fastboot-app-test.ts index 9fb45b539..77860dbc8 100644 --- a/tests/scenarios/fastboot-app-test.ts +++ b/tests/scenarios/fastboot-app-test.ts @@ -31,7 +31,7 @@ appScenarios let v2Example = baseV2Addon(); v2Example.pkg.name = 'v2-example'; (v2Example.pkg as any)['ember-addon']['app-js']['./components/v2-example-component.js'] = - 'app/components/v2-example-component.js'; + './app/components/v2-example-component.js'; merge(v2Example.files, { app: { components: { diff --git a/tests/v2-addon-template/package.json b/tests/v2-addon-template/package.json index d0a9180f1..8bb9c07c6 100644 --- a/tests/v2-addon-template/package.json +++ b/tests/v2-addon-template/package.json @@ -13,5 +13,8 @@ "version": 2, "app-js": {}, "main": "addon-main.js" + }, + "exports": { + "./*": "./*" } } From 3c6f16204b065592e8002c177481f600c334c756 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Wed, 8 Mar 2023 20:11:07 -0500 Subject: [PATCH 08/26] fix qunit warning --- tests/scenarios/core-resolver-test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/scenarios/core-resolver-test.ts b/tests/scenarios/core-resolver-test.ts index 4ffb40a39..1439d2225 100644 --- a/tests/scenarios/core-resolver-test.ts +++ b/tests/scenarios/core-resolver-test.ts @@ -119,6 +119,10 @@ Scenarios.fromProject(() => new Project()) }; }); + hooks.afterEach(() => { + expectAudit.hasNoProblems(); + }); + Qmodule('#embroider_compat', function () { test('js-only component', async function () { givenFiles({ @@ -134,10 +138,6 @@ Scenarios.fromProject(() => new Project()) .to('./components/hello-world.js'); }); - hooks.afterEach(() => { - expectAudit.hasNoProblems(); - }); - test('js-and-hbs component', async function () { givenFiles({ 'components/hello-world.js': 'export default function() {}', From f0b8aa418048442fa099998ea0ab27282ab5f37f Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Wed, 8 Mar 2023 22:05:06 -0500 Subject: [PATCH 09/26] run virtual-loader-plugin content through babel too --- packages/webpack/src/ember-webpack.ts | 22 +++++++++++-------- packages/webpack/src/virtual-loader.ts | 4 +++- .../webpack/src/webpack-resolver-plugin.ts | 21 ++++++++++-------- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/packages/webpack/src/ember-webpack.ts b/packages/webpack/src/ember-webpack.ts index 582ba4104..dee70c47e 100644 --- a/packages/webpack/src/ember-webpack.ts +++ b/packages/webpack/src/ember-webpack.ts @@ -197,6 +197,15 @@ const Webpack: PackagerConstructor = class Webpack implements Packager let { plugins: stylePlugins, loaders: styleLoaders } = this.setupStyleConfig(variant); + let babelLoaderOptions = makeBabelLoaderOptions( + babel.majorVersion, + variant, + join(this.pathToVanillaApp, babel.filename), + this.extraBabelLoaderOptions + ); + + let babelLoaderPrefix = `babel-loader-8?${JSON.stringify(babelLoaderOptions.options)}!`; + return { mode: variant.optimizeForProduction ? 'production' : 'development', context: this.pathToVanillaApp, @@ -206,7 +215,7 @@ const Webpack: PackagerConstructor = class Webpack implements Packager }, plugins: [ ...stylePlugins, - new EmbroiderPlugin(resolverConfig), + new EmbroiderPlugin(resolverConfig, babelLoaderPrefix), compiler => { compiler.hooks.done.tapPromise('EmbroiderPlugin', async stats => { this.summarizeStats(stats, variant, variantIndex); @@ -221,12 +230,7 @@ const Webpack: PackagerConstructor = class Webpack implements Packager test: /\.hbs$/, use: nonNullArray([ maybeThreadLoader(babel.isParallelSafe, this.extraThreadLoaderOptions), - babelLoaderOptions( - babel.majorVersion, - variant, - join(this.pathToVanillaApp, babel.filename), - this.extraBabelLoaderOptions - ), + babelLoaderOptions, { loader: require.resolve('@embroider/hbs-loader'), options: (() => { @@ -246,7 +250,7 @@ const Webpack: PackagerConstructor = class Webpack implements Packager test: require(join(this.pathToVanillaApp, babel.fileFilter)), use: nonNullArray([ maybeThreadLoader(babel.isParallelSafe, this.extraThreadLoaderOptions), - babelLoaderOptions( + makeBabelLoaderOptions( babel.majorVersion, variant, join(this.pathToVanillaApp, babel.filename), @@ -697,7 +701,7 @@ function nonNullArray(array: T[]): NonNullable[] { return array.filter(Boolean) as NonNullable[]; } -function babelLoaderOptions( +function makeBabelLoaderOptions( _majorVersion: 7, variant: Variant, appBabelConfigPath: string, diff --git a/packages/webpack/src/virtual-loader.ts b/packages/webpack/src/virtual-loader.ts index 690db8905..d42ea7149 100644 --- a/packages/webpack/src/virtual-loader.ts +++ b/packages/webpack/src/virtual-loader.ts @@ -3,7 +3,9 @@ import { LoaderContext } from 'webpack'; export default function virtualLoader(this: LoaderContext) { if (typeof this.query === 'string' && this.query[0] === '?') { - return virtualContent(this.query.slice(1)); + let filename = this.query.slice(1); + this.resourcePath = filename; + return virtualContent(filename); } throw new Error(`@embroider/webpack/src/virtual-loader received unexpected request: ${this.query}`); } diff --git a/packages/webpack/src/webpack-resolver-plugin.ts b/packages/webpack/src/webpack-resolver-plugin.ts index 6cea9e9e9..bde43ecda 100644 --- a/packages/webpack/src/webpack-resolver-plugin.ts +++ b/packages/webpack/src/webpack-resolver-plugin.ts @@ -14,13 +14,15 @@ export { EmbroiderResolverOptions as Options }; const virtualLoaderName = '@embroider/webpack/src/virtual-loader'; const virtualLoaderPath = resolve(__dirname, './virtual-loader.js'); -const virtualRequestPattern = new RegExp(`^${escapeRegExp(virtualLoaderPath)}\\?(?.+)!`); +const virtualRequestPattern = new RegExp(`${escapeRegExp(virtualLoaderPath)}\\?(?.+)!`); export class EmbroiderPlugin { #resolver: EmbroiderResolver; + #babelLoaderPrefix: string; - constructor(opts: EmbroiderResolverOptions) { + constructor(opts: EmbroiderResolverOptions, babelLoaderPrefix: string) { this.#resolver = new EmbroiderResolver(opts); + this.#babelLoaderPrefix = babelLoaderPrefix; } #addLoaderAlias(compiler: Compiler, name: string, alias: string) { @@ -44,7 +46,7 @@ export class EmbroiderPlugin { let adaptedResolve = getAdaptedResolve(defaultResolve); nmf.hooks.resolve.tapAsync({ name: '@embroider/webpack', stage: 50 }, (state: unknown, callback: CB) => { - let request = WebpackModuleRequest.from(state); + let request = WebpackModuleRequest.from(state, this.#babelLoaderPrefix); if (!request) { defaultResolve(state, callback); return; @@ -107,7 +109,7 @@ class WebpackModuleRequest implements ModuleRequest { specifier: string; fromFile: string; - static from(state: any): WebpackModuleRequest | undefined { + static from(state: any, babelLoaderPrefix: string): WebpackModuleRequest | undefined { // when the files emitted from our virtual-loader try to import things, // those requests show in webpack as having no issuer. But we can see here // which requests they are and adjust the issuer so they resolve things from @@ -127,14 +129,15 @@ class WebpackModuleRequest implements ModuleRequest { typeof state.context === 'string' && typeof state.contextInfo?.issuer === 'string' && state.contextInfo.issuer !== '' && - !state.request.startsWith(virtualLoaderName) && // prevents recursion on requests we have already sent to our virtual loader + !state.request.includes(virtualLoaderName) && // prevents recursion on requests we have already sent to our virtual loader !state.request.startsWith('!') // ignores internal webpack resolvers ) { - return new WebpackModuleRequest(state); + return new WebpackModuleRequest(babelLoaderPrefix, state); } } constructor( + private babelLoaderPrefix: string, public state: { request: string; context: string; @@ -155,15 +158,15 @@ class WebpackModuleRequest implements ModuleRequest { alias(newSpecifier: string) { this.state.request = newSpecifier; - return new WebpackModuleRequest(this.state) as this; + return new WebpackModuleRequest(this.babelLoaderPrefix, this.state) as this; } rehome(newFromFile: string) { this.state.contextInfo.issuer = newFromFile; this.state.context = dirname(newFromFile); - return new WebpackModuleRequest(this.state) as this; + return new WebpackModuleRequest(this.babelLoaderPrefix, this.state) as this; } virtualize(filename: string) { - let next = this.alias(`${virtualLoaderName}?${filename}!`); + let next = this.alias(`${this.babelLoaderPrefix}${virtualLoaderName}?${filename}!`); next.isVirtual = true; return next; } From 4422c09c154157d2dfc5240e2e9f7b5870cd65f9 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 9 Mar 2023 15:05:44 -0500 Subject: [PATCH 10/26] updating module-resolver rules to account for removal of on-disk app-tree merging --- packages/compat/src/compat-app.ts | 8 --- packages/compat/tests/audit.test.ts | 1 - packages/core/src/app-differ.ts | 21 ++----- packages/core/src/app-files.ts | 9 --- packages/core/src/module-resolver.ts | 81 ++++++++++--------------- tests/scenarios/compat-renaming-test.ts | 22 ++++--- tests/scenarios/compat-resolver-test.ts | 1 - tests/scenarios/core-resolver-test.ts | 29 ++++++++- 8 files changed, 78 insertions(+), 94 deletions(-) diff --git a/packages/compat/src/compat-app.ts b/packages/compat/src/compat-app.ts index 0309d1d25..bfa83fa44 100644 --- a/packages/compat/src/compat-app.ts +++ b/packages/compat/src/compat-app.ts @@ -354,19 +354,11 @@ class CompatAppAdapter implements AppAdapter { activeAddons[addon.name] = addon.root; } - let relocatedFiles: CompatResolverOptions['relocatedFiles'] = {}; - for (let { destPath, appFiles } of engines) { - for (let [relativePath, originalPath] of appFiles.relocatedFiles) { - relocatedFiles[join(destPath, relativePath)] = originalPath; - } - } - let config: CompatResolverOptions = { // this part is the base ModuleResolverOptions as required by @embroider/core activeAddons, renameModules, renamePackages, - relocatedFiles, resolvableExtensions: this.resolvableExtensions(), appRoot: this.root, engines: engines.map(engine => ({ diff --git a/packages/compat/tests/audit.test.ts b/packages/compat/tests/audit.test.ts index 1d90d0b2d..17b1bb7c8 100644 --- a/packages/compat/tests/audit.test.ts +++ b/packages/compat/tests/audit.test.ts @@ -47,7 +47,6 @@ describe('audit', function () { root: app.baseDir, }, ], - relocatedFiles: {}, resolvableExtensions, }; diff --git a/packages/core/src/app-differ.ts b/packages/core/src/app-differ.ts index f800b76dc..eaf1d2eb0 100644 --- a/packages/core/src/app-differ.ts +++ b/packages/core/src/app-differ.ts @@ -16,13 +16,8 @@ export default class AppDiffer { private sources: Source[]; private firstFastbootTree = Infinity; - // maps from each filename in the app to the original directory from whence it - // came, if it came from an addon. The mapping allows us to preserve - // resolution semantics so that each of the app files can still resolve - // relative to where it was authored. - // - // files authored within the app map to null - readonly files: Map = new Map(); + // set of filenames logically located in the app + readonly files: Set = new Set(); // true for files that are fastboot-only. isFastbootOnly: Map = new Map(); @@ -110,7 +105,7 @@ export default class AppDiffer { if (!source.isRelocated) { copySync(sourceFile, outputPath, { dereference: true }); } - this.updateFiles(relativePath, source, sourceFile); + this.updateFiles(relativePath); } else { // we have both fastboot and non-fastboot files for this path. // Because of the way fastbootMerge is written, the first one is the @@ -132,7 +127,7 @@ export default class AppDiffer { switcher(browserDest, fastbootDest, this.babelParserConfig!, readFileSync(browserSourceFile, 'utf8')) ); } - this.updateFiles(relativePath, browserSrc, browserSourceFile); + this.updateFiles(relativePath); } break; default: @@ -141,12 +136,8 @@ export default class AppDiffer { } } - private updateFiles(relativePath: string, source: Source, sourceFile: string) { - if (source.isRelocated) { - this.files.set(relativePath, sourceFile); - } else { - this.files.set(relativePath, null); - } + private updateFiles(relativePath: string) { + this.files.add(relativePath); } } diff --git a/packages/core/src/app-files.ts b/packages/core/src/app-files.ts index 0dcd8175b..912c58708 100644 --- a/packages/core/src/app-files.ts +++ b/packages/core/src/app-files.ts @@ -16,7 +16,6 @@ export class AppFiles { readonly modifiers: ReadonlyArray; private perRoute: RouteFiles; readonly otherAppFiles: ReadonlyArray; - readonly relocatedFiles: Map; readonly isFastbootOnly: Map; constructor(appDiffer: AppDiffer, resolvableExtensions: RegExp, podModulePrefix?: string) { @@ -83,14 +82,6 @@ export class AppFiles { this.helpers = helpers; this.modifiers = modifiers; this.otherAppFiles = otherAppFiles; - - let relocatedFiles: Map = new Map(); - for (let [relativePath, owningPath] of appDiffer.files) { - if (owningPath) { - relocatedFiles.set(relativePath, owningPath); - } - } - this.relocatedFiles = relocatedFiles; this.isFastbootOnly = appDiffer.isFastbootOnly; } diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index c82ff87f3..b11592916 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -55,7 +55,6 @@ export interface Options { activeAddons: { [packageName: string]: string; }; - relocatedFiles: { [relativePath: string]: string }; resolvableExtensions: string[]; appRoot: string; engines: EngineConfig[]; @@ -272,15 +271,16 @@ export class Resolver { return PackageCache.shared('embroider-stage3', this.options.appRoot).ownerOfFile(fromFile); } - private originalPackage(fromFile: string): V2Package | undefined { - let originalFile = this.options.relocatedFiles[fromFile]; - if (originalFile) { - let owning = PackageCache.shared('embroider-stage3', this.options.appRoot).ownerOfFile(originalFile); - if (owning && !owning.isV2Ember()) { - throw new Error(`bug: it should only be possible for a v2 ember package to own relocated files`); + private logicalPackage(owningPackage: V2Package, file: string): V2Package { + let logicalLocation = this.reverseSearchAppTree(owningPackage, file); + if (logicalLocation) { + let pkg = PackageCache.shared('embroider-stage3', this.options.appRoot).get(logicalLocation.owningEngine.root); + if (!pkg.isV2Ember()) { + throw new Error(`bug: all engines should be v2 addons by the time we see them here`); } - return owning; + return pkg; } + return owningPackage; } private handleFastbootCompat(request: R): R { @@ -620,13 +620,6 @@ export class Resolver { return logTransition(`v1 self-import`, request, this.resolveWithinPackage(request, pkg)); } - let originalPkg = this.originalPackage(request.fromFile); - if (originalPkg && pkg.meta['auto-upgraded'] && originalPkg.name === packageName) { - // A file that was relocated out of a package is importing that package's - // name, it should find its own original copy. - return logTransition(`self-import in app-js`, request, this.resolveWithinPackage(request, originalPkg)); - } - return request; } @@ -707,9 +700,14 @@ export class Resolver { return logTransition(`emberVirtualPeerDeps in v2 addon`, request, request.rehome(newHome)); } - if (pkg.meta['auto-upgraded'] && !pkg.hasDependency('ember-auto-import')) { + // if this file is part of an addon's app-js, it's really the logical + // package to which it belongs (normally the app) that affects some policy + // choices about what it can import + let logicalPackage = this.logicalPackage(pkg, fromFile); + + if (logicalPackage.meta['auto-upgraded'] && !logicalPackage.hasDependency('ember-auto-import')) { try { - let dep = PackageCache.shared('embroider-stage3', this.options.appRoot).resolve(packageName, pkg); + let dep = PackageCache.shared('embroider-stage3', this.options.appRoot).resolve(packageName, logicalPackage); if (!dep.isEmberPackage()) { // classic ember addons can only import non-ember dependencies if they // have ember-auto-import. @@ -724,23 +722,10 @@ export class Resolver { // assertions on what native v2 addons can import if (!pkg.meta['auto-upgraded']) { - let originalPkg = this.originalPackage(fromFile); - if (originalPkg) { - // this file has been moved into another package (presumably the app). - if (packageName !== pkg.name) { - // the only thing that native v2 addons are allowed to import from - // within the app tree is their own name. - throw new Error( - `${pkg.name} is trying to import ${packageName} from within its app tree. This is unsafe, because ${pkg.name} can't control which dependencies are resolvable from the app` - ); - } - } else { - // this file has not been moved. The normal case. - if (!pkg.meta['auto-upgraded'] && !reliablyResolvable(pkg, packageName)) { - throw new Error( - `${pkg.name} is trying to import from ${packageName} but that is not one of its explicit dependencies` - ); - } + if (!pkg.meta['auto-upgraded'] && !reliablyResolvable(pkg, packageName)) { + throw new Error( + `${pkg.name} is trying to import from ${packageName} but that is not one of its explicit dependencies` + ); } } return request; @@ -795,13 +780,6 @@ export class Resolver { } } - let originalPkg = this.originalPackage(fromFile); - if (originalPkg) { - // we didn't find it from the original package, so try from the relocated - // package - return logTransition(`relocation fallback`, request, request.rehome(resolve(originalPkg.root, 'package.json'))); - } - // auto-upgraded packages can fall back to the set of known active addons // // v2 packages can fall back to the set of known active addons only to find @@ -832,7 +810,7 @@ export class Resolver { // next try to resolve it from the corresponding logical location in the // app. return logTransition( - 'fallbackResolve: retry from original home of relocated file', + 'fallbackResolve: retry from logical home of app-js file', request, request.rehome(resolve(logicalLocation.owningEngine.root, logicalLocation.inAppName)) ); @@ -892,16 +870,21 @@ export class Resolver { // check whether the given file with the given owningPackage is an addon's // appTree, and if so return the notional location within the app (or owning // engine) that it "logically" lives at. - private reverseSearchAppTree(owningPackage: Package, fromFile: string) { + private reverseSearchAppTree( + owningPackage: Package, + fromFile: string + ): { owningEngine: EngineConfig; inAppName: string } | undefined { // if the requesting file is in an addon's app-js, the request should // really be understood as a request for a module in the containing engine if (owningPackage.isV2Addon()) { - let appJS = owningPackage.meta['app-js']; - if (appJS) { - let fromPackageRelativePath = explicitRelative(owningPackage.root, fromFile); - for (let [inAppName, inAddonName] of Object.entries(appJS)) { - if (inAddonName === fromPackageRelativePath) { - return { owningEngine: this.owningEngine(owningPackage), inAppName }; + let sections = [owningPackage.meta['app-js'], owningPackage.meta['fastboot-js']]; + for (let section of sections) { + if (section) { + let fromPackageRelativePath = explicitRelative(owningPackage.root, fromFile); + for (let [inAppName, inAddonName] of Object.entries(section)) { + if (inAddonName === fromPackageRelativePath) { + return { owningEngine: this.owningEngine(owningPackage), inAppName }; + } } } } diff --git a/tests/scenarios/compat-renaming-test.ts b/tests/scenarios/compat-renaming-test.ts index 2b010ba98..0d5d04653 100644 --- a/tests/scenarios/compat-renaming-test.ts +++ b/tests/scenarios/compat-renaming-test.ts @@ -267,31 +267,33 @@ appScenarios /single file package/ ); }); - test('files copied into app from addons resolve their own original packages', function () { + test('files logically copied into app from addons resolve their own original packages', function () { expectAudit - .module('./first.js') + .module('./node_modules/has-app-tree-import/_app_/first.js') .resolves('has-app-tree-import') .to('./node_modules/has-app-tree-import/index.js'); expectAudit - .module('./second.js') + .module('./node_modules/intermediate/node_modules/has-app-tree-import/_app_/second.js') .resolves('has-app-tree-import') .to('./node_modules/intermediate/node_modules/has-app-tree-import/index.js'); }); - test(`files copied into app from addons resolve the addon's deps`, function () { + test(`files logically copied into app from addons resolve the addon's deps`, function () { expectAudit - .module('./imports-dep.js') + .module('./node_modules/has-app-tree-import/_app_/imports-dep.js') .resolves('inner-dep') .to('./node_modules/has-app-tree-import/node_modules/inner-dep/index.js'); }); - test(`app-tree files from addons that import from the app get rewritten to relative imports`, function () { + test(`app-tree files from addons can import from the app`, function () { expectAudit - .module('./mirage/config.js') + .module('./node_modules/mirage-like/_app_/mirage/config.js') .resolves('app-template/components/import-lodash') .to('./components/import-lodash.js'); }); - test(`files copied into app from addons can resolve the app's deps`, function () { - let assertFile = expectFile('mirage/config.js').transform(build.transpile); - assertFile.matches(/import ['"]a-library['"]/); + test(`files logically copied into app from addons can resolve the app's deps`, function () { + expectAudit + .module('./node_modules/mirage-like/_app_/mirage/config.js') + .resolves('a-library') + .to('./node_modules/a-library/index.js'); }); }); }); diff --git a/tests/scenarios/compat-resolver-test.ts b/tests/scenarios/compat-resolver-test.ts index 31d07b88d..59fb50c77 100644 --- a/tests/scenarios/compat-resolver-test.ts +++ b/tests/scenarios/compat-resolver-test.ts @@ -72,7 +72,6 @@ Scenarios.fromProject(() => new Project()) activeAddons: {}, renameModules: {}, renamePackages: {}, - relocatedFiles: {}, resolvableExtensions: ['.js', '.hbs'], appRoot: app.dir, engines: [ diff --git a/tests/scenarios/core-resolver-test.ts b/tests/scenarios/core-resolver-test.ts index 1439d2225..410a0f560 100644 --- a/tests/scenarios/core-resolver-test.ts +++ b/tests/scenarios/core-resolver-test.ts @@ -32,6 +32,16 @@ Scenarios.fromProject(() => new Project()) app.mergeFiles({ 'index.html': '', }); + app.addDependency('the-apps-dep', { + files: { + 'index.js': '', + }, + }); + + // this is just an empty fixture package, it's the presence of a dependency + // named ember-auto-import that tells us that the app was allowed to import + // deps from npm. + app.addDependency('ember-auto-import', { version: '2.0.0' }); }) .forEachScenario(scenario => { Qmodule(scenario.name, function (hooks) { @@ -60,7 +70,6 @@ Scenarios.fromProject(() => new Project()) activeAddons: {}, renameModules: {}, renamePackages: opts?.renamePackages ?? {}, - relocatedFiles: {}, resolvableExtensions: ['.js', '.hbs'], appRoot: app.dir, engines: [ @@ -482,6 +491,24 @@ Scenarios.fromProject(() => new Project()) .to('./secondary.js'); }); + test(`classic addon's app tree can resolve app's dependencies`, async function () { + givenFiles({ + 'node_modules/my-addon/_app_/hello-world.js': `import "the-apps-dep"`, + 'app.js': `import "my-app/hello-world"`, + }); + + await configure({ + addonMeta: { + 'app-js': { './hello-world.js': './_app_/hello-world.js' }, + }, + }); + + expectAudit + .module('./node_modules/my-addon/_app_/hello-world.js') + .resolves('the-apps-dep') + .to('./node_modules/the-apps-dep/index.js'); + }); + test(`absolute import in addon's app tree resolves to app`, async function () { givenFiles({ 'node_modules/my-addon/_app_/hello-world.js': `import "my-app/secondary"`, From f28db6ebf3e20e433312d2b22b850916adfc4ce9 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 9 Mar 2023 15:22:58 -0500 Subject: [PATCH 11/26] update some tests and always resolve symlinks in our nodeResolve --- packages/core/src/module-resolver.ts | 3 ++- tests/scenarios/compat-stage2-test.ts | 30 +++++++++++++++++++-------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index b11592916..91bcf35d9 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -18,7 +18,7 @@ import { } from './virtual-content'; import { Memoize } from 'typescript-memoize'; import { describeExports } from './describe-exports'; -import { readFileSync } from 'fs'; +import { readFileSync, realpathSync } from 'fs'; const debug = makeDebug('embroider:resolver'); function logTransition(reason: string, before: R, after: R = before): R { @@ -249,6 +249,7 @@ export class Resolver { basedir: dirname(request.fromFile), extensions: this.options.resolvableExtensions, }); + filename = realpathSync(filename); return { type: 'found', result: { type: 'real' as 'real', filename } }; } catch (err) { if (err.code !== 'MODULE_NOT_FOUND') { diff --git a/tests/scenarios/compat-stage2-test.ts b/tests/scenarios/compat-stage2-test.ts index d8c715cb1..1854a6e8b 100644 --- a/tests/scenarios/compat-stage2-test.ts +++ b/tests/scenarios/compat-stage2-test.ts @@ -53,6 +53,11 @@ stage2Scenarios null, 2 ), + app: { + services: { + 'primary.js': `import "secondary-in-repo-addon/components/secondary"`, + }, + }, }); // critically, this in-repo addon gets removed from the app's actual @@ -90,6 +95,8 @@ stage2Scenarios expectFile = expectFilesAt(readFileSync(join(app.dir, 'dist/.stage2-output'), 'utf8'), { qunit: assert }); }); + let expectAudit = setupAuditTest(hooks, () => app.dir); + test('in repo addons are symlinked correctly', function () { // check that package json contains in repo dep expectFile('./node_modules/dep-a/package.json').json().get('dependencies.in-repo-a').equals('0.0.0'); @@ -116,19 +123,24 @@ stage2Scenarios .equals(2); // check that the app trees with in repo addon are combined correctly - expectFile('./service/in-repo.js').matches(/in-repo-c/); + expectAudit + .module('./assets/my-app.js') + .resolves('../service/in-repo.js') + .to('./node_modules/dep-b/lib/in-repo-c/_app_/service/in-repo.js'); }); - test('incorporates in-repo-addons of in-repo-addons correctly', function (assert) { + test('incorporates in-repo-addons of in-repo-addons correctly', function () { // secondary in-repo-addon was correctly detected and activated - expectFile('./services/secondary.js').exists(); + expectAudit + .module('./assets/my-app.js') + .resolves('../services/secondary.js') + .to('./lib/secondary-in-repo-addon/_app_/services/secondary.js'); - assert.ok( - resolve.sync('secondary-in-repo-addon/components/secondary', { - basedir: join(expectFile.basePath, 'node_modules', 'primary-in-repo-addon'), - }), - 'secondary is resolvable from primary' - ); + // secondary is resolvable from primary + expectAudit + .module('./lib/primary-in-repo-addon/_app_/services/primary.js') + .resolves('secondary-in-repo-addon/components/secondary') + .to('./lib/secondary-in-repo-addon/components/secondary.js'); }); }); }); From 36fb27fb7ce013538da704e2aafbe46db9444b52 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 9 Mar 2023 15:34:27 -0500 Subject: [PATCH 12/26] update more tests --- tests/scenarios/compat-stage2-test.ts | 32 ++++++++++++++++++--------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/tests/scenarios/compat-stage2-test.ts b/tests/scenarios/compat-stage2-test.ts index 1854a6e8b..9bca09f3c 100644 --- a/tests/scenarios/compat-stage2-test.ts +++ b/tests/scenarios/compat-stage2-test.ts @@ -185,7 +185,6 @@ stage2Scenarios throwOnWarnings(hooks); let app: PreparedApp; - let expectFile: ExpectFile; hooks.before(async assert => { app = await scenario.prepare(); @@ -193,30 +192,41 @@ stage2Scenarios assert.equal(result.exitCode, 0, result.output); }); - hooks.beforeEach(assert => { - expectFile = expectFilesAt(readFileSync(join(app.dir, 'dist/.stage2-output'), 'utf8'), { qunit: assert }); - }); + let expectAudit = setupAuditTest(hooks, () => app.dir); test('verifies that the correct lexigraphically sorted addons win', function () { - expectFile('./service/in-repo.js').matches(/in-repo-b/); - expectFile('./service/addon.js').matches(/dep-b/); - expectFile('./service/dev-addon.js').matches(/dev-c/); + let expectModule = expectAudit.module('./assets/my-app.js'); + expectModule.resolves('../service/in-repo.js').to('./lib/in-repo-b/_app_/service/in-repo.js'); + expectModule.resolves('../service/addon.js').to('./node_modules/dep-b/_app_/service/addon.js'); + expectModule.resolves('../service/dev-addon.js').to('./node_modules/dev-c/_app_/service/dev-addon.js'); }); test('addons declared as dependencies should win over devDependencies', function () { - expectFile('./service/dep-wins-over-dev.js').matches(/dep-b/); + expectAudit + .module('./assets/my-app.js') + .resolves('../service/dep-wins-over-dev.js') + .to('./node_modules/dep-b/_app_/service/dep-wins-over-dev.js'); }); test('in repo addons declared win over dependencies', function () { - expectFile('./service/in-repo-over-deps.js').matches(/in-repo-a/); + expectAudit + .module('./assets/my-app.js') + .resolves('../service/in-repo-over-deps.js') + .to('./lib/in-repo-a/_app_/service/in-repo-over-deps.js'); }); test('ordering with before specified', function () { - expectFile('./service/test-before.js').matches(/dev-d/); + expectAudit + .module('./assets/my-app.js') + .resolves('../service/test-before.js') + .to('./node_modules/dev-d/_app_/service/test-before.js'); }); test('ordering with after specified', function () { - expectFile('./service/test-after.js').matches(/dev-b/); + expectAudit + .module('./assets/my-app.js') + .resolves('../service/test-after.js') + .to('./node_modules/dev-b/_app_/service/test-after.js'); }); }); }); From 267eee9a5d796f9b3949a3f52aa8ab6f4807dcfb Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 10 Mar 2023 09:03:23 -0500 Subject: [PATCH 13/26] fix the ways appTemplates and appModules rules get applied now that the modules don't move their on-disk locations --- .../compat/src/babel-plugin-adjust-imports.ts | 15 ++++++++++++--- packages/core/src/module-resolver.ts | 3 +-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/packages/compat/src/babel-plugin-adjust-imports.ts b/packages/compat/src/babel-plugin-adjust-imports.ts index 20f2f849d..00cd0d3c6 100644 --- a/packages/compat/src/babel-plugin-adjust-imports.ts +++ b/packages/compat/src/babel-plugin-adjust-imports.ts @@ -1,4 +1,4 @@ -import { join } from 'path'; +import { join, resolve } from 'path'; import type { NodePath } from '@babel/traverse'; import type * as Babel from '@babel/core'; import type { types as t } from '@babel/core'; @@ -145,7 +145,14 @@ function preprocessExtraImports(config: CompatResolverOptions): ExtraImports { } if (rule.appModules) { for (let [filename, moduleRules] of Object.entries(rule.appModules)) { - expandDependsOnRules(config.appRoot, filename, moduleRules, extraImports); + for (let root of rule.roots) { + // 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 those we always organize with their treeForApp output + // in _app_. + expandDependsOnRules(resolve(root, '_app_'), filename, moduleRules, extraImports); + } } } if (rule.addonTemplates) { @@ -157,7 +164,9 @@ function preprocessExtraImports(config: CompatResolverOptions): ExtraImports { } if (rule.appTemplates) { for (let [filename, moduleRules] of Object.entries(rule.appTemplates)) { - expandInvokesRules(config.appRoot, filename, moduleRules, extraImports); + for (let root of rule.roots) { + expandInvokesRules(resolve(root, '_app_'), filename, moduleRules, extraImports); + } } } } diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index 91bcf35d9..b11592916 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -18,7 +18,7 @@ import { } from './virtual-content'; import { Memoize } from 'typescript-memoize'; import { describeExports } from './describe-exports'; -import { readFileSync, realpathSync } from 'fs'; +import { readFileSync } from 'fs'; const debug = makeDebug('embroider:resolver'); function logTransition(reason: string, before: R, after: R = before): R { @@ -249,7 +249,6 @@ export class Resolver { basedir: dirname(request.fromFile), extensions: this.options.resolvableExtensions, }); - filename = realpathSync(filename); return { type: 'found', result: { type: 'real' as 'real', filename } }; } catch (err) { if (err.code !== 'MODULE_NOT_FOUND') { From e95836808c36d8199a113e74259652e0a559e3ec Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 10 Mar 2023 09:21:26 -0500 Subject: [PATCH 14/26] update tests to see symlinks --- tests/scenarios/compat-stage2-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scenarios/compat-stage2-test.ts b/tests/scenarios/compat-stage2-test.ts index 9bca09f3c..a8dd9a4ec 100644 --- a/tests/scenarios/compat-stage2-test.ts +++ b/tests/scenarios/compat-stage2-test.ts @@ -140,7 +140,7 @@ stage2Scenarios expectAudit .module('./lib/primary-in-repo-addon/_app_/services/primary.js') .resolves('secondary-in-repo-addon/components/secondary') - .to('./lib/secondary-in-repo-addon/components/secondary.js'); + .to('./lib/primary-in-repo-addon/node_modules/secondary-in-repo-addon/components/secondary.js'); }); }); }); From 73c7fb72aebc79f76294f9c59d48e8065acc597b Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 10 Mar 2023 09:29:13 -0500 Subject: [PATCH 15/26] make appTemplates rules apply correctly in apps, not just addons --- .../compat/src/babel-plugin-adjust-imports.ts | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/packages/compat/src/babel-plugin-adjust-imports.ts b/packages/compat/src/babel-plugin-adjust-imports.ts index 00cd0d3c6..01eb955d2 100644 --- a/packages/compat/src/babel-plugin-adjust-imports.ts +++ b/packages/compat/src/babel-plugin-adjust-imports.ts @@ -43,10 +43,11 @@ export default function main(babel: typeof Babel) { return cached; } let resolverOptions: CompatResolverOptions = readJSONSync(join(appRoot, '.embroider', 'resolver.json')); + let resolver = new Resolver(resolverOptions); cached = { resolverOptions, - resolver: new Resolver(resolverOptions), - extraImports: preprocessExtraImports(resolverOptions), + resolver, + extraImports: preprocessExtraImports(resolverOptions, resolver), componentExtraImports: preprocessComponentExtraImports(resolverOptions), }; return cached; @@ -133,7 +134,7 @@ function amdDefine(t: BabelTypes, adder: ImportUtil, path: NodePath, ); } -function preprocessExtraImports(config: CompatResolverOptions): ExtraImports { +function preprocessExtraImports(config: CompatResolverOptions, resolver: Resolver): ExtraImports { let extraImports: ExtraImports = {}; for (let rule of config.activePackageRules) { if (rule.addonModules) { @@ -151,7 +152,7 @@ function preprocessExtraImports(config: CompatResolverOptions): ExtraImports { // But this code is only for applying packageRules to auto-upgraded v1 // addons, and those we always organize with their treeForApp output // in _app_. - expandDependsOnRules(resolve(root, '_app_'), filename, moduleRules, extraImports); + expandDependsOnRules(appTreeDir(root, resolver), filename, moduleRules, extraImports); } } } @@ -165,7 +166,7 @@ function preprocessExtraImports(config: CompatResolverOptions): ExtraImports { if (rule.appTemplates) { for (let [filename, moduleRules] of Object.entries(rule.appTemplates)) { for (let root of rule.roots) { - expandInvokesRules(resolve(root, '_app_'), filename, moduleRules, extraImports); + expandInvokesRules(appTreeDir(root, resolver), filename, moduleRules, extraImports); } } } @@ -173,6 +174,20 @@ function preprocessExtraImports(config: CompatResolverOptions): ExtraImports { return extraImports; } +function appTreeDir(root: string, resolver: Resolver) { + let pkg = resolver.owningPackage(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; + } +} + function lazyPackageLookup(config: InternalConfig, filename: string) { let owningPackage: { result: Package | undefined } | undefined; let owningEngine: { result: ReturnType | undefined } | undefined; From 21deccd2640d099fe34370b2b386af3e6f2d2132 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 10 Mar 2023 10:13:49 -0500 Subject: [PATCH 16/26] update the invokes rules matching too, and update more tests --- .../compat/src/babel-plugin-adjust-imports.ts | 22 ++++--------------- packages/compat/src/dependency-rules.ts | 17 +++++++++++++- packages/compat/src/resolver-transform.ts | 5 ++++- tests/scenarios/compat-stage2-test.ts | 16 +++++++------- 4 files changed, 32 insertions(+), 28 deletions(-) diff --git a/packages/compat/src/babel-plugin-adjust-imports.ts b/packages/compat/src/babel-plugin-adjust-imports.ts index 01eb955d2..4eb0447de 100644 --- a/packages/compat/src/babel-plugin-adjust-imports.ts +++ b/packages/compat/src/babel-plugin-adjust-imports.ts @@ -1,4 +1,4 @@ -import { join, resolve } from 'path'; +import { join } from 'path'; import type { NodePath } from '@babel/traverse'; import type * as Babel from '@babel/core'; import type { types as t } from '@babel/core'; @@ -7,7 +7,7 @@ import { readJSONSync } from 'fs-extra'; import { CompatResolverOptions } from './resolver-transform'; import { Package, packageName, Resolver, unrelativize } from '@embroider/core'; import { snippetToDasherizedName } from './dasherize-component-name'; -import { ActivePackageRules, ComponentRules, ModuleRules, TemplateRules } from './dependency-rules'; +import { ActivePackageRules, appTreeRulesDir, ComponentRules, ModuleRules, TemplateRules } from './dependency-rules'; export type Options = { appRoot: string }; @@ -152,7 +152,7 @@ function preprocessExtraImports(config: CompatResolverOptions, resolver: Resolve // But this code is only for applying packageRules to auto-upgraded v1 // addons, and those we always organize with their treeForApp output // in _app_. - expandDependsOnRules(appTreeDir(root, resolver), filename, moduleRules, extraImports); + expandDependsOnRules(appTreeRulesDir(root, resolver), filename, moduleRules, extraImports); } } } @@ -166,7 +166,7 @@ function preprocessExtraImports(config: CompatResolverOptions, resolver: Resolve if (rule.appTemplates) { for (let [filename, moduleRules] of Object.entries(rule.appTemplates)) { for (let root of rule.roots) { - expandInvokesRules(appTreeDir(root, resolver), filename, moduleRules, extraImports); + expandInvokesRules(appTreeRulesDir(root, resolver), filename, moduleRules, extraImports); } } } @@ -174,20 +174,6 @@ function preprocessExtraImports(config: CompatResolverOptions, resolver: Resolve return extraImports; } -function appTreeDir(root: string, resolver: Resolver) { - let pkg = resolver.owningPackage(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; - } -} - function lazyPackageLookup(config: InternalConfig, filename: string) { let owningPackage: { result: Package | undefined } | undefined; let owningEngine: { result: ReturnType | undefined } | undefined; diff --git a/packages/compat/src/dependency-rules.ts b/packages/compat/src/dependency-rules.ts index cf020706d..6a194a9b0 100644 --- a/packages/compat/src/dependency-rules.ts +++ b/packages/compat/src/dependency-rules.ts @@ -1,4 +1,5 @@ -import { Package, getOrCreate } from '@embroider/core'; +import { Package, getOrCreate, Resolver } from '@embroider/core'; +import { resolve } from 'path'; import { satisfies } from 'semver'; export interface PackageRules { @@ -224,3 +225,17 @@ export function activePackageRules(packageRules: PackageRules[], activePackages: } return output; } + +export function appTreeRulesDir(root: string, resolver: Resolver) { + let pkg = resolver.owningPackage(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; + } +} diff --git a/packages/compat/src/resolver-transform.ts b/packages/compat/src/resolver-transform.ts index 0b7b5b9a6..ee3c3b112 100644 --- a/packages/compat/src/resolver-transform.ts +++ b/packages/compat/src/resolver-transform.ts @@ -6,6 +6,7 @@ import { ComponentRules, PackageRules, ModuleRules, + appTreeRulesDir, } from './dependency-rules'; import { Memoize } from 'typescript-memoize'; import type { WithJSUtils } from 'babel-plugin-ember-template-compilation'; @@ -323,7 +324,9 @@ class TemplateResolver implements ASTPlugin { if (rule.appTemplates) { for (let [path, templateRules] of Object.entries(rule.appTemplates)) { let processedRules = preprocessComponentRule(templateRules); - files.set(join(this.config.appRoot, path), processedRules); + for (let root of rule.roots) { + files.set(join(appTreeRulesDir(root, this.moduleResolver), path), processedRules); + } } } if (rule.addonTemplates) { diff --git a/tests/scenarios/compat-stage2-test.ts b/tests/scenarios/compat-stage2-test.ts index a8dd9a4ec..743ee1884 100644 --- a/tests/scenarios/compat-stage2-test.ts +++ b/tests/scenarios/compat-stage2-test.ts @@ -500,7 +500,7 @@ stage2Scenarios expectModule .resolves('#embroider_compat/components/hello-world') - .to('./components/hello-world.js', 'explicit dependency'); + .to('./node_modules/my-addon/_app_/components/hello-world.js', 'explicit dependency'); expectModule .resolves('#embroider_compat/components/third-choice') @@ -522,7 +522,7 @@ stage2Scenarios let expectModule = expectAudit.module('./templates/curly.hbs'); expectModule .resolves('#embroider_compat/ambiguous/hello-world') - .to('./components/hello-world.js', 'explicit dependency'); + .to('./node_modules/my-addon/_app_/components/hello-world.js', 'explicit dependency'); expectModule .resolves('#embroider_compat/components/third-choice') .toModule() @@ -556,7 +556,7 @@ stage2Scenarios }); test('app/hello-world.js', function () { - expectAudit.module('./components/hello-world.js').codeEquals(` + expectAudit.module('./node_modules/my-addon/_app_/components/hello-world.js').codeEquals(` window.define("my-addon/synthetic-import-1", function () { return importSync("my-addon/synthetic-import-1"); }); @@ -565,14 +565,14 @@ stage2Scenarios `); expectAudit - .module('./components/hello-world.js') + .module('./node_modules/my-addon/_app_/components/hello-world.js') .resolves('my-addon/components/hello-world') .to('./node_modules/my-addon/components/hello-world.js', 'remapped to precise copy of my-addon'); }); test('app/templates/components/direct-template-reexport.js', function () { expectAudit - .module('./templates/components/direct-template-reexport.js') + .module('./node_modules/my-addon/_app_/templates/components/direct-template-reexport.js') .resolves('my-addon/templates/components/hello-world') .to('./node_modules/my-addon/templates/components/hello-world.hbs', 'rewrites reexports of templates'); }); @@ -699,16 +699,16 @@ stage2Scenarios test('invokes rule on appTemplates produces synthetic import', function () { expectAudit - .module('./templates/app-example.hbs') + .module('./node_modules/my-addon/_app_/templates/app-example.hbs') .resolves('#embroider_compat/components/synthetic-import2') - .to('./components/synthetic-import2.js'); + .to('./node_modules/my-addon/_app_/components/synthetic-import2.js'); }); test('invokes rule on addonTemplates produces synthetic import', function () { expectAudit .module('./node_modules/my-addon/templates/addon-example.hbs') .resolves('#embroider_compat/components/synthetic-import2') - .to('./components/synthetic-import2.js'); + .to('./node_modules/my-addon/_app_/components/synthetic-import2.js'); }); }); }); From 43d0b972d3de95099ec952c5ac3e6dd774e4eb59 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 10 Mar 2023 11:24:37 -0500 Subject: [PATCH 17/26] fix rule matching within (moved) app --- packages/compat/src/compat-app.ts | 2 +- packages/compat/src/dependency-rules.ts | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/compat/src/compat-app.ts b/packages/compat/src/compat-app.ts index bfa83fa44..3ce706965 100644 --- a/packages/compat/src/compat-app.ts +++ b/packages/compat/src/compat-app.ts @@ -317,7 +317,7 @@ class CompatAppAdapter implements AppAdapter { @Memoize() private activeRules() { return activePackageRules(this.options.packageRules.concat(defaultAddonPackageRules()), [ - this.appPackage, + { name: this.appPackage.name, version: this.appPackage.version, root: this.root }, ...this.allActiveAddons.filter(p => p.meta['auto-upgraded']), ]); } diff --git a/packages/compat/src/dependency-rules.ts b/packages/compat/src/dependency-rules.ts index 6a194a9b0..9346dff52 100644 --- a/packages/compat/src/dependency-rules.ts +++ b/packages/compat/src/dependency-rules.ts @@ -1,4 +1,4 @@ -import { Package, getOrCreate, Resolver } from '@embroider/core'; +import { getOrCreate, Resolver } from '@embroider/core'; import { resolve } from 'path'; import { satisfies } from 'semver'; @@ -206,7 +206,10 @@ export function preprocessComponentRule(componentRules: ComponentRules): Preproc }; } -export function activePackageRules(packageRules: PackageRules[], activePackages: Package[]): ActivePackageRules[] { +export function activePackageRules( + packageRules: PackageRules[], + activePackages: { name: string; root: string; version: string }[] +): ActivePackageRules[] { // rule order implies precedence. The first rule that matches a given package // applies to that package, and no other rule does. let rootsPerRule = new Map(); From cf738a16089c3a147ec5b5646d8017fd6b951caf Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 10 Mar 2023 11:42:00 -0500 Subject: [PATCH 18/26] update more tests and add an assertion --- packages/core/src/module-resolver.ts | 31 +++++++++++++++++++++++++++- tests/scenarios/v2-addon-test.ts | 4 ++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index b11592916..bc9ac4d27 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -490,6 +490,16 @@ export class Resolver { let appJS = addon.meta['app-js']; if (appJS) { for (let [inEngineName, inAddonName] of Object.entries(appJS)) { + if (!inEngineName.startsWith('./')) { + throw new Error( + `addon ${addon.name} declares app-js in its package.json with the illegal name "${inEngineName}". It must start with "./" to make it clear that it's relative to the app` + ); + } + if (!inAddonName.startsWith('./')) { + throw new Error( + `addon ${addon.name} declares app-js in its package.json with the illegal name "${inAddonName}". It must start with "./" to make it clear that it's relative to the addon` + ); + } inEngineName = withoutJSExt(inEngineName); let prevEntry = engineModules.get(inEngineName); switch (prevEntry?.type) { @@ -525,6 +535,16 @@ export class Resolver { let fastbootJS = addon.meta['fastboot-js']; if (fastbootJS) { for (let [inEngineName, inAddonName] of Object.entries(fastbootJS)) { + if (!inEngineName.startsWith('./')) { + throw new Error( + `addon ${addon.name} declares fastboot-js in its package.json with the illegal name "${inEngineName}". It must start with "./" to make it clear that it's relative to the app` + ); + } + if (!inAddonName.startsWith('./')) { + throw new Error( + `addon ${addon.name} declares fastboot-js in its package.json with the illegal name "${inAddonName}". It must start with "./" to make it clear that it's relative to the addon` + ); + } inEngineName = withoutJSExt(inEngineName); let prevEntry = engineModules.get(inEngineName); switch (prevEntry?.type) { @@ -722,7 +742,11 @@ export class Resolver { // assertions on what native v2 addons can import if (!pkg.meta['auto-upgraded']) { - if (!pkg.meta['auto-upgraded'] && !reliablyResolvable(pkg, packageName)) { + if ( + !pkg.meta['auto-upgraded'] && + !appImportInAppTree(pkg, logicalPackage, packageName) && + !reliablyResolvable(pkg, packageName) + ) { throw new Error( `${pkg.name} is trying to import from ${packageName} but that is not one of its explicit dependencies` ); @@ -959,6 +983,11 @@ function reliablyResolvable(pkg: V2Package, packageName: string) { return false; } +// +function appImportInAppTree(inPackage: Package, inLogicalPackage: Package, importedPackageName: string): boolean { + return inPackage !== inLogicalPackage && importedPackageName === inLogicalPackage.name; +} + function external(label: string, request: R, specifier: string): R { let filename = virtualExternalModule(specifier); return logTransition(label, request, request.virtualize(filename)); diff --git a/tests/scenarios/v2-addon-test.ts b/tests/scenarios/v2-addon-test.ts index 77c7f8f3a..cca7f42f8 100644 --- a/tests/scenarios/v2-addon-test.ts +++ b/tests/scenarios/v2-addon-test.ts @@ -10,7 +10,7 @@ appScenarios let addon = baseV2Addon(); addon.pkg.name = 'v2-addon'; (addon.pkg as any)['ember-addon']['app-js']['./components/example-component.js'] = - 'app/components/example-component.js'; + './app/components/example-component.js'; merge(addon.files, { app: { components: { @@ -72,7 +72,7 @@ appScenarios // the inner v2 addon, which gets consumed by `intermediate` let inner = baseV2Addon(); inner.pkg.name = 'inner'; - (inner.pkg as any)['ember-addon']['app-js']['./components/inner.js'] = 'app/components/inner.js'; + (inner.pkg as any)['ember-addon']['app-js']['./components/inner.js'] = './app/components/inner.js'; merge(inner.files, { app: { components: { From c7db53f3c96e17849a418695f88214c9a6b92108 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 10 Mar 2023 12:27:17 -0500 Subject: [PATCH 19/26] fix engines config sharing and stop relativizing engine entrypoints --- packages/compat/src/compat-app.ts | 4 ++-- packages/core/src/app.ts | 19 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/packages/compat/src/compat-app.ts b/packages/compat/src/compat-app.ts index 3ce706965..bffc12e00 100644 --- a/packages/compat/src/compat-app.ts +++ b/packages/compat/src/compat-app.ts @@ -361,9 +361,9 @@ class CompatAppAdapter implements AppAdapter { renamePackages, resolvableExtensions: this.resolvableExtensions(), appRoot: this.root, - engines: engines.map(engine => ({ + engines: engines.map((engine, index) => ({ packageName: engine.package.name, - root: this.root, + root: index === 0 ? this.root : engine.package.root, // first engine is the app, which has been relocated to this.roto activeAddons: [...engine.addons] .map(a => ({ name: a.name, diff --git a/packages/core/src/app.ts b/packages/core/src/app.ts index 53806485a..df46df5ff 100644 --- a/packages/core/src/app.ts +++ b/packages/core/src/app.ts @@ -12,7 +12,7 @@ import { compile } from './js-handlebars'; import resolve from 'resolve'; import { Memoize } from 'typescript-memoize'; import { copySync, ensureDirSync, outputJSONSync, readJSONSync, statSync, unlinkSync, writeFileSync } from 'fs-extra'; -import { dirname, join, resolve as resolvePath, sep } from 'path'; +import { dirname, join, resolve as resolvePath, sep, posix } from 'path'; import { debug, warn } from './messages'; import sortBy from 'lodash/sortBy'; import flatten from 'lodash/flatten'; @@ -1183,7 +1183,7 @@ export class AppBuilder { } lazyRoutes.push({ names: routeNames, - path: this.importPaths(engine, routeEntrypoint, relativePath).buildtime, + path: this.importPaths(engine, routeEntrypoint).buildtime, }); } ); @@ -1192,8 +1192,8 @@ export class AppBuilder { let [fastboot, nonFastboot] = partition(excludeDotFiles(flatten(requiredAppFiles)), file => appFiles.isFastbootOnly.get(file) ); - let amdModules = nonFastboot.map(file => this.importPaths(engine, file, relativePath)); - let fastbootOnlyAmdModules = fastboot.map(file => this.importPaths(engine, file, relativePath)); + let amdModules = nonFastboot.map(file => this.importPaths(engine, file)); + let fastbootOnlyAmdModules = fastboot.map(file => this.importPaths(engine, file)); // this is a backward-compatibility feature: addons can force inclusion of // modules. @@ -1220,12 +1220,11 @@ export class AppBuilder { return this.adapter.modulePrefix(); } - private importPaths(engine: Engine, engineRelativePath: string, fromFile: string) { - let appRelativePath = join(engine.appRelativePath, engineRelativePath); + private importPaths(engine: Engine, engineRelativePath: string) { let noHBS = engineRelativePath.replace(this.resolvableExtensionsPattern, '').replace(/\.hbs$/, ''); return { runtime: `${engine.modulePrefix}/${noHBS}`, - buildtime: explicitRelative(dirname(fromFile), appRelativePath), + buildtime: posix.join(engine.package.name, engineRelativePath), }; } @@ -1235,8 +1234,8 @@ export class AppBuilder { let asset: InternalAsset = { kind: 'in-memory', source: routeEntryTemplate({ - files: nonFastboot.map(f => this.importPaths(engine, f, relativePath)), - fastbootOnlyFiles: fastboot.map(f => this.importPaths(engine, f, relativePath)), + files: nonFastboot.map(f => this.importPaths(engine, f)), + fastbootOnlyFiles: fastboot.map(f => this.importPaths(engine, f)), }), relativePath, }; @@ -1272,7 +1271,7 @@ export class AppBuilder { let { appFiles } = engine; for (let relativePath of appFiles.tests) { - amdModules.push(this.importPaths(engine, relativePath, myName)); + amdModules.push(this.importPaths(engine, relativePath)); } let source = entryTemplate({ From 7a631755b828c0101be4621c206a9d9797a4ee0e Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 13 Mar 2023 11:31:00 -0400 Subject: [PATCH 20/26] updating test --- tests/scenarios/compat-template-colocation-test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/scenarios/compat-template-colocation-test.ts b/tests/scenarios/compat-template-colocation-test.ts index c4bac5494..5a42221c4 100644 --- a/tests/scenarios/compat-template-colocation-test.ts +++ b/tests/scenarios/compat-template-colocation-test.ts @@ -294,13 +294,13 @@ appScenarios test(`app's pod components and templates are implicitly included correctly`, function () { let assertFile = expectFile('assets/my-app.js'); assertFile.matches( - /d\(["']my-app\/components\/pod-component\/component["'], function\(\)\s*\{\s*return i\(["']\.\.\/components\/pod-component\/component\.js['"]\);\}\)/ + /d\(["']my-app\/components\/pod-component\/component["'], function\(\)\s*\{\s*return i\(["']my-app\/components\/pod-component\/component\.js['"]\);\}\)/ ); assertFile.matches( - /d\(["']my-app\/components\/pod-component\/template["'], function\(\)\s*\{\s*return i\(["']\.\.\/components\/pod-component\/template\.hbs['"]\);\}\)/ + /d\(["']my-app\/components\/pod-component\/template["'], function\(\)\s*\{\s*return i\(["']my-app\/components\/pod-component\/template\.hbs['"]\);\}\)/ ); assertFile.matches( - /d\(["']my-app\/components\/template-only\/template["'], function\(\)\s*\{\s*return i\(["']\.\.\/components\/template-only\/template\.hbs['"]\);\s*\}/ + /d\(["']my-app\/components\/template-only\/template["'], function\(\)\s*\{\s*return i\(["']my-app\/components\/template-only\/template\.hbs['"]\);\s*\}/ ); }); }); From 5dc362eb0ce63bdb81340cae30807c609ddcc667 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 13 Mar 2023 11:33:39 -0400 Subject: [PATCH 21/26] updating tests --- tests/scenarios/compat-route-split-test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/scenarios/compat-route-split-test.ts b/tests/scenarios/compat-route-split-test.ts index 4f1726b32..26262fb21 100644 --- a/tests/scenarios/compat-route-split-test.ts +++ b/tests/scenarios/compat-route-split-test.ts @@ -139,7 +139,7 @@ splitScenarios }); test('dynamically imports the route entrypoint from the main entrypoint', function () { - expectFile('./assets/my-app.js').matches('import("./_route_/people.js")'); + expectFile('./assets/my-app.js').matches('import("my-app/assets/_route_/people.js")'); }); test('has split controllers in route entrypoint', function () { @@ -321,7 +321,7 @@ splitScenarios }); test('dynamically imports the route entrypoint from the main entrypoint', function () { - expectFile('./assets/my-app.js').matches('import("./_route_/people.js")'); + expectFile('./assets/my-app.js').matches('import("my-app/assets/_route_/people.js")'); }); test('has split controllers in route entrypoint', function () { From 838939bae16040b326ca3670b8cfc306dccad998 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 13 Mar 2023 11:41:01 -0400 Subject: [PATCH 22/26] standardized around non-relative implicit-modules --- packages/core/src/app.ts | 5 +--- packages/core/src/options.ts | 26 ------------------- tests/scenarios/compat-namespaced-app-test.ts | 4 +-- 3 files changed, 3 insertions(+), 32 deletions(-) diff --git a/packages/core/src/app.ts b/packages/core/src/app.ts index df46df5ff..8e84a11ab 100644 --- a/packages/core/src/app.ts +++ b/packages/core/src/app.ts @@ -1321,10 +1321,7 @@ export class AppBuilder { runtime = runtime.split(sep).join('/'); lazyModules.push({ runtime, - buildtime: - this.options.implicitModulesStrategy === 'packageNames' - ? join(packageName, name) - : explicitRelative(dirname(join(this.root, relativeTo)), join(addon.root, name)), + buildtime: posix.join(packageName, name), }); } } diff --git a/packages/core/src/options.ts b/packages/core/src/options.ts index b0532faf7..abc9b8af1 100644 --- a/packages/core/src/options.ts +++ b/packages/core/src/options.ts @@ -79,32 +79,6 @@ export default interface Options { // useMethod optionally lets you pick which property within the module to use. // If not provided, we use the module.exports itself. pluginHints?: { resolve: string[]; useMethod?: string }[]; - - // Our addons' implicit-modules and implicit-test-modules are not necessarily - // resolvable directly from the app, but the meaning of those - // backward-compatibility features is "the app should import this module". So - // we need some strategy for making them importable by the app. We can either - // turn those imports into complete relative paths, or leave them as package - // names: - // - // relativePaths: - // - // import('./node_modules/intermediate/node_modules/some-addon/thing.js') - // - // packageNames: - // - // import('some-addon/thing.js') - // - // When building under a tool like webpack, the relativePaths are safe and - // always work, although they can be uglier to look at when debugging in - // development. - // - // When building under a tool like snowpack, the package names can be easier - // to work with because you already have web-bundles per package, and can't - // necessarily address arbitrary places on the filesystem. - // - // Defaults to "relativePaths". - implicitModulesStrategy?: 'packageNames' | 'relativePaths'; } export function optionsWithDefaults(options?: Options): Required { diff --git a/tests/scenarios/compat-namespaced-app-test.ts b/tests/scenarios/compat-namespaced-app-test.ts index 1d1f07d8a..12e986797 100644 --- a/tests/scenarios/compat-namespaced-app-test.ts +++ b/tests/scenarios/compat-namespaced-app-test.ts @@ -45,11 +45,11 @@ appScenarios test(`imports within app js`, function () { let assertFile = expectFile('assets/@ef4/namespaced-app.js'); assertFile.matches( - /d\(["'"]my-addon\/my-implicit-module["'], function\(\)\{ return i\(["']\.\.\/\.\.\/node_modules\/my-addon\/my-implicit-module\.js["']\);/, + /d\(["'"]my-addon\/my-implicit-module["'], function\(\)\{ return i\(["']my-addon\/my-implicit-module\.js["']\);/, 'implicit-modules have correct paths' ); assertFile.matches( - /d\(["']@ef4\/namespaced-app\/app['"], function\(\)\{ return i\(['"]\.\.\/\.\.\/app\.js"\);\}\);/, + /d\(["']@ef4\/namespaced-app\/app['"], function\(\)\{ return i\(['"]@ef4\/namespaced-app\/app\.js"\);\}\);/, `app's own modules are correct` ); }); From ca7f0c669f1448d11cf0d97085ab1f0b74cd07ea Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 13 Mar 2023 11:43:53 -0400 Subject: [PATCH 23/26] update more tests --- tests/scenarios/compat-stage2-test.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/scenarios/compat-stage2-test.ts b/tests/scenarios/compat-stage2-test.ts index 743ee1884..447e672bc 100644 --- a/tests/scenarios/compat-stage2-test.ts +++ b/tests/scenarios/compat-stage2-test.ts @@ -125,7 +125,7 @@ stage2Scenarios // check that the app trees with in repo addon are combined correctly expectAudit .module('./assets/my-app.js') - .resolves('../service/in-repo.js') + .resolves('my-app/service/in-repo.js') .to('./node_modules/dep-b/lib/in-repo-c/_app_/service/in-repo.js'); }); @@ -133,7 +133,7 @@ stage2Scenarios // secondary in-repo-addon was correctly detected and activated expectAudit .module('./assets/my-app.js') - .resolves('../services/secondary.js') + .resolves('my-app/services/secondary.js') .to('./lib/secondary-in-repo-addon/_app_/services/secondary.js'); // secondary is resolvable from primary @@ -196,36 +196,36 @@ stage2Scenarios test('verifies that the correct lexigraphically sorted addons win', function () { let expectModule = expectAudit.module('./assets/my-app.js'); - expectModule.resolves('../service/in-repo.js').to('./lib/in-repo-b/_app_/service/in-repo.js'); - expectModule.resolves('../service/addon.js').to('./node_modules/dep-b/_app_/service/addon.js'); - expectModule.resolves('../service/dev-addon.js').to('./node_modules/dev-c/_app_/service/dev-addon.js'); + expectModule.resolves('my-app/service/in-repo.js').to('./lib/in-repo-b/_app_/service/in-repo.js'); + expectModule.resolves('my-app/service/addon.js').to('./node_modules/dep-b/_app_/service/addon.js'); + expectModule.resolves('my-app/service/dev-addon.js').to('./node_modules/dev-c/_app_/service/dev-addon.js'); }); test('addons declared as dependencies should win over devDependencies', function () { expectAudit .module('./assets/my-app.js') - .resolves('../service/dep-wins-over-dev.js') + .resolves('my-app/service/dep-wins-over-dev.js') .to('./node_modules/dep-b/_app_/service/dep-wins-over-dev.js'); }); test('in repo addons declared win over dependencies', function () { expectAudit .module('./assets/my-app.js') - .resolves('../service/in-repo-over-deps.js') + .resolves('my-app/service/in-repo-over-deps.js') .to('./lib/in-repo-a/_app_/service/in-repo-over-deps.js'); }); test('ordering with before specified', function () { expectAudit .module('./assets/my-app.js') - .resolves('../service/test-before.js') + .resolves('my-app/service/test-before.js') .to('./node_modules/dev-d/_app_/service/test-before.js'); }); test('ordering with after specified', function () { expectAudit .module('./assets/my-app.js') - .resolves('../service/test-after.js') + .resolves('my-app/service/test-after.js') .to('./node_modules/dev-b/_app_/service/test-after.js'); }); }); From c4d7309082de457c85c4ef81da795c02917dd9f0 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 13 Mar 2023 11:51:58 -0400 Subject: [PATCH 24/26] remove unused args --- packages/core/src/app.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/core/src/app.ts b/packages/core/src/app.ts index 8e84a11ab..f6acd12df 100644 --- a/packages/core/src/app.ts +++ b/packages/core/src/app.ts @@ -1197,7 +1197,7 @@ export class AppBuilder { // this is a backward-compatibility feature: addons can force inclusion of // modules. - this.gatherImplicitModules('implicit-modules', relativePath, engine, amdModules); + this.gatherImplicitModules('implicit-modules', engine, amdModules); let params = { amdModules, fastbootOnlyAmdModules, lazyRoutes, lazyEngines, eagerModules, styles }; if (entryParams) { @@ -1267,7 +1267,7 @@ export class AppBuilder { let amdModules: { runtime: string; buildtime: string }[] = []; // this is a backward-compatibility feature: addons can force inclusion of // test support modules. - this.gatherImplicitModules('implicit-test-modules', myName, engine, amdModules); + this.gatherImplicitModules('implicit-test-modules', engine, amdModules); let { appFiles } = engine; for (let relativePath of appFiles.tests) { @@ -1291,7 +1291,6 @@ export class AppBuilder { private gatherImplicitModules( section: 'implicit-modules' | 'implicit-test-modules', - relativeTo: string, engine: Engine, lazyModules: { runtime: string; buildtime: string }[] ) { From 7a3aa65b179795355cbff22cf802d3ef310741db Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 13 Mar 2023 11:52:01 -0400 Subject: [PATCH 25/26] updating more tests --- tests/scenarios/compat-stage2-test.ts | 8 ++++---- tests/scenarios/compat-template-colocation-test.ts | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/scenarios/compat-stage2-test.ts b/tests/scenarios/compat-stage2-test.ts index 447e672bc..03cbd9f99 100644 --- a/tests/scenarios/compat-stage2-test.ts +++ b/tests/scenarios/compat-stage2-test.ts @@ -682,19 +682,19 @@ stage2Scenarios }); test('non-static other paths are included in the entrypoint', function () { - expectFile('assets/my-app.js').matches(/i\("..\/non-static-dir\/another-library\.js"\)/); + expectFile('assets/my-app.js').matches(/i\("my-app\/non-static-dir\/another-library\.js"\)/); }); test('static other paths are not included in the entrypoint', function () { - expectFile('assets/my-app.js').doesNotMatch(/i\("..\/static-dir\/my-library\.js"\)/); + expectFile('assets/my-app.js').doesNotMatch(/i\("my-app\/static-dir\/my-library\.js"\)/); }); test('top-level static other paths are not included in the entrypoint', function () { - expectFile('assets/my-app.js').doesNotMatch(/i\("..\/top-level-static\.js"\)/); + expectFile('assets/my-app.js').doesNotMatch(/i\("my-app\/top-level-static\.js"\)/); }); test('staticAppPaths do not match partial path segments', function () { - expectFile('assets/my-app.js').matches(/i\("..\/static-dir-not-really\/something\.js"\)/); + expectFile('assets/my-app.js').matches(/i\("my-app\/static-dir-not-really\/something\.js"\)/); }); test('invokes rule on appTemplates produces synthetic import', function () { diff --git a/tests/scenarios/compat-template-colocation-test.ts b/tests/scenarios/compat-template-colocation-test.ts index 5a42221c4..2223fbec9 100644 --- a/tests/scenarios/compat-template-colocation-test.ts +++ b/tests/scenarios/compat-template-colocation-test.ts @@ -127,10 +127,10 @@ scenarios test(`app's colocated components are implicitly included correctly`, function () { let assertFile = expectFile('assets/my-app.js'); assertFile.matches( - /d\(["']my-app\/components\/has-colocated-template["'], function\(\)\s*\{\s*return i\(["']\.\.\/components\/has-colocated-template\.js['"]\);\s*\}/ + /d\(["']my-app\/components\/has-colocated-template["'], function\(\)\s*\{\s*return i\(["']my-app\/components\/has-colocated-template\.js['"]\);\s*\}/ ); assertFile.matches( - /d\(["']my-app\/components\/template-only-component["'], function\(\)\s*\{\s*return i\(["']\.\.\/components\/template-only-component\.js['"]\);\s*\}/ + /d\(["']my-app\/components\/template-only-component["'], function\(\)\s*\{\s*return i\(["']my-app\/components\/template-only-component\.js['"]\);\s*\}/ ); }); @@ -202,10 +202,10 @@ scenarios test(`app's colocated components are not implicitly included`, function () { let assertFile = expectFile('assets/my-app.js'); assertFile.doesNotMatch( - /d\(["']my-app\/components\/has-colocated-template["'], function\(\)\s*\{\s*return i\(["']\.\.\/components\/has-colocated-template['"]\);\s*\}/ + /d\(["']my-app\/components\/has-colocated-template["'], function\(\)\s*\{\s*return i\(["']my-app\/components\/has-colocated-template['"]\);\s*\}/ ); assertFile.doesNotMatch( - /d\(["']my-app\/components\/template-only-component["'], function\(\)\s*\{\s*return i\(["']\.\.\/components\/template-only-component['"]\);\s*\}/ + /d\(["']my-app\/components\/template-only-component["'], function\(\)\s*\{\s*return i\(["']my-app\/components\/template-only-component['"]\);\s*\}/ ); }); From d885fd033e1c4255bed1f04088e7810de534fa70 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 13 Mar 2023 12:53:00 -0400 Subject: [PATCH 26/26] updating more tests --- tests/scenarios/compat-renaming-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scenarios/compat-renaming-test.ts b/tests/scenarios/compat-renaming-test.ts index 0d5d04653..145f551ab 100644 --- a/tests/scenarios/compat-renaming-test.ts +++ b/tests/scenarios/compat-renaming-test.ts @@ -243,7 +243,7 @@ appScenarios assertFile.matches( definesPattern( 'somebody-elses-package/environment', - '../node_modules/emits-multiple-packages/somebody-elses-package/environment' + 'emits-multiple-packages/somebody-elses-package/environment' ) ); });