From ba9d849baf94a00ff3b5dfe800e26d7a171047ee Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 13 Jan 2023 17:36:00 -0500 Subject: [PATCH 01/16] Move resolving into dedicated plugins --- packages/compat/src/audit.ts | 37 ++- packages/compat/tests/audit.test.ts | 1 + packages/core/src/app.ts | 4 +- packages/core/src/index.ts | 1 + packages/core/src/module-resolver.ts | 237 +++++++++--------- packages/webpack/package.json | 6 +- packages/webpack/src/ember-webpack.ts | 11 +- .../webpack/src/webpack-resolver-plugin.ts | 158 ++++++++++++ tests/scenarios/compat-stage2-test.ts | 4 - yarn.lock | 39 ++- 10 files changed, 368 insertions(+), 130 deletions(-) create mode 100644 packages/webpack/src/webpack-resolver-plugin.ts diff --git a/packages/compat/src/audit.ts b/packages/compat/src/audit.ts index c08b56a8a..846e4236f 100644 --- a/packages/compat/src/audit.ts +++ b/packages/compat/src/audit.ts @@ -1,7 +1,7 @@ import { readFileSync, readJSONSync } from 'fs-extra'; import { dirname, join, resolve as resolvePath } from 'path'; import resolveModule from 'resolve'; -import { AppMeta, explicitRelative, hbsToJS } from '@embroider/core'; +import { AppMeta, explicitRelative, hbsToJS, ResolverPluginOptions, Resolver } from '@embroider/core'; import { Memoize } from 'typescript-memoize'; import chalk from 'chalk'; import jsdom from 'jsdom'; @@ -18,6 +18,7 @@ import { import { AuditBuildOptions, AuditOptions } from './audit/options'; import { buildApp, BuildError, isBuildError } from './audit/build'; import { AuditMessage } from './resolver'; +import assertNever from 'assert-never'; const { JSDOM } = jsdom; @@ -249,6 +250,13 @@ export class Audit { return config; } + @Memoize() + private get resolverParams(): ResolverPluginOptions { + // eslint-disable-next-line @typescript-eslint/no-require-imports + let config = require(join(this.appDir, '_adjust_imports.json')); + return config; + } + private debug(message: string, ...args: any[]) { if (this.options.debug) { console.log(message, ...args); @@ -294,8 +302,9 @@ export class Audit { } else { module.parsed = visitResult; let resolved = new Map() as NonNullable; + let resolver = new Resolver(filename, this.resolverParams); for (let dep of visitResult.dependencies) { - let depFilename = await this.resolve(dep, filename); + let depFilename = await this.resolve(dep, filename, resolver); if (depFilename) { resolved.set(dep, depFilename); if (!isResolutionFailure(depFilename)) { @@ -520,8 +529,30 @@ export class Audit { return this.visitJS(filename, js); } - private async resolve(specifier: string, fromPath: string): Promise { + private async resolve( + specifier: string, + fromPath: string, + resolver: Resolver + ): Promise { + let resolution = resolver.resolve(specifier); + switch (resolution.result) { + case 'external': + // nothing to audit + return; + case 'redirect-to': + // follow the redirect + specifier = resolution.specifier; + break; + case 'continue': + // nothing to do + break; + default: + throw assertNever(resolution); + } + if (['@embroider/macros', '@ember/template-factory'].includes(specifier)) { + // the audit process deliberately removes the @embroider/macros babel + // plugins, so the imports are still present and should be left alone. return; } try { diff --git a/packages/compat/tests/audit.test.ts b/packages/compat/tests/audit.test.ts index 17d1290f5..4e2496d6a 100644 --- a/packages/compat/tests/audit.test.ts +++ b/packages/compat/tests/audit.test.ts @@ -74,6 +74,7 @@ describe('audit', function () { null, 2 )}`, + '_adjust_imports.json': JSON.stringify(resolver.adjustImportsOptions), }); let appMeta: AppMeta = { type: 'app', diff --git a/packages/core/src/app.ts b/packages/core/src/app.ts index 648e4913f..076bed314 100644 --- a/packages/core/src/app.ts +++ b/packages/core/src/app.ts @@ -422,7 +422,9 @@ export class AppBuilder { colocationOptions, ]); - babel.plugins.push(this.adjustImportsPlugin(appFiles)); + // TODO: Keeping this just for the side effect of emitting the config json + this.adjustImportsPlugin(appFiles); + // babel.plugins.push(); // we can use globally shared babel runtime by default babel.plugins.push([ diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 6c5c48f0e..51946e245 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -17,6 +17,7 @@ export { compile as jsHandlebarsCompile } from './js-handlebars'; export { AppAdapter, AppBuilder, EmberENV } from './app'; export { todo, unsupported, warn, debug, expectWarning, throwOnWarnings } from './messages'; export { mangledEngineRoot } from './engine-mangler'; +export { Resolver, Options as ResolverOptions, Resolution } from './module-resolver'; // this is reexported because we already make users manage a peerDep from some // other packages (like embroider/webpack and @embroider/compat diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index 6f034afce..a4fb6bf8a 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -1,7 +1,7 @@ import { emberVirtualPackages, emberVirtualPeerDeps, packageName as getPackageName } from '@embroider/shared-internals'; import { dirname, resolve } from 'path'; import { PackageCache, Package, V2Package, explicitRelative } from '@embroider/shared-internals'; -import { Memoize } from 'typescript-memoize'; +import { compile } from './js-handlebars'; export interface Options { renamePackages: { @@ -26,17 +26,14 @@ export interface Options { export type Resolution = | { result: 'continue' } - | { result: 'redirect-to'; specifier: string } - | { result: 'external'; specifier: string } - | { result: 'runtime-failure'; specifier: string }; + | { result: 'alias'; specifier: string; fromFile?: string } + | { result: 'rehome'; fromFile: string } + | { result: 'virtual'; filename: string; content: string }; export class Resolver { - readonly originalFilename: string; + constructor(private options: Options) {} - constructor(readonly filename: string, private options: Options) { - this.originalFilename = options.relocatedFiles[filename] || filename; - } - resolve(specifier: string, isDynamic: boolean): Resolution { + beforeResolve(specifier: string, fromFile: string): Resolution { if (specifier === '@embroider/macros') { // the macros package is always handled directly within babel (not // necessarily as a real resolvable package), so we should not mess with it. @@ -45,23 +42,26 @@ export class Resolver { return { result: 'continue' }; } - let maybeRenamed = this.handleRenaming(specifier); - let resolution = this.handleExternal(maybeRenamed, isDynamic); + let maybeRenamed = this.handleRenaming(specifier, fromFile); + let resolution = this.preHandleExternal(maybeRenamed, fromFile); if (resolution.result === 'continue' && maybeRenamed !== specifier) { - return { result: 'redirect-to', specifier: maybeRenamed }; + return { result: 'alias', specifier: maybeRenamed }; } - return resolution; + return { result: 'continue' }; + } + + fallbackResolve(specifier: string, fromFile: string): Resolution { + return this.postHandleExternal(specifier, fromFile); } - @Memoize() - owningPackage(): Package | undefined { - return PackageCache.shared('embroider-stage3', this.options.appRoot).ownerOfFile(this.originalFilename); + private owningPackage(fromFile: string): Package | undefined { + return PackageCache.shared('embroider-stage3', this.options.appRoot).ownerOfFile(fromFile); } - @Memoize() - private relocatedIntoPackage(): V2Package | undefined { - if (this.originalFilename !== this.filename) { - let owning = PackageCache.shared('embroider-stage3', this.options.appRoot).ownerOfFile(this.filename); + private relocatedIntoPackage(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 to get relocated into a v2 ember package here`); } @@ -69,7 +69,7 @@ export class Resolver { } } - private handleRenaming(specifier: string) { + private handleRenaming(specifier: string, fromFile: string) { let packageName = getPackageName(specifier); if (!packageName) { return specifier; @@ -93,34 +93,33 @@ export class Resolver { return specifier.replace(packageName, this.options.renamePackages[packageName]); } - let pkg = this.owningPackage(); + let pkg = this.owningPackage(fromFile); if (!pkg || !pkg.isV2Ember()) { return specifier; } if (pkg.meta['auto-upgraded'] && pkg.name === packageName) { - // we found a self-import, make it relative. Only auto-upgraded packages get - // this help, v2 packages are natively supposed to use relative imports for - // their own modules, and we want to push them all to do that correctly. - let fullPath = specifier.replace(packageName, pkg.root); - return explicitRelative(dirname(this.filename), fullPath); + // we found a self-import, resolve it for them. Only auto-upgraded + // packages get this help, v2 packages are natively supposed to use + // relative imports for their own modules, and we want to push them all to + // do that correctly. + return specifier.replace(packageName, pkg.root); } - let relocatedIntoPkg = this.relocatedIntoPackage(); + let relocatedIntoPkg = this.relocatedIntoPackage(fromFile); if (relocatedIntoPkg && pkg.meta['auto-upgraded'] && relocatedIntoPkg.name === packageName) { // a file that was relocated into a package does a self-import of that // package's name. This can happen when an addon (like ember-cli-mirage) // emits files from its own treeForApp that contain imports of the app's own // fully qualified name. - let fullPath = specifier.replace(packageName, relocatedIntoPkg.root); - return explicitRelative(dirname(this.filename), fullPath); + return specifier.replace(packageName, relocatedIntoPkg.root); } return specifier; } - private handleExternal(specifier: string, isDynamic: boolean): Resolution { - let pkg = this.owningPackage(); + private preHandleExternal(specifier: string, fromFile: string): Resolution { + let pkg = this.owningPackage(fromFile); if (!pkg || !pkg.isV2Ember()) { return { result: 'continue' }; } @@ -132,11 +131,11 @@ export class Resolver { // we do allow them to be explicitly externalized by the package author (or // a compat adapter). In the metadata, they would be listed in // package-relative form, so we need to convert this specifier to that. - let absoluteSpecifier = resolve(dirname(this.filename), specifier); + let absoluteSpecifier = resolve(dirname(fromFile), specifier); let packageRelativeSpecifier = explicitRelative(pkg.root, absoluteSpecifier); if (isExplicitlyExternal(packageRelativeSpecifier, pkg)) { let publicSpecifier = absoluteSpecifier.replace(pkg.root, pkg.name); - return { result: 'external', specifier: publicSpecifier }; + return external(publicSpecifier); } else { return { result: 'continue' }; } @@ -145,7 +144,7 @@ export class Resolver { // absolute package imports can also be explicitly external based on their // full specifier name if (isExplicitlyExternal(specifier, pkg)) { - return { result: 'external', specifier }; + return external(specifier); } if (!pkg.meta['auto-upgraded'] && emberVirtualPeerDeps.has(packageName)) { @@ -159,65 +158,77 @@ export class Resolver { // original place they might accidentally resolve the emberVirtualPeerDeps // that are present there in v1 format. // - // So before we even check isResolvable, we adjust these imports to point at - // the app's copies instead. - if (emberVirtualPeerDeps.has(packageName)) { - if (!this.options.activeAddons[packageName]) { - throw new Error( - `${pkg.name} is trying to import the app's ${packageName} package, but it seems to be missing` - ); - } - return { - result: 'redirect-to', - specifier: explicitRelative( - dirname(this.filename), - specifier.replace(packageName, this.options.activeAddons[packageName]) - ), - }; + // So before we let normal resolving happen, we adjust these imports to + // point at the app's copies instead. + if (!this.options.activeAddons[packageName]) { + throw new Error(`${pkg.name} is trying to import the app's ${packageName} package, but it seems to be missing`); } + return { + result: 'rehome', + fromFile: this.options.appRoot + './package.json', + }; } - let relocatedPkg = this.relocatedIntoPackage(); - if (relocatedPkg) { - // this file has been moved into another package (presumably the app). + if (pkg.meta['auto-upgraded'] && !pkg.hasDependency('ember-auto-import')) { + try { + let dep = PackageCache.shared('embroider-stage3', this.options.appRoot).resolve(packageName, pkg); + if (!dep.isEmberPackage()) { + // classic ember addons can only import non-ember dependencies if they + // have ember-auto-import. + return external(specifier); + } + } catch (err) { + if (err.code !== 'MODULE_NOT_FOUND') { + throw err; + } + } + } - // first try to resolve from the destination package - if (isResolvable(packageName, relocatedPkg, this.options.appRoot)) { - // self-imports are legal in the app tree, even for v2 packages. - if (!pkg.meta['auto-upgraded'] && packageName !== pkg.name) { + // assertions on what native v2 addons can import + if (!pkg.meta['auto-upgraded']) { + let relocatedPkg = this.relocatedIntoPackage(fromFile); + if (relocatedPkg) { + // 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` ); } - return { result: 'continue' }; } else { - // second try to resolve from the source package - let targetPkg = isResolvable(packageName, pkg, this.options.appRoot); - if (targetPkg) { - // self-imports are legal in the app tree, even for v2 packages. - if (!pkg.meta['auto-upgraded'] && packageName !== pkg.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` - ); - } - // we found it, but we need to rewrite it because it's not really going to - // resolve from where its sitting - return { - result: 'redirect-to', - specifier: explicitRelative(dirname(this.filename), specifier.replace(packageName, targetPkg.root)), - }; - } - } - } else { - if (isResolvable(packageName, pkg, this.options.appRoot)) { + // 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` ); } - return { result: 'continue' }; } } + return { result: 'continue' }; + } + + private postHandleExternal(specifier: string, fromFile: string): Resolution { + let pkg = this.owningPackage(fromFile); + if (!pkg || !pkg.isV2Ember()) { + return { result: 'continue' }; + } + + let packageName = getPackageName(specifier); + if (!packageName) { + // this is a relative import, we have nothing more to for it. + return { result: 'continue' }; + } + + let relocatedPkg = this.relocatedIntoPackage(fromFile); + if (relocatedPkg) { + // we didn't find it from the original package, so try from the relocated + // package + return { + result: 'rehome', + fromFile: relocatedPkg.root + '/package.json', + }; + } // auto-upgraded packages can fall back to the set of known active addons // @@ -225,11 +236,8 @@ export class Resolver { // themselves (which is needed due to app tree merging) if ((pkg.meta['auto-upgraded'] || packageName === pkg.name) && this.options.activeAddons[packageName]) { return { - result: 'redirect-to', - specifier: explicitRelative( - dirname(this.filename), - specifier.replace(packageName, this.options.activeAddons[packageName]) - ), + result: 'alias', + specifier: specifier.replace(packageName, this.options.activeAddons[packageName]), }; } @@ -238,25 +246,16 @@ export class Resolver { // runtime. Native v2 packages can only get this behavior in the // isExplicitlyExternal case above because they need to explicitly ask for // externals. - return { result: 'external', specifier }; + return external(specifier); } else { // native v2 packages don't automatically externalize *everything* the way // auto-upgraded packages do, but they still externalize known and approved // ember virtual packages (like @ember/component) if (emberVirtualPackages.has(packageName)) { - return { result: 'external', specifier }; + return external(specifier); } } - // non-resolvable imports in dynamic positions become runtime errors, not - // build-time errors, so we emit the runtime error module here before the - // stage3 packager has a chance to see the missing module. (Maybe some stage3 - // packagers will have this behavior by default, because it would make sense, - // but webpack at least does not.) - if (isDynamic) { - return { result: 'runtime-failure', specifier }; - } - // this is falling through with the original specifier which was // non-resolvable, which will presumably cause a static build error in stage3. return { result: 'continue' }; @@ -267,25 +266,6 @@ function isExplicitlyExternal(specifier: string, fromPkg: V2Package): boolean { return Boolean(fromPkg.isV2Addon() && fromPkg.meta['externals'] && fromPkg.meta['externals'].includes(specifier)); } -function isResolvable(packageName: string, fromPkg: V2Package, appRoot: string): false | Package { - try { - let dep = PackageCache.shared('embroider-stage3', appRoot).resolve(packageName, fromPkg); - if (!dep.isEmberPackage() && fromPkg.meta['auto-upgraded'] && !fromPkg.hasDependency('ember-auto-import')) { - // classic ember addons can only import non-ember dependencies if they - // have ember-auto-import. - // - // whereas native v2 packages can always import any dependency - return false; - } - return dep; - } catch (err) { - if (err.code !== 'MODULE_NOT_FOUND') { - throw err; - } - return false; - } -} - // we don't want to allow things that resolve only by accident that are likely // to break in other setups. For example: import your dependencies' // dependencies, or importing your own name from within a monorepo (which will @@ -306,3 +286,34 @@ function reliablyResolvable(pkg: V2Package, packageName: string) { return false; } + +function external(specifier: string): Resolution { + return { + result: 'virtual', + filename: `@embroider/external/${specifier}`, + content: externalShim({ moduleName: specifier }), + }; +} + +const externalShim = compile(` +{{#if (eq moduleName "require")}} +const m = window.requirejs; +{{else}} +const m = window.require("{{{js-string-escape moduleName}}}"); +{{/if}} +{{!- + There are plenty of hand-written AMD defines floating around + that lack this, and they will break when other build systems + encounter them. + + As far as I can tell, Ember's loader was already treating this + case as a module, so in theory we aren't breaking anything by + marking it as such when other packagers come looking. + + todo: get review on this part. +-}} +if (m.default && !m.__esModule) { + m.__esModule = true; +} +module.exports = m; +`) as (params: { moduleName: string }) => string; diff --git a/packages/webpack/package.json b/packages/webpack/package.json index 1533c5bc0..d109ea713 100644 --- a/packages/webpack/package.json +++ b/packages/webpack/package.json @@ -24,9 +24,9 @@ "@embroider/shared-internals": "2.0.0", "@types/source-map": "^0.5.7", "@types/supports-color": "^8.1.0", + "assert-never": "^1.2.1", "babel-loader": "^8.2.2", "babel-preset-env": "^1.7.0", - "supports-color": "^8.1.0", "css-loader": "^5.2.6", "csso": "^4.2.0", "debug": "^4.3.2", @@ -37,8 +37,10 @@ "semver": "^7.3.5", "source-map-url": "^0.4.1", "style-loader": "^2.0.0", + "supports-color": "^8.1.0", "terser": "^5.7.0", - "thread-loader": "^3.0.4" + "thread-loader": "^3.0.4", + "webpack-virtual-modules": "^0.5.0" }, "devDependencies": { "@types/csso": "^3.5.1", diff --git a/packages/webpack/src/ember-webpack.ts b/packages/webpack/src/ember-webpack.ts index bbf764195..a84fe2390 100644 --- a/packages/webpack/src/ember-webpack.ts +++ b/packages/webpack/src/ember-webpack.ts @@ -22,7 +22,7 @@ import { } from '@embroider/core'; import { tmpdir } from '@embroider/shared-internals'; import webpack, { Configuration, RuleSetUseItem, WebpackPluginInstance } from 'webpack'; -import { readFileSync, outputFileSync, copySync, realpathSync, Stats, statSync, readJsonSync } from 'fs-extra'; +import { readFileSync, outputFileSync, copySync, realpathSync, Stats, statSync, readJSONSync } from 'fs-extra'; import { join, dirname, relative, sep } from 'path'; import isEqual from 'lodash/isEqual'; import mergeWith from 'lodash/mergeWith'; @@ -36,6 +36,7 @@ import crypto from 'crypto'; import semverSatisfies from 'semver/functions/satisfies'; import supportsColor from 'supports-color'; import { Options as HbsLoaderOptions } from '@embroider/hbs-loader'; +import { EmbroiderPlugin, Options as EmbroiderPluginOptions } from './webpack-resolver-plugin'; const debug = makeDebug('embroider:debug'); @@ -194,6 +195,11 @@ const Webpack: PackagerConstructor = class Webpack implements Packager let { plugins: stylePlugins, loaders: styleLoaders } = this.setupStyleConfig(variant); + let resolverConfig: EmbroiderPluginOptions = { + ...readJSONSync(join(this.pathToVanillaApp, '_adjust_imports.json')), + ...readJSONSync(join(this.pathToVanillaApp, '_relocated_files.json')), + }; + return { mode: variant.optimizeForProduction ? 'production' : 'development', context: this.pathToVanillaApp, @@ -203,6 +209,7 @@ const Webpack: PackagerConstructor = class Webpack implements Packager }, plugins: [ ...stylePlugins, + new EmbroiderPlugin(resolverConfig), compiler => { compiler.hooks.done.tapPromise('EmbroiderPlugin', async stats => { this.summarizeStats(stats, variant, variantIndex); @@ -319,7 +326,7 @@ const Webpack: PackagerConstructor = class Webpack implements Packager appRelativeSourceMapURL = join(dirname(script), fileRelativeSourceMapURL); let content; try { - content = readJsonSync(join(this.pathToVanillaApp, appRelativeSourceMapURL)); + content = readJSONSync(join(this.pathToVanillaApp, appRelativeSourceMapURL)); } catch (err) { // the script refers to a sourcemap that doesn't exist, so we just leave // the map out. diff --git a/packages/webpack/src/webpack-resolver-plugin.ts b/packages/webpack/src/webpack-resolver-plugin.ts new file mode 100644 index 000000000..6ad5dac3a --- /dev/null +++ b/packages/webpack/src/webpack-resolver-plugin.ts @@ -0,0 +1,158 @@ +import { dirname } from 'path'; +import VirtualModulesPlugin from 'webpack-virtual-modules'; +import { + Resolver as EmbroiderResolver, + ResolverOptions as EmbroiderResolverOptions, + Resolution, +} from '@embroider/core'; +import type { Compiler, Resolver as WebpackResolver } from 'webpack'; +import assertNever from 'assert-never'; + +export { EmbroiderResolverOptions as Options }; + +export class EmbroiderPlugin { + constructor(private opts: EmbroiderResolverOptions) {} + apply(compiler: Compiler) { + if (!compiler.options.resolve.plugins) { + compiler.options.resolve.plugins = []; + } + + let vfs = compiler.options.plugins.find((i: unknown) => i instanceof VirtualModulesPlugin) as + | VirtualModulesPlugin + | undefined; + + if (!vfs) { + vfs = new VirtualModulesPlugin(); + compiler.options.plugins.push(vfs); + } + + let resolverPlugin = new ResolverPlugin(vfs, this.opts); + compiler.options.resolve.plugins.push(resolverPlugin); + } +} + +class ResolverPlugin { + private resolver: EmbroiderResolver; + + constructor(private vfs: VirtualModulesPlugin, opts: EmbroiderResolverOptions) { + this.resolver = new EmbroiderResolver(opts); + } + + #resolve( + resolution: Resolution, + resolver: WebpackResolver, + request: Request, + context: unknown, + callback: (err?: Error | null, result?: any) => void + ) { + if (resolution.result === 'virtual') { + this.vfs.writeModule(`node_modules/${resolution.filename}`, resolution.content); + resolution = { + result: 'alias', + specifier: resolution.filename, + }; + } + + switch (resolution.result) { + case 'alias': { + let newRequest = { + request: resolution.specifier, + path: resolution.fromFile ? dirname(resolution.fromFile) : request.path, + fullySpecified: false, + context: { + issuer: resolution.fromFile ?? request.context.issuer, + }, + }; + resolver.doResolve( + resolver.ensureHook('internal-resolve'), + newRequest, + '@embroider/webpack', + context, + wrapCallback(callback) + ); + return; + } + case 'rehome': { + let newRequest = { + request: request.request, + path: dirname(resolution.fromFile), + fullySpecified: false, + context: { + issuer: resolution.fromFile, + }, + }; + resolver.doResolve( + resolver.ensureHook('internal-resolve'), + newRequest, + '@embroider/webpack', + context, + wrapCallback(callback) + ); + return; + } + case 'continue': + return; + default: + throw assertNever(resolution); + } + } + + apply(resolver: WebpackResolver) { + // raw-resolve -> internal-resolve is the same place in the pipeline that + // webpack's built-in `resolve.alias` takes effect. It's supposed to take + // precedence over other resolving decisions. + resolver.getHook('raw-resolve').tapAsync('my-resolver-plugin', async (request, context, callback) => { + if (!isRelevantRequest(request)) { + return callback(); + } + let result = this.resolver.beforeResolve(request.request, request.context.issuer); + this.#resolve(result, resolver, request, context, callback); + }); + + // described-resolve -> internal-resolve is the same place in the pipeline + // that webpack's built-in `resolve.fallback` takes effect. It's supposed to + // only run when the rest of resolving fails to find something. + resolver.getHook('described-resolve').tapAsync( + // we need to set the stage here because otherwise we end up before the + // built-in NextPlugin. Instead we want to behave like the built-in + // AliasPlugin that implements resolve.fallback -- it comes after + // NextPlugin. + // + // The number just needs to be greater than zero to come after the + // defaults (tapable assigned them stage 0 by default). + { name: 'my-resolver-plugin', stage: 10 }, + async (request, context, callback) => { + if (!isRelevantRequest(request)) { + return callback(); + } + let result = this.resolver.fallbackResolve(request.request, request.context.issuer); + this.#resolve(result, resolver, request, context, callback); + } + ); + } +} + +interface Request { + request: string; + path: string; + context: { + issuer: string; + }; +} + +function isRelevantRequest(request: any): request is Request { + return ( + typeof request.request === 'string' && + typeof request.context?.issuer === 'string' && + request.context.issuer !== '' && + typeof request.path === 'string' + ); +} + +function wrapCallback(callback: (err?: Error | null, result?: T) => void) { + return (err: Error | null, result: T) => { + if (err) return callback(err); + if (result) return callback(null, result); + return callback(); + }; +} diff --git a/tests/scenarios/compat-stage2-test.ts b/tests/scenarios/compat-stage2-test.ts index 1ce0c1a9a..103eb2866 100644 --- a/tests/scenarios/compat-stage2-test.ts +++ b/tests/scenarios/compat-stage2-test.ts @@ -485,10 +485,6 @@ stage2Scenarios assertFile.matches( /window\.define\(["']my-app\/templates\/components\/second-choice["'],\s*function\s\(\)\s*\{\s*return\s+esc\(require\(["']\.\.\/\.\.\/\.\.\/templates\/components\/second-choice\.hbs["']/ ); - assertFile.matches( - /import somethingExternal from ["'].*\/externals\/not-a-resolvable-package["']/, - 'externals are handled correctly' - ); }); test('app/hello-world.js', function () { diff --git a/yarn.lock b/yarn.lock index c85a5b54d..49d2cdcd4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5404,7 +5404,26 @@ browserslist-generator@^1.0.66: semver "^7.3.7" ua-parser-js "^1.0.2" -browserslist@4.20.2, browserslist@^3.2.6, browserslist@^4.0.0, browserslist@^4.14.0, browserslist@^4.14.5, browserslist@^4.20.4, browserslist@^4.21.3, browserslist@^4.21.4: +browserslist@4.20.2: + version "4.20.2" + resolved "https://registry.yarnpkg.com/browserslist/-/browserslist-4.20.2.tgz#567b41508757ecd904dab4d1c646c612cd3d4f88" + integrity sha512-CQOBCqp/9pDvDbx3xfMi+86pr4KXIf2FDkTTdeuYw8OxS9t898LA1Khq57gtufFILXpfgsSx5woNgsBgvGjpsA== + dependencies: + caniuse-lite "^1.0.30001317" + electron-to-chromium "^1.4.84" + escalade "^3.1.1" + node-releases "^2.0.2" + picocolors "^1.0.0" + +browserslist@^3.2.6: + version "3.2.8" + resolved "https://registry.yarnpkg.com/browserslist/-/browserslist-3.2.8.tgz#b0005361d6471f0f5952797a76fc985f1f978fc6" + integrity sha512-WHVocJYavUwVgVViC0ORikPHQquXwVh939TaelZ4WDqpWgTX/FsGhl/+P4qBUAGcRvtOgDgC+xftNWWp2RUTAQ== + dependencies: + caniuse-lite "^1.0.30000844" + electron-to-chromium "^1.3.47" + +browserslist@^4.0.0, browserslist@^4.14.5, browserslist@^4.20.4, browserslist@^4.21.3, browserslist@^4.21.4: version "4.21.4" resolved "https://registry.yarnpkg.com/browserslist/-/browserslist-4.21.4.tgz#e7496bbc67b9e39dd0f98565feccdcb0d4ff6987" integrity sha512-CBHJJdDmgjl3daYjN5Cp5kbTf1mUhZoS+beLklHIvkOWscs83YAhLlF3Wsh/lciQYAcbBJgTOD44VtG31ZM4Hw== @@ -5563,6 +5582,11 @@ caniuse-lite@^1.0.0, caniuse-lite@^1.0.30001328, caniuse-lite@^1.0.30001400: resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001439.tgz#ab7371faeb4adff4b74dad1718a6fd122e45d9cb" integrity sha512-1MgUzEkoMO6gKfXflStpYgZDlFM7M/ck/bgfVCACO5vnAf0fXoNVHdWtqGU+MYca+4bL9Z5bpOVmR33cWW9G2A== +caniuse-lite@^1.0.30000844, caniuse-lite@^1.0.30001317: + version "1.0.30001444" + resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001444.tgz#c0a530776eb44d933b493de1d05346f2527b30fc" + integrity sha512-ecER9xgJQVMqcrxThKptsW0pPxSae8R2RB87LNa+ivW9ppNWRHEplXcDzkCOP4LYWGj8hunXLqaiC41iBATNyg== + capture-exit@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/capture-exit/-/capture-exit-2.0.0.tgz#fb953bfaebeb781f62898239dabb426d08a509a4" @@ -6659,7 +6683,7 @@ ee-first@1.1.1: resolved "https://registry.yarnpkg.com/ee-first/-/ee-first-1.1.1.tgz#590c61156b0ae2f4f0255732a158b266bc56b21d" integrity sha512-WMwm9LhRUo+WUaRN+vRuETqG89IgZphVSNkdFgeb6sS/E4OrDIN7t48CAewSHXc6C8lefD8KKfr5vY61brQlow== -electron-to-chromium@^1.4.251: +electron-to-chromium@^1.3.47, electron-to-chromium@^1.4.251, electron-to-chromium@^1.4.84: version "1.4.284" resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.4.284.tgz#61046d1e4cab3a25238f6bf7413795270f125592" integrity sha512-M8WEXFuKXMYMVr45fo8mq0wUrrJHheiKZf6BArTKk9ZBYCKJEOU5H8cdWgDT+qCVZf7Na4lVUaZsA+h6uA9+PA== @@ -10053,7 +10077,7 @@ got@^9.6.0: to-readable-stream "^1.0.0" url-parse-lax "^3.0.0" -graceful-fs@^4.0.0, graceful-fs@^4.1.2, graceful-fs@^4.1.3, graceful-fs@^4.1.5, graceful-fs@^4.1.6, graceful-fs@^4.1.9, graceful-fs@^4.2.0, graceful-fs@^4.2.2, graceful-fs@^4.2.4, graceful-fs@^4.2.9: +graceful-fs@^4.1.2, graceful-fs@^4.1.3, graceful-fs@^4.1.5, graceful-fs@^4.1.6, graceful-fs@^4.1.9, graceful-fs@^4.2.0, graceful-fs@^4.2.2, graceful-fs@^4.2.4, graceful-fs@^4.2.9: version "4.2.10" resolved "https://registry.yarnpkg.com/graceful-fs/-/graceful-fs-4.2.10.tgz#147d3a006da4ca3ce14728c7aefc287c367d7a6c" integrity sha512-9ByhssR2fPVsNZj478qUUbKfmL0+t5BDVyjShtyZZLiK7ZDAArFFfopyOTj0M05wE2tJPisA4iTnnXl2YoPvOA== @@ -12725,7 +12749,7 @@ node-notifier@^10.0.0: uuid "^8.3.2" which "^2.0.2" -node-releases@^2.0.6: +node-releases@^2.0.2, node-releases@^2.0.6: version "2.0.8" resolved "https://registry.yarnpkg.com/node-releases/-/node-releases-2.0.8.tgz#0f349cdc8fcfa39a92ac0be9bc48b7706292b9ae" integrity sha512-dFSmB8fFHEH/s81Xi+Y/15DQY6VHW81nXRj86EMSL3lmuTmK1e+aT4wrFCkTbm+gSwkw4KpX+rT/pMM2c1mF+A== @@ -13702,7 +13726,7 @@ qunit-dom@^2.0.0: ember-cli-babel "^7.23.0" ember-cli-version-checker "^5.1.1" -qunit@^2.14.1, qunit@^2.16.0, qunit@^2.19.1: +qunit@^2.16.0, qunit@^2.19.1: version "2.19.3" resolved "https://registry.yarnpkg.com/qunit/-/qunit-2.19.3.tgz#bcf81a2e8d176dc19fe8dd358c4cbd08619af03a" integrity sha512-vEnspSZ37u2oR01OA/IZ1Td5V7BvQYFECdKPv86JaBplDNa5IHg0v7jFSPoP5L5o78Dbi8sl7/ATtpRDAKlSdw== @@ -16130,6 +16154,11 @@ webpack-sources@^3.2.3: resolved "https://registry.yarnpkg.com/webpack-sources/-/webpack-sources-3.2.3.tgz#2d4daab8451fd4b240cc27055ff6a0c2ccea0cde" integrity sha512-/DyMEOrDgLKKIG0fmvtz+4dUX/3Ghozwgm6iPp8KRhvn+eQf9+Q7GWxVNMk3+uCPWfdXYC4ExGBckIXdFEfH1w== +webpack-virtual-modules@^0.5.0: + version "0.5.0" + resolved "https://registry.yarnpkg.com/webpack-virtual-modules/-/webpack-virtual-modules-0.5.0.tgz#362f14738a56dae107937ab98ea7062e8bdd3b6c" + integrity sha512-kyDivFZ7ZM0BVOUteVbDFhlRt7Ah/CSPwJdi8hBpkK7QLumUqdLtVfm/PX/hkcnrvr0i77fO5+TjZ94Pe+C9iw== + webpack@^5, webpack@^5.38.1, webpack@^5.72.1, webpack@^5.74.0: version "5.75.0" resolved "https://registry.yarnpkg.com/webpack/-/webpack-5.75.0.tgz#1e440468647b2505860e94c9ff3e44d5b582c152" From 0bca7fb71dd957386faa5fa0e69603cd7eae981c Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 19 Jan 2023 14:08:22 -0500 Subject: [PATCH 02/16] move resolution into webpack plugin --- packages/compat/src/audit.ts | 56 +++++- packages/compat/src/compat-app.ts | 11 +- packages/compat/src/resolver.ts | 13 +- packages/compat/tests/resolver.test.ts | 5 +- packages/core/src/app.ts | 14 +- .../core/src/babel-plugin-adjust-imports.ts | 171 +----------------- packages/core/src/module-resolver.ts | 2 +- .../tests/babel-plugin-adjust-imports.test.ts | 129 ------------- packages/webpack/src/ember-webpack.ts | 4 +- .../webpack/src/webpack-resolver-plugin.ts | 15 +- yarn.lock | 34 +--- 11 files changed, 91 insertions(+), 363 deletions(-) delete mode 100644 packages/core/tests/babel-plugin-adjust-imports.test.ts diff --git a/packages/compat/src/audit.ts b/packages/compat/src/audit.ts index 846e4236f..69b9e99ba 100644 --- a/packages/compat/src/audit.ts +++ b/packages/compat/src/audit.ts @@ -1,7 +1,7 @@ import { readFileSync, readJSONSync } from 'fs-extra'; import { dirname, join, resolve as resolvePath } from 'path'; import resolveModule from 'resolve'; -import { AppMeta, explicitRelative, hbsToJS, ResolverPluginOptions, Resolver } from '@embroider/core'; +import { AppMeta, explicitRelative, hbsToJS, Resolver, ResolverOptions } from '@embroider/core'; import { Memoize } from 'typescript-memoize'; import chalk from 'chalk'; import jsdom from 'jsdom'; @@ -251,7 +251,7 @@ export class Audit { } @Memoize() - private get resolverParams(): ResolverPluginOptions { + private get resolverParams(): ResolverOptions { // eslint-disable-next-line @typescript-eslint/no-require-imports let config = require(join(this.appDir, '_adjust_imports.json')); return config; @@ -302,7 +302,7 @@ export class Audit { } else { module.parsed = visitResult; let resolved = new Map() as NonNullable; - let resolver = new Resolver(filename, this.resolverParams); + let resolver = new Resolver(this.resolverParams); for (let dep of visitResult.dependencies) { let depFilename = await this.resolve(dep, filename, resolver); if (depFilename) { @@ -531,17 +531,23 @@ export class Audit { private async resolve( specifier: string, - fromPath: string, + fromFile: string, resolver: Resolver ): Promise { - let resolution = resolver.resolve(specifier); + let resolution = resolver.beforeResolve(specifier, fromFile); switch (resolution.result) { - case 'external': + case 'virtual': // nothing to audit return; - case 'redirect-to': + case 'alias': // follow the redirect specifier = resolution.specifier; + if (resolution.fromFile) { + fromFile = resolution.fromFile; + } + break; + case 'rehome': + fromFile = resolution.fromFile; break; case 'continue': // nothing to do @@ -557,12 +563,44 @@ export class Audit { } try { return resolveModule.sync(specifier, { - basedir: dirname(fromPath), + basedir: dirname(fromFile), extensions: this.meta['resolvable-extensions'], }); } catch (err) { if (err.code === 'MODULE_NOT_FOUND') { - return { isResolutionFailure: true }; + resolution = resolver.fallbackResolve(specifier, fromFile); + switch (resolution.result) { + case 'virtual': + // nothing to audit + return; + case 'alias': + // follow the redirect + specifier = resolution.specifier; + if (resolution.fromFile) { + fromFile = resolution.fromFile; + } + break; + case 'rehome': + fromFile = resolution.fromFile; + break; + case 'continue': + // nothing to do + break; + default: + throw assertNever(resolution); + } + try { + return resolveModule.sync(specifier, { + basedir: dirname(fromFile), + extensions: this.meta['resolvable-extensions'], + }); + } catch (err) { + if (err.code === 'MODULE_NOT_FOUND') { + return { isResolutionFailure: true }; + } else { + throw err; + } + } } else { throw err; } diff --git a/packages/compat/src/compat-app.ts b/packages/compat/src/compat-app.ts index 43740c55b..15ad6f3e7 100644 --- a/packages/compat/src/compat-app.ts +++ b/packages/compat/src/compat-app.ts @@ -30,8 +30,7 @@ import { sync as resolveSync } from 'resolve'; import { MacrosConfig } from '@embroider/macros/src/node'; import bind from 'bind-decorator'; import { pathExistsSync } from 'fs-extra'; -import { tmpdir } from '@embroider/core'; -import { Options as AdjustImportsOptions } from '@embroider/core/src/babel-plugin-adjust-imports'; +import { tmpdir, ResolverOptions } from '@embroider/core'; import type { Transform } from 'babel-plugin-ember-template-compilation'; interface TreeNames { @@ -337,21 +336,21 @@ class CompatAppAdapter implements AppAdapter { @Memoize() adjustImportsOptionsPath(): string { let file = join(this.root, '_adjust_imports.json'); - writeFileSync(file, JSON.stringify(this.adjustImportsOptions())); + writeFileSync(file, JSON.stringify(this.resolverConfig())); return file; } @Memoize() - adjustImportsOptions(): AdjustImportsOptions { + resolverConfig(): ResolverOptions { return this.makeAdjustImportOptions(true); } // this gets serialized out by babel plugin and ast plugin - private makeAdjustImportOptions(outer: boolean): AdjustImportsOptions { + private makeAdjustImportOptions(outer: boolean): ResolverOptions { let renamePackages = Object.assign({}, ...this.allActiveAddons.map(dep => dep.meta['renamed-packages'])); let renameModules = Object.assign({}, ...this.allActiveAddons.map(dep => dep.meta['renamed-modules'])); - let activeAddons: AdjustImportsOptions['activeAddons'] = {}; + let activeAddons: ResolverOptions['activeAddons'] = {}; for (let addon of this.allActiveAddons) { activeAddons[addon.name] = addon.root; } diff --git a/packages/compat/src/resolver.ts b/packages/compat/src/resolver.ts index 451e1a2cc..9ed21e94f 100644 --- a/packages/compat/src/resolver.ts +++ b/packages/compat/src/resolver.ts @@ -6,10 +6,15 @@ import { PreprocessedComponentRule, preprocessComponentRule, } from './dependency-rules'; -import { Package, PackageCache, explicitRelative, extensionsPattern } from '@embroider/core'; +import { + Package, + PackageCache, + explicitRelative, + extensionsPattern, + ResolverOptions as CoreResolverOptions, +} from '@embroider/core'; import { dirname, join, relative, sep } from 'path'; -import { Options as AdjustImportsOptions } from '@embroider/core/src/babel-plugin-adjust-imports'; import { Memoize } from 'typescript-memoize'; import Options from './options'; import { dasherize, snippetToDasherizedName } from './dasherize-component-name'; @@ -146,7 +151,7 @@ interface RehydrationParamsWithFile extends RehydrationParamsBase { } interface RehydrationParamsWithOptions extends RehydrationParamsBase { - adjustImportsOptions: AdjustImportsOptions; + adjustImportsOptions: CoreResolverOptions; } type RehydrationParams = RehydrationParamsWithFile | RehydrationParamsWithOptions; @@ -214,7 +219,7 @@ export default class CompatResolver { } @Memoize() - get adjustImportsOptions(): AdjustImportsOptions { + get adjustImportsOptions(): CoreResolverOptions { const { params } = this; return 'adjustImportsOptionsPath' in params ? // eslint-disable-next-line @typescript-eslint/no-require-imports diff --git a/packages/compat/tests/resolver.test.ts b/packages/compat/tests/resolver.test.ts index ec0ea47fc..3d3fbf71d 100644 --- a/packages/compat/tests/resolver.test.ts +++ b/packages/compat/tests/resolver.test.ts @@ -1,9 +1,8 @@ import { removeSync, mkdtempSync, writeFileSync, ensureDirSync, writeJSONSync, realpathSync } from 'fs-extra'; import { join, dirname } from 'path'; import Options, { optionsWithDefaults } from '../src/options'; -import { hbsToJS, tmpdir, throwOnWarnings } from '@embroider/core'; +import { hbsToJS, tmpdir, throwOnWarnings, ResolverOptions } from '@embroider/core'; import { emberTemplateCompiler } from '@embroider/test-support'; -import { Options as AdjustImportsOptions } from '@embroider/core/src/babel-plugin-adjust-imports'; import Resolver from '../src/resolver'; import { PackageRules } from '../src'; import type { AST, ASTPluginEnvironment } from '@glimmer/syntax'; @@ -18,7 +17,7 @@ describe('compat-resolver', function () { compatOptions: Options, otherOptions: { podModulePrefix?: string; - adjustImportsImports?: Partial; + adjustImportsImports?: Partial; plugins?: Transform[]; startingFrom?: 'hbs' | 'js'; } = {} diff --git a/packages/core/src/app.ts b/packages/core/src/app.ts index 076bed314..4ed8a996d 100644 --- a/packages/core/src/app.ts +++ b/packages/core/src/app.ts @@ -25,7 +25,7 @@ import Options from './options'; import { MacrosConfig } from '@embroider/macros/src/node'; import { PluginItem, TransformOptions } from '@babel/core'; import { makePortable } from './portable-babel-config'; -import { Options as AdjustImportsOptions } from './babel-plugin-adjust-imports'; +import { Options as ResolverConfig } from './module-resolver'; import { mangledEngineRoot } from './engine-mangler'; import { AppFiles, Engine, EngineSummary, RouteFiles } from './app-files'; import partition from 'lodash/partition'; @@ -106,7 +106,7 @@ export interface AppAdapter { // describes the special module naming rules that we need to achieve // compatibility - adjustImportsOptions(): AdjustImportsOptions; + resolverConfig(): ResolverConfig; adjustImportsOptionsPath(): string; @@ -254,7 +254,7 @@ export class AppBuilder { @Memoize() private get resolvableExtensionsPattern(): RegExp { - return extensionsPattern(this.adapter.adjustImportsOptions().resolvableExtensions); + return extensionsPattern(this.adapter.resolverConfig().resolvableExtensions); } private impliedAssets( @@ -422,9 +422,7 @@ export class AppBuilder { colocationOptions, ]); - // TODO: Keeping this just for the side effect of emitting the config json - this.adjustImportsPlugin(appFiles); - // babel.plugins.push(); + babel.plugins.push(this.adjustImportsPlugin(appFiles)); // we can use globally shared babel runtime by default babel.plugins.push([ @@ -438,7 +436,7 @@ export class AppBuilder { } private adjustImportsPlugin(engines: Engine[]): PluginItem { - let relocatedFiles: AdjustImportsOptions['relocatedFiles'] = {}; + let relocatedFiles: ResolverConfig['relocatedFiles'] = {}; for (let { destPath, appFiles } of engines) { for (let [relativePath, originalPath] of appFiles.relocatedFiles) { relocatedFiles[join(destPath, relativePath)] = originalPath; @@ -925,7 +923,7 @@ export class AppBuilder { majorVersion: this.adapter.babelMajorVersion(), fileFilter: '_babel_filter_.js', }, - 'resolvable-extensions': this.adapter.adjustImportsOptions().resolvableExtensions, + 'resolvable-extensions': this.adapter.resolverConfig().resolvableExtensions, 'root-url': this.adapter.rootURL(), }; diff --git a/packages/core/src/babel-plugin-adjust-imports.ts b/packages/core/src/babel-plugin-adjust-imports.ts index 34d1c926e..7a4a3cde8 100644 --- a/packages/core/src/babel-plugin-adjust-imports.ts +++ b/packages/core/src/babel-plugin-adjust-imports.ts @@ -1,22 +1,16 @@ -import { join, dirname } 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'; import { ImportUtil } from 'babel-import-util'; -import { randomBytes } from 'crypto'; -import { outputFileSync, pathExistsSync, renameSync } from 'fs-extra'; -import { explicitRelative } from '@embroider/shared-internals'; -import { compile } from './js-handlebars'; -import { Options, Resolver } from './module-resolver'; -import assertNever from 'assert-never'; +import { Options as ModuleResolveroptions } from './module-resolver'; + +export type Options = Pick; interface State { - resolver: Resolver; opts: Options | DeflatedOptions; } -export { Options }; - export interface DeflatedOptions { adjustImportsOptionsPath: string; relocatedFilesPath: string; @@ -78,81 +72,14 @@ export default function main(babel: typeof Babel) { Program: { enter(path: NodePath, state: State) { let opts = ensureOpts(state); - state.resolver = new Resolver(path.hub.file.opts.filename, opts); let adder = new ImportUtil(t, path); addExtraImports(adder, t, path, opts.extraImports); }, - exit(path: NodePath, state: State) { - for (let child of path.get('body')) { - if (child.isImportDeclaration() || child.isExportNamedDeclaration() || child.isExportAllDeclaration()) { - rewriteTopLevelImport(child, state); - } - } - }, - }, - CallExpression(path: NodePath, state: State) { - if (isImportSyncExpression(t, path) || isDynamicImportExpression(t, path)) { - const [source] = path.get('arguments'); - resolve((source.node as any).value, true, state, newSpecifier => { - source.replaceWith(t.stringLiteral(newSpecifier)); - }); - return; - } - - // Should/can we make this early exit when the first define was found? - if (!isDefineExpression(t, path)) { - return; - } - - let pkg = state.resolver.owningPackage(); - if (pkg && pkg.isV2Ember() && !pkg.meta['auto-upgraded']) { - throw new Error( - `The file ${state.resolver.originalFilename} in package ${pkg.name} tried to use AMD define. Native V2 Ember addons are forbidden from using AMD define, they must use ECMA export only.` - ); - } - - const dependencies = path.node.arguments[1]; - - const specifiers = dependencies.elements.slice(); - specifiers.push(path.node.arguments[0]); - - for (const source of specifiers) { - if (!source) { - continue; - } - - if (source.type !== 'StringLiteral') { - throw path.buildCodeFrameError(`expected only string literal arguments`); - } - - if (source.value === 'exports' || source.value === 'require') { - // skip "special" AMD dependencies - continue; - } - - resolve(source.value, false, state, newSpecifier => { - source.value = newSpecifier; - }); - } }, }, }; } -function rewriteTopLevelImport( - path: NodePath, - state: State -) { - const { source } = path.node; - if (source === null || source === undefined) { - return; - } - - resolve(source.value, false, state, newSpecifier => { - source.value = newSpecifier; - }); -} - (main as any).baseDir = function () { return join(__dirname, '..'); }; @@ -192,93 +119,3 @@ function ensureOpts(state: State): Options { } return opts; } - -function makeExternal(specifier: string, sourceFile: string, opts: Options): string { - let target = join(opts.externalsDir, specifier + '.js'); - atomicWrite( - target, - externalTemplate({ - runtimeName: specifier, - }) - ); - return explicitRelative(dirname(sourceFile), target.slice(0, -3)); -} - -function atomicWrite(path: string, content: string) { - if (pathExistsSync(path)) { - return; - } - let suffix = randomBytes(8).toString('hex'); - outputFileSync(path + suffix, content); - try { - renameSync(path + suffix, path); - } catch (err: any) { - // windows throws EPERM for concurrent access. For us it's not an error - // condition because the other thread is writing the exact same value we - // would have. - if (err.code !== 'EPERM') { - throw err; - } - } -} - -function makeMissingModule(specifier: string, sourceFile: string, opts: Options): string { - let target = join(opts.externalsDir, 'missing', specifier + '.js'); - atomicWrite( - target, - dynamicMissingModule({ - moduleName: specifier, - }) - ); - return explicitRelative(dirname(sourceFile), target.slice(0, -3)); -} - -const dynamicMissingModule = compile(` - throw new Error('Could not find module \`{{{js-string-escape moduleName}}}\`'); -`) as (params: { moduleName: string }) => string; - -const externalTemplate = compile(` -{{#if (eq runtimeName "require")}} -const m = window.requirejs; -{{else}} -const m = window.require("{{{js-string-escape runtimeName}}}"); -{{/if}} -{{!- - There are plenty of hand-written AMD defines floating around - that lack this, and they will break when other build systems - encounter them. - - As far as I can tell, Ember's loader was already treating this - case as a module, so in theory we aren't breaking anything by - marking it as such when other packagers come looking. - - todo: get review on this part. --}} -if (m.default && !m.__esModule) { - m.__esModule = true; -} -module.exports = m; -`) as (params: { runtimeName: string }) => string; - -function resolve(specifier: string, isDynamic: boolean, state: State, setter: (specifier: string) => void) { - let resolution = state.resolver.resolve(specifier, isDynamic); - let newSpecifier: string | undefined; - switch (resolution.result) { - case 'continue': - return; - case 'external': - newSpecifier = makeExternal(resolution.specifier, state.resolver.filename, ensureOpts(state)); - break; - case 'redirect-to': - newSpecifier = resolution.specifier; - break; - case 'runtime-failure': - newSpecifier = makeMissingModule(resolution.specifier, state.resolver.filename, ensureOpts(state)); - break; - default: - throw assertNever(resolution); - } - if (newSpecifier) { - setter(newSpecifier); - } -} diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index a4fb6bf8a..d71a15d30 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -290,7 +290,7 @@ function reliablyResolvable(pkg: V2Package, packageName: string) { function external(specifier: string): Resolution { return { result: 'virtual', - filename: `@embroider/external/${specifier}`, + filename: specifier, content: externalShim({ moduleName: specifier }), }; } diff --git a/packages/core/tests/babel-plugin-adjust-imports.test.ts b/packages/core/tests/babel-plugin-adjust-imports.test.ts deleted file mode 100644 index d202c5cb4..000000000 --- a/packages/core/tests/babel-plugin-adjust-imports.test.ts +++ /dev/null @@ -1,129 +0,0 @@ -import main, { - isDefineExpression, - isDynamicImportExpression, - isImportSyncExpression, - Options as AdjustImportsOptions, -} from '../src/babel-plugin-adjust-imports'; -import { transformSync } from '@babel/core'; -import { parse } from '@babel/parser'; -import traverse from '@babel/traverse'; -import { types as t } from '@babel/core'; - -describe('babel-plugin-adjust-imports', function () { - function getFirstCallExpresssionPath(source: string) { - const ast: any = parse(source, { sourceType: 'module' }); - let path: any; - - traverse(ast, { - CallExpression(_path: any) { - if (path) { - return; - } - path = _path; - }, - }); - - return path; - } - - function isDefineExpressionFromSource(source: string) { - return isDefineExpression(t, getFirstCallExpresssionPath(source)); - } - - function isImportSyncExpressionFromSource(source: string) { - return isImportSyncExpression(t, getFirstCallExpresssionPath(source)); - } - function isDynamicImportExpressionFromSource(source: string) { - return isDynamicImportExpression(t, getFirstCallExpresssionPath(source)); - } - - test('isDefineExpression works', function () { - expect(isDefineExpressionFromSource(`apple()`)).toBe(false); - expect(isDefineExpressionFromSource(`(apple())`)).toBe(false); - expect(isDefineExpressionFromSource(`(define('module', [], function() { }))`)).toBe(true); - expect(isDefineExpressionFromSource(`define('module', [], function() {});`)).toBe(true); - expect(isDefineExpressionFromSource(`define('foo', ['apple'], function() {});`)).toBe(true); - expect(isDefineExpressionFromSource(`define;define('module', [], function() {});`)).toBe(true); - expect(isDefineExpressionFromSource(`define;define('module', function() {});`)).toBe(false); - expect(isDefineExpressionFromSource(`define;define('module');`)).toBe(false); - expect(isDefineExpressionFromSource(`define;define(1, [], function() { });`)).toBe(false); - expect(isDefineExpressionFromSource(`define;define('b/a/c', ['a', 'b', 'c'], function() { });`)).toBe(true); - expect(isDefineExpressionFromSource(`import foo from 'foo'; define('apple')`)).toBe(false); - expect(isDefineExpressionFromSource(`define('apple'); import foo from 'foo'`)).toBe(false); - }); - - test('isImportSyncExpression works', function () { - expect( - isImportSyncExpressionFromSource(` - import { importSync } from '@embroider/macros'; - importSync('foo'); - `) - ).toBe(true); - expect( - isImportSyncExpressionFromSource(` - import { importSync as i } from '@embroider/macros'; - i('foo'); - `) - ).toBe(true); - expect( - isImportSyncExpressionFromSource(` - import { foo as importSync } from 'foobar'; - importSync('foo'); - `) - ).toBe(false); - expect( - isImportSyncExpressionFromSource(` - import { foo as i } from 'foobar'; - i('foo'); - `) - ).toBe(false); - }); - - test('isDynamicImportExpression works', function () { - expect(isDynamicImportExpressionFromSource(`import('foo');`)).toBe(true); - expect(isDynamicImportExpressionFromSource(`async () => { await import('foo'); }`)).toBe(true); - expect(isDynamicImportExpressionFromSource(`import foo from 'foo';`)).toBe(false); - }); - - test('main', function () { - const options: AdjustImportsOptions = { - activeAddons: {}, - renameModules: { a: 'c' }, - renamePackages: { module: 'other-module', apple: 'banana' }, - extraImports: [], - relocatedFiles: {}, - externalsDir: 'test', - resolvableExtensions: ['.js', '.hbs'], - appRoot: '/nonexistent', - }; - - { - const { code } = transformSync(`define('module', ['a', 'b', 'c'], function() {})`, { - plugins: [[main, options]], - filename: 'some-file.js', - }) as any; - - expect(code).toBe(`define("other-module", ["c", 'b', 'c'], function () {});`); - } - - { - const { code } = transformSync(`define('module', ['module/a', 'module/b', 'module/c'], function() {})`, { - plugins: [[main, options]], - filename: 'some-file.js', - }) as any; - - expect(code).toBe( - `define("other-module", ["other-module/a", "other-module/b", "other-module/c"], function () {});` - ); - } - - { - const { code } = transformSync(`import apple from 'apple'`, { - plugins: [[main, options]], - filename: 'some-file.js', - }) as any; - - expect(code).toBe(`import apple from "banana";`); - } - }); -}); diff --git a/packages/webpack/src/ember-webpack.ts b/packages/webpack/src/ember-webpack.ts index a84fe2390..2788b79b8 100644 --- a/packages/webpack/src/ember-webpack.ts +++ b/packages/webpack/src/ember-webpack.ts @@ -627,7 +627,9 @@ const Webpack: PackagerConstructor = class Webpack implements Packager error.line = (error.loc ? error.loc.line : null) || (error.location ? error.location.line : null); } if (typeof error.message === 'string') { - error.message = error.message.replace(error.module.context, error.module.userRequest); + if (error.module?.context) { + error.message = error.message.replace(error.module.context, error.module.userRequest); + } // the tmpdir on OSX is horribly long and makes error messages hard to // read. This is doing the same as String.prototype.replaceAll, which node diff --git a/packages/webpack/src/webpack-resolver-plugin.ts b/packages/webpack/src/webpack-resolver-plugin.ts index 6ad5dac3a..f5b5e00a6 100644 --- a/packages/webpack/src/webpack-resolver-plugin.ts +++ b/packages/webpack/src/webpack-resolver-plugin.ts @@ -46,10 +46,10 @@ class ResolverPlugin { callback: (err?: Error | null, result?: any) => void ) { if (resolution.result === 'virtual') { - this.vfs.writeModule(`node_modules/${resolution.filename}`, resolution.content); + this.vfs.writeModule(`/@embroider/externals/${resolution.filename}`, resolution.content); resolution = { result: 'alias', - specifier: resolution.filename, + specifier: `/@embroider/externals/${resolution.filename}`, }; } @@ -91,6 +91,7 @@ class ResolverPlugin { return; } case 'continue': + callback(); return; default: throw assertNever(resolution); @@ -102,8 +103,9 @@ class ResolverPlugin { // webpack's built-in `resolve.alias` takes effect. It's supposed to take // precedence over other resolving decisions. resolver.getHook('raw-resolve').tapAsync('my-resolver-plugin', async (request, context, callback) => { - if (!isRelevantRequest(request)) { - return callback(); + if (!isRelevantRequest(request) || request.request.startsWith('@embroider/externals/')) { + callback(); + return; } let result = this.resolver.beforeResolve(request.request, request.context.issuer); this.#resolve(result, resolver, request, context, callback); @@ -122,8 +124,9 @@ class ResolverPlugin { // defaults (tapable assigned them stage 0 by default). { name: 'my-resolver-plugin', stage: 10 }, async (request, context, callback) => { - if (!isRelevantRequest(request)) { - return callback(); + if (!isRelevantRequest(request) || request.request.startsWith('@embroider/externals/')) { + callback(); + return; } let result = this.resolver.fallbackResolve(request.request, request.context.issuer); this.#resolve(result, resolver, request, context, callback); diff --git a/yarn.lock b/yarn.lock index 49d2cdcd4..008e2b0c6 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5404,26 +5404,7 @@ browserslist-generator@^1.0.66: semver "^7.3.7" ua-parser-js "^1.0.2" -browserslist@4.20.2: - version "4.20.2" - resolved "https://registry.yarnpkg.com/browserslist/-/browserslist-4.20.2.tgz#567b41508757ecd904dab4d1c646c612cd3d4f88" - integrity sha512-CQOBCqp/9pDvDbx3xfMi+86pr4KXIf2FDkTTdeuYw8OxS9t898LA1Khq57gtufFILXpfgsSx5woNgsBgvGjpsA== - dependencies: - caniuse-lite "^1.0.30001317" - electron-to-chromium "^1.4.84" - escalade "^3.1.1" - node-releases "^2.0.2" - picocolors "^1.0.0" - -browserslist@^3.2.6: - version "3.2.8" - resolved "https://registry.yarnpkg.com/browserslist/-/browserslist-3.2.8.tgz#b0005361d6471f0f5952797a76fc985f1f978fc6" - integrity sha512-WHVocJYavUwVgVViC0ORikPHQquXwVh939TaelZ4WDqpWgTX/FsGhl/+P4qBUAGcRvtOgDgC+xftNWWp2RUTAQ== - dependencies: - caniuse-lite "^1.0.30000844" - electron-to-chromium "^1.3.47" - -browserslist@^4.0.0, browserslist@^4.14.5, browserslist@^4.20.4, browserslist@^4.21.3, browserslist@^4.21.4: +browserslist@4.20.2, browserslist@^3.2.6, browserslist@^4.0.0, browserslist@^4.14.0, browserslist@^4.14.5, browserslist@^4.20.4, browserslist@^4.21.3, browserslist@^4.21.4: version "4.21.4" resolved "https://registry.yarnpkg.com/browserslist/-/browserslist-4.21.4.tgz#e7496bbc67b9e39dd0f98565feccdcb0d4ff6987" integrity sha512-CBHJJdDmgjl3daYjN5Cp5kbTf1mUhZoS+beLklHIvkOWscs83YAhLlF3Wsh/lciQYAcbBJgTOD44VtG31ZM4Hw== @@ -5582,11 +5563,6 @@ caniuse-lite@^1.0.0, caniuse-lite@^1.0.30001328, caniuse-lite@^1.0.30001400: resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001439.tgz#ab7371faeb4adff4b74dad1718a6fd122e45d9cb" integrity sha512-1MgUzEkoMO6gKfXflStpYgZDlFM7M/ck/bgfVCACO5vnAf0fXoNVHdWtqGU+MYca+4bL9Z5bpOVmR33cWW9G2A== -caniuse-lite@^1.0.30000844, caniuse-lite@^1.0.30001317: - version "1.0.30001444" - resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001444.tgz#c0a530776eb44d933b493de1d05346f2527b30fc" - integrity sha512-ecER9xgJQVMqcrxThKptsW0pPxSae8R2RB87LNa+ivW9ppNWRHEplXcDzkCOP4LYWGj8hunXLqaiC41iBATNyg== - capture-exit@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/capture-exit/-/capture-exit-2.0.0.tgz#fb953bfaebeb781f62898239dabb426d08a509a4" @@ -6683,7 +6659,7 @@ ee-first@1.1.1: resolved "https://registry.yarnpkg.com/ee-first/-/ee-first-1.1.1.tgz#590c61156b0ae2f4f0255732a158b266bc56b21d" integrity sha512-WMwm9LhRUo+WUaRN+vRuETqG89IgZphVSNkdFgeb6sS/E4OrDIN7t48CAewSHXc6C8lefD8KKfr5vY61brQlow== -electron-to-chromium@^1.3.47, electron-to-chromium@^1.4.251, electron-to-chromium@^1.4.84: +electron-to-chromium@^1.4.251: version "1.4.284" resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.4.284.tgz#61046d1e4cab3a25238f6bf7413795270f125592" integrity sha512-M8WEXFuKXMYMVr45fo8mq0wUrrJHheiKZf6BArTKk9ZBYCKJEOU5H8cdWgDT+qCVZf7Na4lVUaZsA+h6uA9+PA== @@ -10077,7 +10053,7 @@ got@^9.6.0: to-readable-stream "^1.0.0" url-parse-lax "^3.0.0" -graceful-fs@^4.1.2, graceful-fs@^4.1.3, graceful-fs@^4.1.5, graceful-fs@^4.1.6, graceful-fs@^4.1.9, graceful-fs@^4.2.0, graceful-fs@^4.2.2, graceful-fs@^4.2.4, graceful-fs@^4.2.9: +graceful-fs@^4.0.0, graceful-fs@^4.1.2, graceful-fs@^4.1.3, graceful-fs@^4.1.5, graceful-fs@^4.1.6, graceful-fs@^4.1.9, graceful-fs@^4.2.0, graceful-fs@^4.2.2, graceful-fs@^4.2.4, graceful-fs@^4.2.9: version "4.2.10" resolved "https://registry.yarnpkg.com/graceful-fs/-/graceful-fs-4.2.10.tgz#147d3a006da4ca3ce14728c7aefc287c367d7a6c" integrity sha512-9ByhssR2fPVsNZj478qUUbKfmL0+t5BDVyjShtyZZLiK7ZDAArFFfopyOTj0M05wE2tJPisA4iTnnXl2YoPvOA== @@ -12749,7 +12725,7 @@ node-notifier@^10.0.0: uuid "^8.3.2" which "^2.0.2" -node-releases@^2.0.2, node-releases@^2.0.6: +node-releases@^2.0.6: version "2.0.8" resolved "https://registry.yarnpkg.com/node-releases/-/node-releases-2.0.8.tgz#0f349cdc8fcfa39a92ac0be9bc48b7706292b9ae" integrity sha512-dFSmB8fFHEH/s81Xi+Y/15DQY6VHW81nXRj86EMSL3lmuTmK1e+aT4wrFCkTbm+gSwkw4KpX+rT/pMM2c1mF+A== @@ -13726,7 +13702,7 @@ qunit-dom@^2.0.0: ember-cli-babel "^7.23.0" ember-cli-version-checker "^5.1.1" -qunit@^2.16.0, qunit@^2.19.1: +qunit@^2.14.1, qunit@^2.16.0, qunit@^2.19.1: version "2.19.3" resolved "https://registry.yarnpkg.com/qunit/-/qunit-2.19.3.tgz#bcf81a2e8d176dc19fe8dd358c4cbd08619af03a" integrity sha512-vEnspSZ37u2oR01OA/IZ1Td5V7BvQYFECdKPv86JaBplDNa5IHg0v7jFSPoP5L5o78Dbi8sl7/ATtpRDAKlSdw== From acf0962e5f637996aa7616d6ab4c4d840819dfe2 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 19 Jan 2023 14:54:36 -0500 Subject: [PATCH 03/16] pipe exceptions back to webpack --- .../webpack/src/webpack-resolver-plugin.ts | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/packages/webpack/src/webpack-resolver-plugin.ts b/packages/webpack/src/webpack-resolver-plugin.ts index f5b5e00a6..115307946 100644 --- a/packages/webpack/src/webpack-resolver-plugin.ts +++ b/packages/webpack/src/webpack-resolver-plugin.ts @@ -103,12 +103,16 @@ class ResolverPlugin { // webpack's built-in `resolve.alias` takes effect. It's supposed to take // precedence over other resolving decisions. resolver.getHook('raw-resolve').tapAsync('my-resolver-plugin', async (request, context, callback) => { - if (!isRelevantRequest(request) || request.request.startsWith('@embroider/externals/')) { - callback(); - return; + try { + if (!isRelevantRequest(request) || request.request.startsWith('@embroider/externals/')) { + callback(); + return; + } + let result = this.resolver.beforeResolve(request.request, request.context.issuer); + this.#resolve(result, resolver, request, context, callback); + } catch (err) { + callback(err); } - let result = this.resolver.beforeResolve(request.request, request.context.issuer); - this.#resolve(result, resolver, request, context, callback); }); // described-resolve -> internal-resolve is the same place in the pipeline @@ -124,12 +128,16 @@ class ResolverPlugin { // defaults (tapable assigned them stage 0 by default). { name: 'my-resolver-plugin', stage: 10 }, async (request, context, callback) => { - if (!isRelevantRequest(request) || request.request.startsWith('@embroider/externals/')) { - callback(); - return; + try { + if (!isRelevantRequest(request) || request.request.startsWith('@embroider/externals/')) { + callback(); + return; + } + let result = this.resolver.fallbackResolve(request.request, request.context.issuer); + this.#resolve(result, resolver, request, context, callback); + } catch (err) { + callback(err); } - let result = this.resolver.fallbackResolve(request.request, request.context.issuer); - this.#resolve(result, resolver, request, context, callback); } ); } From d351d8df0e53f83ae88334e74f055136a9bca671 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 19 Jan 2023 15:24:29 -0500 Subject: [PATCH 04/16] bugfixes --- 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 d71a15d30..31a467e9d 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -47,7 +47,7 @@ export class Resolver { if (resolution.result === 'continue' && maybeRenamed !== specifier) { return { result: 'alias', specifier: maybeRenamed }; } - return { result: 'continue' }; + return resolution; } fallbackResolve(specifier: string, fromFile: string): Resolution { @@ -280,7 +280,7 @@ function reliablyResolvable(pkg: V2Package, packageName: string) { return true; } - if (emberVirtualPeerDeps.has(packageName)) { + if (emberVirtualPeerDeps.has(packageName) || emberVirtualPackages.has(packageName)) { return true; } From 9e236d2e5607c3cc09b9b2a9db4ae59bc6dd716e Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 19 Jan 2023 18:24:18 -0500 Subject: [PATCH 05/16] starting to port compat renaming tests --- tests/scenarios/compat-renaming-test.ts | 105 +++++++++++++++++++----- 1 file changed, 83 insertions(+), 22 deletions(-) diff --git a/tests/scenarios/compat-renaming-test.ts b/tests/scenarios/compat-renaming-test.ts index 450afaad2..d0806356b 100644 --- a/tests/scenarios/compat-renaming-test.ts +++ b/tests/scenarios/compat-renaming-test.ts @@ -9,6 +9,7 @@ import { definesPattern, ExpectFile, expectFilesAt, Transpiler } from '@embroide import { throwOnWarnings } from '@embroider/core'; import merge from 'lodash/merge'; +import { Audit, AuditResults } from '@embroider/compat/src/audit'; appScenarios .map('compat-renaming', app => { @@ -155,57 +156,117 @@ appScenarios let app: PreparedApp; let expectFile: ExpectFile; + let expectAudit: ExpectAuditResults; let build: Transpiler; + let result: AuditResults; - hooks.before(async assert => { + class ExpectAuditResults { + constructor(private result: AuditResults, private assert: Assert) {} + + module(name: string) { + let m = this.result.modules[name]; + if (!m) { + this.assert.pushResult({ + result: false, + actual: `${name} is not in audit results`, + expected: `${name} in audit results`, + }); + } + return new ExpectModule(this.assert, m); + } + } + + class ExpectModule { + constructor(private assert: Assert, private module: AuditResults['modules'][string] | undefined) {} + + resolves(specifier: string) { + return { + to: (filename: string) => { + if (this.module) { + if (specifier in this.module.resolutions) { + this.assert.pushResult({ + result: this.module.resolutions[specifier] === filename, + expected: filename, + actual: this.module.resolutions[specifier], + }); + } else { + this.assert.pushResult({ + result: false, + expected: specifier, + actual: Object.keys(this.module.resolutions), + }); + } + } + }, + }; + } + } + + hooks.before(async () => { app = await scenario.prepare(); - let result = await app.execute('ember build', { env: { STAGE2_ONLY: 'true' } }); - assert.equal(result.exitCode, 0, result.output); + result = await Audit.run({ app: app.dir, 'reuse-build': false }); }); hooks.beforeEach(assert => { expectFile = expectFilesAt(readFileSync(join(app.dir, 'dist/.stage2-output'), 'utf8'), { qunit: assert }); + expectAudit = new ExpectAuditResults(result, assert); build = new Transpiler(expectFile.basePath); }); - test('whole package renaming works for top-level module', function () { - let assertFile = expectFile('components/import-lodash.js').transform(build.transpile); - assertFile.matches(/import lodash from ["']ember-lodash["']/); + test('whole package renaming works for top-level module', function (assert) { + expectAudit + .module('./components/import-lodash.js') + .resolves('lodash') + .to('./node_modules/ember-lodash/index.js'); + assert.equal( + result.modules['./components/import-lodash.js']?.resolutions['lodash'], + './node_modules/ember-lodash/index.js' + ); expectFile('node_modules/ember-lodash/index.js').matches(/lodash index/); }); + test('whole package renaming works for interior module', function () { - let assertFile = expectFile('components/import-capitalize.js').transform(build.transpile); - assertFile.matches(/import capitalize from ["']ember-lodash\/capitalize["']/); + expectAudit + .module('./components/import-capitalize.js') + .resolves('lodash/capitalize') + .to('./node_modules/ember-lodash/capitalize.js'); + expectFile('node_modules/ember-lodash/capitalize.js').matches(/lodash capitalize/); }); + test("modules in own namespace don't get renamed", function () { - let assertFile = expectFile('components/import-own-thing.js').transform(build.transpile); - assertFile.matches(/import ownThing from ["']emits-multiple-packages\/own-thing["']/); + expectAudit + .module('./components/import-own-thing.js') + .resolves('emits-multiple-packages/own-thing') + .to('./node_modules/emits-multiple-packages/own-thing.js'); expectFile('node_modules/emits-multiple-packages/own-thing.js').matches(/own thing/); }); + test('modules outside our namespace do get renamed', function () { - let assertFile = expectFile('components/import-somebody-elses.js').transform(build.transpile); - assertFile.matches( - /import environment from ["']emits-multiple-packages\/somebody-elses-package\/environment(\.js)?["']/ - ); + expectAudit + .module('./components/import-somebody-elses.js') + .resolves('somebody-elses-package/environment') + .to('./node_modules/emits-multiple-packages/somebody-elses-package/environment.js'); expectFile('node_modules/emits-multiple-packages/somebody-elses-package/environment.js').matches( /somebody elses environment/ ); }); + test('modules outside our namespace do get renamed, with index.js', function () { - let assertFile = expectFile('components/import-somebody-elses-utils.js').transform(build.transpile); - assertFile.matches( - /import environment from ["']emits-multiple-packages\/somebody-elses-package\/utils\/index\.js["']/ - ); + expectAudit + .module('./components/import-somebody-elses-utils.js') + .resolves('somebody-elses-package/utils') + .to('./node_modules/emits-multiple-packages/somebody-elses-package/utils/index.js'); expectFile('node_modules/emits-multiple-packages/somebody-elses-package/utils/index.js').matches( /somebody elses utils/ ); }); + test('modules outside our namespace do get renamed, with index', function () { - let assertFile = expectFile('components/import-somebody-elses-utils-index.js').transform(build.transpile); - assertFile.matches( - /import environment from ["']emits-multiple-packages\/somebody-elses-package\/utils\/index\.js["']/ - ); + expectAudit + .module('./components/import-somebody-elses-utils-index.js') + .resolves('somebody-elses-package/utils/index') + .to('./node_modules/emits-multiple-packages/somebody-elses-package/utils/index.js'); }); test('modules outside our namespace do get renamed, with index with extension', function () { let assertFile = expectFile('components/import-somebody-elses-utils-index-explicit.js').transform( From 482785799c26766996b32d2f384488ba8051868e Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 20 Jan 2023 15:00:28 -0500 Subject: [PATCH 06/16] fix rehomed path --- packages/core/src/module-resolver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index 31a467e9d..d35b29820 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -165,7 +165,7 @@ export class Resolver { } return { result: 'rehome', - fromFile: this.options.appRoot + './package.json', + fromFile: this.options.appRoot + '/package.json', }; } From 13bfe94a2d5ac4896aef8c27bf4d93fc337e17e8 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 20 Jan 2023 19:18:23 -0500 Subject: [PATCH 07/16] windows friendly path append --- 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 d35b29820..cff2ba06d 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -165,7 +165,7 @@ export class Resolver { } return { result: 'rehome', - fromFile: this.options.appRoot + '/package.json', + fromFile: resolve(this.options.appRoot, 'package.json'), }; } @@ -226,7 +226,7 @@ export class Resolver { // package return { result: 'rehome', - fromFile: relocatedPkg.root + '/package.json', + fromFile: resolve(relocatedPkg.root, 'package.json'), }; } From 21811c5b0188db8920bdcabe4c69cc9cee689fa7 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Sat, 21 Jan 2023 19:58:51 -0500 Subject: [PATCH 08/16] updating more tests --- tests/scenarios/compat-renaming-test.ts | 45 ++++++++++++++----------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/tests/scenarios/compat-renaming-test.ts b/tests/scenarios/compat-renaming-test.ts index d0806356b..b9afa7204 100644 --- a/tests/scenarios/compat-renaming-test.ts +++ b/tests/scenarios/compat-renaming-test.ts @@ -181,7 +181,7 @@ appScenarios resolves(specifier: string) { return { - to: (filename: string) => { + to: (filename: string | null) => { if (this.module) { if (specifier in this.module.resolutions) { this.assert.pushResult({ @@ -269,12 +269,10 @@ appScenarios .to('./node_modules/emits-multiple-packages/somebody-elses-package/utils/index.js'); }); test('modules outside our namespace do get renamed, with index with extension', function () { - let assertFile = expectFile('components/import-somebody-elses-utils-index-explicit.js').transform( - build.transpile - ); - assertFile.matches( - /import environment from ["']emits-multiple-packages\/somebody-elses-package\/utils\/index\.js["']/ - ); + expectAudit + .module('./components/import-somebody-elses-utils-index-explicit.js') + .resolves('somebody-elses-package/utils/index.js') + .to('./node_modules/emits-multiple-packages/somebody-elses-package/utils/index.js'); }); test('renamed modules keep their classic runtime name when used as implicit-modules', function () { let assertFile = expectFile('assets/app-template.js').transform(build.transpile); @@ -286,25 +284,34 @@ appScenarios ); }); test('rewriting one modules does not capture entire package namespace', function () { - let assertFile = expectFile('components/import-somebody-elses-original.js').transform(build.transpile); - assertFile.matches(/import topLevel from ["']somebody-elses-package["']/); - assertFile.matches(/import deeper from ["']somebody-elses-package\/deeper["']/); + expectAudit + .module('./components/import-somebody-elses-original.js') + .resolves('somebody-elses-package') + .to(null); + + expectAudit + .module('./components/import-somebody-elses-original.js') + .resolves('somebody-elses-package/deeper') + .to(null); }); test('single file package gets captured and renamed', function () { - let assertFile = expectFile('components/import-single-file-package.js').transform(build.transpile); - assertFile.matches(/import whatever from ["']emits-multiple-packages\/single-file-package\/index.js['"]/); + expectAudit + .module('./components/import-single-file-package.js') + .resolves('single-file-package') + .to('./node_modules/emits-multiple-packages/single-file-package/index.js'); expectFile('./node_modules/emits-multiple-packages/single-file-package/index.js').matches( /single file package/ ); }); test('files copied into app from addons resolve their own original packages', function () { - let assertFile = expectFile('first.js').transform(build.transpile); - assertFile.matches(/export \{ default \} from ['"]\.\/node_modules\/has-app-tree-import['"]/); - - assertFile = expectFile('second.js').transform(build.transpile); - assertFile.matches( - /export \{ default \} from ['"]\.\/node_modules\/intermediate\/node_modules\/has-app-tree-import['"]/ - ); + expectAudit + .module('./first.js') + .resolves('has-app-tree-import') + .to('./node_modules/has-app-tree-import/index.js'); + expectAudit + .module('./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 () { let assertFile = expectFile('imports-dep.js').transform(build.transpile); From 11ed1c6f882c0faea3e7007845003887a7b8b05d Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 23 Jan 2023 16:01:23 -0500 Subject: [PATCH 09/16] clarify naming --- packages/core/src/module-resolver.ts | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/core/src/module-resolver.ts b/packages/core/src/module-resolver.ts index cff2ba06d..3daa1a81e 100644 --- a/packages/core/src/module-resolver.ts +++ b/packages/core/src/module-resolver.ts @@ -58,12 +58,12 @@ export class Resolver { return PackageCache.shared('embroider-stage3', this.options.appRoot).ownerOfFile(fromFile); } - private relocatedIntoPackage(fromFile: string): V2Package | undefined { + 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 to get relocated into a v2 ember package here`); + throw new Error(`bug: it should only be possible for a v2 ember package to own relocated files`); } return owning; } @@ -100,19 +100,19 @@ export class Resolver { if (pkg.meta['auto-upgraded'] && pkg.name === packageName) { // we found a self-import, resolve it for them. Only auto-upgraded - // packages get this help, v2 packages are natively supposed to use - // relative imports for their own modules, and we want to push them all to - // do that correctly. + // packages get this help, v2 packages are natively supposed to make their + // own modules resolvable, and we want to push them all to do that + // correctly. return specifier.replace(packageName, pkg.root); } - let relocatedIntoPkg = this.relocatedIntoPackage(fromFile); - if (relocatedIntoPkg && pkg.meta['auto-upgraded'] && relocatedIntoPkg.name === packageName) { + let originalPkg = this.originalPackage(fromFile); + if (originalPkg && pkg.meta['auto-upgraded'] && originalPkg.name === packageName) { // a file that was relocated into a package does a self-import of that // package's name. This can happen when an addon (like ember-cli-mirage) // emits files from its own treeForApp that contain imports of the app's own // fully qualified name. - return specifier.replace(packageName, relocatedIntoPkg.root); + return specifier.replace(packageName, originalPkg.root); } return specifier; @@ -186,8 +186,8 @@ export class Resolver { // assertions on what native v2 addons can import if (!pkg.meta['auto-upgraded']) { - let relocatedPkg = this.relocatedIntoPackage(fromFile); - if (relocatedPkg) { + 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 @@ -220,13 +220,13 @@ export class Resolver { return { result: 'continue' }; } - let relocatedPkg = this.relocatedIntoPackage(fromFile); - if (relocatedPkg) { + let originalPkg = this.originalPackage(fromFile); + if (originalPkg) { // we didn't find it from the original package, so try from the relocated // package return { result: 'rehome', - fromFile: resolve(relocatedPkg.root, 'package.json'), + fromFile: resolve(originalPkg.root, 'package.json'), }; } From 880b7d128670d923857930d4792641a1a603ff63 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 23 Jan 2023 16:01:35 -0500 Subject: [PATCH 10/16] update more tests --- tests/scenarios/compat-renaming-test.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/scenarios/compat-renaming-test.ts b/tests/scenarios/compat-renaming-test.ts index b9afa7204..f47bbe4b0 100644 --- a/tests/scenarios/compat-renaming-test.ts +++ b/tests/scenarios/compat-renaming-test.ts @@ -314,14 +314,16 @@ appScenarios .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 () { - let assertFile = expectFile('imports-dep.js').transform(build.transpile); - assertFile.matches( - /export \{ default \} from ['"]\.\/node_modules\/has-app-tree-import\/node_modules\/inner-dep['"]/ - ); + expectAudit + .module('./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 () { - let assertFile = expectFile('mirage/config.js').transform(build.transpile); - assertFile.matches(/import ['"]\.\.\/components\/import-lodash['"]/); + expectAudit + .module('./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); From f8500afd324c5655fc2058bd6b0d5b3b0a850d38 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 23 Jan 2023 16:01:46 -0500 Subject: [PATCH 11/16] auditor needs to know how to load whole config --- packages/compat/src/audit.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/compat/src/audit.ts b/packages/compat/src/audit.ts index 69b9e99ba..f7991e57d 100644 --- a/packages/compat/src/audit.ts +++ b/packages/compat/src/audit.ts @@ -252,8 +252,10 @@ export class Audit { @Memoize() private get resolverParams(): ResolverOptions { - // eslint-disable-next-line @typescript-eslint/no-require-imports - let config = require(join(this.appDir, '_adjust_imports.json')); + let config = { + ...readJSONSync(join(this.appDir, '_adjust_imports.json')), + ...readJSONSync(join(this.appDir, '_relocated_files.json')), + }; return config; } From 138065bf5f097916cebcfb3518f760c8517d28fb Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 23 Jan 2023 16:41:42 -0500 Subject: [PATCH 12/16] extract audit assertions and use them to update more tests --- test-packages/support/audit-assertions.ts | 70 +++++++++++++++++++++++ tests/scenarios/compat-renaming-test.ts | 56 ++---------------- tests/scenarios/compat-stage2-test.ts | 29 ++++++---- 3 files changed, 92 insertions(+), 63 deletions(-) create mode 100644 test-packages/support/audit-assertions.ts diff --git a/test-packages/support/audit-assertions.ts b/test-packages/support/audit-assertions.ts new file mode 100644 index 000000000..aaf2fde14 --- /dev/null +++ b/test-packages/support/audit-assertions.ts @@ -0,0 +1,70 @@ +import { Audit, AuditResults } from '@embroider/compat/src/audit'; + +/* + The audit tool in @embroider/compat can be used directly to tell you about + potential problems in an app that is trying to adopt embroider. But we also + take advantage of the audit tool within our test suite to help us analyze + Embroider's output. +*/ +export function setupAuditTest(hooks: NestedHooks, getAppDir: () => string) { + let result: AuditResults; + let expectAudit: ExpectAuditResults; + + hooks.before(async () => { + result = await Audit.run({ app: getAppDir(), 'reuse-build': false }); + }); + + hooks.beforeEach(assert => { + expectAudit = new ExpectAuditResults(result, assert); + }); + + return { + module(name: string) { + return expectAudit.module(name); + }, + }; +} + +class ExpectAuditResults { + constructor(private result: AuditResults, private assert: Assert) {} + + module(name: string) { + let m = this.result.modules[name]; + if (!m) { + this.assert.pushResult({ + result: false, + actual: `${name} is not in audit results`, + expected: `${name} in audit results`, + }); + } + return new ExpectModule(this.assert, m); + } +} + +class ExpectModule { + constructor(private assert: Assert, private module: AuditResults['modules'][string] | undefined) {} + + resolves(specifier: string) { + return { + to: (filename: string | null, message?: string) => { + if (this.module) { + if (specifier in this.module.resolutions) { + this.assert.pushResult({ + result: this.module.resolutions[specifier] === filename, + expected: filename, + actual: this.module.resolutions[specifier], + message, + }); + } else { + this.assert.pushResult({ + result: false, + expected: specifier, + actual: Object.keys(this.module.resolutions), + message, + }); + } + } + }, + }; + } +} diff --git a/tests/scenarios/compat-renaming-test.ts b/tests/scenarios/compat-renaming-test.ts index f47bbe4b0..109c8aadc 100644 --- a/tests/scenarios/compat-renaming-test.ts +++ b/tests/scenarios/compat-renaming-test.ts @@ -9,7 +9,7 @@ import { definesPattern, ExpectFile, expectFilesAt, Transpiler } from '@embroide import { throwOnWarnings } from '@embroider/core'; import merge from 'lodash/merge'; -import { Audit, AuditResults } from '@embroider/compat/src/audit'; +import { setupAuditTest } from '@embroider/test-support/audit-assertions'; appScenarios .map('compat-renaming', app => { @@ -156,72 +156,24 @@ appScenarios let app: PreparedApp; let expectFile: ExpectFile; - let expectAudit: ExpectAuditResults; let build: Transpiler; - let result: AuditResults; - - class ExpectAuditResults { - constructor(private result: AuditResults, private assert: Assert) {} - - module(name: string) { - let m = this.result.modules[name]; - if (!m) { - this.assert.pushResult({ - result: false, - actual: `${name} is not in audit results`, - expected: `${name} in audit results`, - }); - } - return new ExpectModule(this.assert, m); - } - } - - class ExpectModule { - constructor(private assert: Assert, private module: AuditResults['modules'][string] | undefined) {} - - resolves(specifier: string) { - return { - to: (filename: string | null) => { - if (this.module) { - if (specifier in this.module.resolutions) { - this.assert.pushResult({ - result: this.module.resolutions[specifier] === filename, - expected: filename, - actual: this.module.resolutions[specifier], - }); - } else { - this.assert.pushResult({ - result: false, - expected: specifier, - actual: Object.keys(this.module.resolutions), - }); - } - } - }, - }; - } - } hooks.before(async () => { app = await scenario.prepare(); - result = await Audit.run({ app: app.dir, 'reuse-build': false }); }); hooks.beforeEach(assert => { expectFile = expectFilesAt(readFileSync(join(app.dir, 'dist/.stage2-output'), 'utf8'), { qunit: assert }); - expectAudit = new ExpectAuditResults(result, assert); build = new Transpiler(expectFile.basePath); }); - test('whole package renaming works for top-level module', function (assert) { + let expectAudit = setupAuditTest(hooks, () => app.dir); + + test('whole package renaming works for top-level module', function () { expectAudit .module('./components/import-lodash.js') .resolves('lodash') .to('./node_modules/ember-lodash/index.js'); - assert.equal( - result.modules['./components/import-lodash.js']?.resolutions['lodash'], - './node_modules/ember-lodash/index.js' - ); expectFile('node_modules/ember-lodash/index.js').matches(/lodash index/); }); diff --git a/tests/scenarios/compat-stage2-test.ts b/tests/scenarios/compat-stage2-test.ts index 103eb2866..837a55680 100644 --- a/tests/scenarios/compat-stage2-test.ts +++ b/tests/scenarios/compat-stage2-test.ts @@ -9,6 +9,8 @@ import { ExpectFile, expectFilesAt, Rebuilder, Transpiler } from '@embroider/tes import { throwOnWarnings } from '@embroider/core'; import merge from 'lodash/merge'; import QUnit from 'qunit'; +import { setupAuditTest } from '@embroider/test-support/audit-assertions'; + const { module: Qmodule, test } = QUnit; let stage2Scenarios = appScenarios.map('compat-stage2-build', app => { @@ -287,6 +289,7 @@ stage2Scenarios + `, 'curly.hbs': ` {{hello-world useDynamic="first-choice" }} @@ -441,6 +444,8 @@ stage2Scenarios build = new Transpiler(expectFile.basePath); }); + let expectAudit = setupAuditTest(hooks, () => app.dir); + test('index.hbs', function () { let assertFile = expectFile('templates/index.hbs').transform(build.transpile); assertFile.matches(/import \w+ from ["']..\/components\/hello-world\.js["']/, 'explicit dependency'); @@ -492,18 +497,18 @@ stage2Scenarios assertFile.matches( /window\.define\(["']\my-addon\/synthetic-import-1["'],\s*function\s\(\)\s*\{\s*return\s+esc\(require\(["']\.\.\/node_modules\/my-addon\/synthetic-import-1/ ); - assertFile.matches( - /export \{ default \} from ['"]\.\.\/node_modules\/my-addon\/components\/hello-world['"]/, - 'remapped to precise copy of my-addon' - ); + + expectAudit + .module('./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 () { - let assertFile = expectFile('./templates/components/direct-template-reexport.js').transform(build.transpile); - assertFile.matches( - /export \{ default \} from ['"]\.\.\/\.\.\/node_modules\/my-addon\/templates\/components\/hello-world['"]/, - 'rewrites reexports of templates' - ); + expectAudit + .module('./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'); }); test('uses-inline-template.js', function () { @@ -520,8 +525,10 @@ stage2Scenarios }); test('app can import a deep addon', function () { - let assertFile = expectFile('use-deep-addon.js').transform(build.transpile); - assertFile.matches(/import thing from ["']\.\/node_modules\/my-addon\/node_modules\/deep-addon['"]/); + expectAudit + .module('./use-deep-addon.js') + .resolves('deep-addon') + .to('./node_modules/my-addon/node_modules/deep-addon/index.js'); }); test('amd require in an addon gets rewritten to window.require', function () { From 6f2ce7f53eb6ca7a146149bd8c05dd5a638a9b9d Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 23 Jan 2023 17:00:44 -0500 Subject: [PATCH 13/16] newer rollup-plugin-ts pulls in a dep that breaks on our oldest support node version --- packages/router/package.json | 2 +- tests/scenarios/package.json | 2 +- yarn.lock | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/router/package.json b/packages/router/package.json index 025f80ff8..ad31b2248 100644 --- a/packages/router/package.json +++ b/packages/router/package.json @@ -70,7 +70,7 @@ "prettier": "^2.5.1", "rollup": "^2.67.0", "rollup-plugin-copy": "^3.4.0", - "rollup-plugin-ts": "^3.0.2", + "rollup-plugin-ts": "~3.0.2", "typescript": "^4.7.4" }, "peerDependencies": { diff --git a/tests/scenarios/package.json b/tests/scenarios/package.json index d150c3b8f..e65fe04ff 100644 --- a/tests/scenarios/package.json +++ b/tests/scenarios/package.json @@ -66,7 +66,7 @@ "ember-source-latest": "npm:ember-source@latest", "ember-truth-helpers": "^3.0.0", "rollup": "^2.69.1", - "rollup-plugin-ts": "^3.0.0", + "rollup-plugin-ts": "~3.0.0", "typescript": "~4.4.2" }, "volta": { diff --git a/yarn.lock b/yarn.lock index 008e2b0c6..7870b4a67 100644 --- a/yarn.lock +++ b/yarn.lock @@ -14250,7 +14250,7 @@ rollup-plugin-delete@^2.0.0: dependencies: del "^5.1.0" -rollup-plugin-ts@^3.0.0, rollup-plugin-ts@^3.0.2: +rollup-plugin-ts@~3.0.0, rollup-plugin-ts@~3.0.2: version "3.0.2" resolved "https://registry.yarnpkg.com/rollup-plugin-ts/-/rollup-plugin-ts-3.0.2.tgz#ee1a3f9ffe202ceff0b4d2f725fa268fa0c921bf" integrity sha512-67qi2QTHewhLyKDG6fX3jpohWpmUPPIT/xJ7rsYK46X6MqmoWy64Ti0y8ygPfLv8mXDCdRZUofM3mTxDfCswRA== From db817ec059cac117eddaa6760b408b0f1e87ea0f Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 23 Jan 2023 17:21:39 -0500 Subject: [PATCH 14/16] fix audit tests --- packages/compat/tests/audit.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/compat/tests/audit.test.ts b/packages/compat/tests/audit.test.ts index 4e2496d6a..9d6c7717c 100644 --- a/packages/compat/tests/audit.test.ts +++ b/packages/compat/tests/audit.test.ts @@ -75,6 +75,7 @@ describe('audit', function () { 2 )}`, '_adjust_imports.json': JSON.stringify(resolver.adjustImportsOptions), + '_relocated_files.json': JSON.stringify({}), }); let appMeta: AppMeta = { type: 'app', From d3e2f5c3bf1cc5caa45a624a7f5741ddb4204c09 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 24 Jan 2023 10:06:53 -0500 Subject: [PATCH 15/16] rework auditor's resolver loop and improve audit-based tests --- packages/compat/src/audit.ts | 112 ++++++++++------------ test-packages/support/audit-assertions.ts | 7 ++ tests/scenarios/compat-renaming-test.ts | 27 ++++-- 3 files changed, 75 insertions(+), 71 deletions(-) diff --git a/packages/compat/src/audit.ts b/packages/compat/src/audit.ts index f7991e57d..b075fff59 100644 --- a/packages/compat/src/audit.ts +++ b/packages/compat/src/audit.ts @@ -1,7 +1,7 @@ import { readFileSync, readJSONSync } from 'fs-extra'; import { dirname, join, resolve as resolvePath } from 'path'; import resolveModule from 'resolve'; -import { AppMeta, explicitRelative, hbsToJS, Resolver, ResolverOptions } from '@embroider/core'; +import { AppMeta, explicitRelative, hbsToJS, Resolution, Resolver, ResolverOptions } from '@embroider/core'; import { Memoize } from 'typescript-memoize'; import chalk from 'chalk'; import jsdom from 'jsdom'; @@ -84,6 +84,11 @@ function isLinked(module: InternalModule | undefined): module is LinkedInternalM return Boolean(module?.parsed && module.resolved && module.linked); } +interface Request { + specifier: string; + fromFile: string; +} + export interface Import { source: string; specifiers: { @@ -306,7 +311,7 @@ export class Audit { let resolved = new Map() as NonNullable; let resolver = new Resolver(this.resolverParams); for (let dep of visitResult.dependencies) { - let depFilename = await this.resolve(dep, filename, resolver); + let depFilename = await this.resolve({ specifier: dep, fromFile: filename }, resolver); if (depFilename) { resolved.set(dep, depFilename); if (!isResolutionFailure(depFilename)) { @@ -531,80 +536,61 @@ export class Audit { return this.visitJS(filename, js); } - private async resolve( - specifier: string, - fromFile: string, - resolver: Resolver - ): Promise { - let resolution = resolver.beforeResolve(specifier, fromFile); + private nextRequest( + prevRequest: { specifier: string; fromFile: string }, + resolution: Resolution + ): { specifier: string; fromFile: string } | undefined { switch (resolution.result) { case 'virtual': // nothing to audit - return; + return undefined; case 'alias': // follow the redirect - specifier = resolution.specifier; - if (resolution.fromFile) { - fromFile = resolution.fromFile; - } - break; + let specifier = resolution.specifier; + let fromFile = resolution.fromFile ?? prevRequest.fromFile; + return { specifier, fromFile }; case 'rehome': - fromFile = resolution.fromFile; - break; + return { specifier: prevRequest.specifier, fromFile: resolution.fromFile }; case 'continue': - // nothing to do - break; + return prevRequest; default: throw assertNever(resolution); } + } - if (['@embroider/macros', '@ember/template-factory'].includes(specifier)) { - // the audit process deliberately removes the @embroider/macros babel - // plugins, so the imports are still present and should be left alone. - return; - } - try { - return resolveModule.sync(specifier, { - basedir: dirname(fromFile), - extensions: this.meta['resolvable-extensions'], - }); - } catch (err) { - if (err.code === 'MODULE_NOT_FOUND') { - resolution = resolver.fallbackResolve(specifier, fromFile); - switch (resolution.result) { - case 'virtual': - // nothing to audit - return; - case 'alias': - // follow the redirect - specifier = resolution.specifier; - if (resolution.fromFile) { - fromFile = resolution.fromFile; - } - break; - case 'rehome': - fromFile = resolution.fromFile; - break; - case 'continue': - // nothing to do - break; - default: - throw assertNever(resolution); + private async resolve(request: Request, resolver: Resolver): Promise { + let current: Request | undefined = request; + + while (true) { + current = this.nextRequest(current, resolver.beforeResolve(current.specifier, current.fromFile)); + if (!current) { + return; + } + + if (['@embroider/macros', '@ember/template-factory'].includes(current.specifier)) { + // the audit process deliberately removes the @embroider/macros babel + // plugins, so the imports are still present and should be left alone. + return; + } + try { + return resolveModule.sync(current.specifier, { + basedir: dirname(current.fromFile), + extensions: this.meta['resolvable-extensions'], + }); + } catch (err) { + if (err.code !== 'MODULE_NOT_FOUND') { + throw err; } - try { - return resolveModule.sync(specifier, { - basedir: dirname(fromFile), - extensions: this.meta['resolvable-extensions'], - }); - } catch (err) { - if (err.code === 'MODULE_NOT_FOUND') { - return { isResolutionFailure: true }; - } else { - throw err; - } + let retry = this.nextRequest(current, resolver.fallbackResolve(current.specifier, current.fromFile)); + if (!retry) { + // the request got virtualized + return; } - } else { - throw err; + if (retry === current) { + // the fallback has nothing new to offer, so this is a real resolution failure + return { isResolutionFailure: true }; + } + current = retry; } } } diff --git a/test-packages/support/audit-assertions.ts b/test-packages/support/audit-assertions.ts index aaf2fde14..f59b3654b 100644 --- a/test-packages/support/audit-assertions.ts +++ b/test-packages/support/audit-assertions.ts @@ -22,6 +22,9 @@ export function setupAuditTest(hooks: NestedHooks, getAppDir: () => string) { module(name: string) { return expectAudit.module(name); }, + get findings() { + return expectAudit.findings; + }, }; } @@ -39,6 +42,10 @@ class ExpectAuditResults { } return new ExpectModule(this.assert, m); } + + get findings() { + return this.result.findings; + } } class ExpectModule { diff --git a/tests/scenarios/compat-renaming-test.ts b/tests/scenarios/compat-renaming-test.ts index 109c8aadc..7e0baecdd 100644 --- a/tests/scenarios/compat-renaming-test.ts +++ b/tests/scenarios/compat-renaming-test.ts @@ -63,8 +63,8 @@ appScenarios } `, addon: { - 'index.js': `// lodash index`, - 'capitalize.js': `// lodash capitalize`, + 'index.js': `// lodash index\nexport default function() {}`, + 'capitalize.js': `// lodash capitalize\nexport default function() {}`, }, }); app.addDevDependency(emberLodash); @@ -83,15 +83,15 @@ appScenarios `, addon: { 'emits-multiple-packages': { - 'own-thing.js': '// own thing', + 'own-thing.js': '// own thing\nexport default function() {}', }, 'somebody-elses-package': { - 'environment.js': '// somebody elses environment', + 'environment.js': '// somebody elses environment\nexport default function() {}', utils: { - 'index.js': '// somebody elses utils', + 'index.js': '// somebody elses utils\nexport default function() {}', }, }, - 'single-file-package.js': '// single file package', + 'single-file-package.js': '// single file package\nexport default function() {}', }, }); app.addDependency(emitsMultiple); @@ -149,6 +149,13 @@ appScenarios }, }, }); + + app.addDevDependency('somebody-elses-package', { + files: { + 'index.js': `export default function() {}`, + 'deeper.js': `export default function() {}`, + }, + }); }) .forEachScenario(scenario => { Qmodule(scenario.name, function (hooks) { @@ -169,6 +176,10 @@ appScenarios let expectAudit = setupAuditTest(hooks, () => app.dir); + test('audit issues', function (assert) { + assert.deepEqual(expectAudit.findings, [], 'expected no problem findings in audit'); + }); + test('whole package renaming works for top-level module', function () { expectAudit .module('./components/import-lodash.js') @@ -239,12 +250,12 @@ appScenarios expectAudit .module('./components/import-somebody-elses-original.js') .resolves('somebody-elses-package') - .to(null); + .to('./node_modules/somebody-elses-package/index.js'); expectAudit .module('./components/import-somebody-elses-original.js') .resolves('somebody-elses-package/deeper') - .to(null); + .to('./node_modules/somebody-elses-package/deeper.js'); }); test('single file package gets captured and renamed', function () { expectAudit From f840a2374f3b0234bc3230e513a0a9253e062c51 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 26 Jan 2023 12:22:31 -0500 Subject: [PATCH 16/16] move audit build out of process because ember-cli traditionally leaks workers and doesn't let our tests exit cleanly --- packages/compat/src/audit/build.ts | 88 +++++++++++++++++++--------- packages/compat/src/audit/capture.ts | 28 --------- 2 files changed, 60 insertions(+), 56 deletions(-) delete mode 100644 packages/compat/src/audit/capture.ts diff --git a/packages/compat/src/audit/build.ts b/packages/compat/src/audit/build.ts index d509d320f..305c99c74 100644 --- a/packages/compat/src/audit/build.ts +++ b/packages/compat/src/audit/build.ts @@ -1,35 +1,19 @@ import chalk from 'chalk'; -import resolveModule from 'resolve'; import { AuditBuildOptions } from '../audit'; -import { CaptureStream } from './capture'; +import { spawn } from 'child_process'; export async function buildApp(options: AuditBuildOptions): Promise { - let { default: cli } = await import(resolveModule.sync('ember-cli', { basedir: options.app })); - process.env.STAGE2_ONLY = 'true'; - let capture = new CaptureStream(); - let orig = { cwd: process.cwd(), log: console.log, error: console.error, warn: console.warn }; - process.chdir(options.app); - // this is icky, but too many things in the build don't respect the - // `outputStream`, etc, options we pass below. - console.log = console.warn = console.error = capture.log; - try { - let result = await cli({ - cliArgs: ['build'], - outputStream: capture, - errorStream: capture, - }); - let exitCode = typeof result === 'object' ? result.exitCode : result; - // an undefined exit code means success, because of course it does. - if (exitCode != null && exitCode !== 0) { - throw new BuildError( - `${chalk.yellow('Unable to begin audit')} because the build failed. Build output follows:\n${capture.output}` - ); - } - } finally { - process.chdir(orig.cwd); - console.log = orig.log; - console.warn = orig.warn; - console.error = orig.error; + let result = await execute(`node node_modules/ember-cli/bin/ember build`, { + pwd: options.app, + env: { + STAGE2_ONLY: 'true', + }, + }); + + if (result.exitCode !== 0) { + throw new BuildError( + `${chalk.yellow('Unable to begin audit')} because the build failed. Build output follows:\n${result.output}` + ); } } @@ -43,3 +27,51 @@ export class BuildError extends Error { export function isBuildError(err: any): err is BuildError { return err?.isBuildError; } + +async function execute( + shellCommand: string, + opts?: { env?: Record; pwd?: string } +): Promise<{ + exitCode: number; + stderr: string; + stdout: string; + output: string; +}> { + let env: Record | undefined; + if (opts?.env) { + env = { ...process.env, ...opts.env }; + } + let child = spawn(shellCommand, { + stdio: ['inherit', 'pipe', 'pipe'], + cwd: opts?.pwd, + shell: true, + env, + }); + let stderrBuffer: string[] = []; + let stdoutBuffer: string[] = []; + let combinedBuffer: string[] = []; + child.stderr.on('data', data => { + stderrBuffer.push(data); + combinedBuffer.push(data); + }); + child.stdout.on('data', data => { + stdoutBuffer.push(data); + combinedBuffer.push(data); + }); + return new Promise(resolve => { + child.on('close', (exitCode: number) => { + resolve({ + exitCode, + get stdout() { + return stdoutBuffer.join(''); + }, + get stderr() { + return stderrBuffer.join(''); + }, + get output() { + return combinedBuffer.join(''); + }, + }); + }); + }); +} diff --git a/packages/compat/src/audit/capture.ts b/packages/compat/src/audit/capture.ts deleted file mode 100644 index 411463005..000000000 --- a/packages/compat/src/audit/capture.ts +++ /dev/null @@ -1,28 +0,0 @@ -import bind from 'bind-decorator'; -import { Writable } from 'stream'; -import { format } from 'util'; - -export class CaptureStream extends Writable { - private gather = [] as (string | Buffer)[]; - _write(chunk: string | Buffer, _encoding: string, callback: (err?: any) => void) { - this.gather.push(chunk); - callback(); - } - - @bind - log(msg: string, ...args: any[]) { - this.gather.push(format(msg, ...args)); - } - - get output(): string { - return this.gather - .map(element => { - if (typeof element === 'string') { - return element; - } else { - return element.toString('utf8'); - } - }) - .join(''); - } -}