From c0e49e9d0f7afb95762b53d21a385d0b934c7361 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Fri, 22 Dec 2023 21:07:41 -0500 Subject: [PATCH] Fix dynamic import inside allowAppImports dirs The `ember-auto-import/babel-plugin` is necessary for apps that want to add dynamic import support to their classically built code. But inside a file controlled by allowAppImports, webpack is doing the buildng and there is already dynamic import support, so using the plugin there is actually an error. --- packages/ember-auto-import/ts/package.ts | 40 ++++++++++++++++++++++-- test-scenarios/dynamic-import-test.ts | 31 ++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/packages/ember-auto-import/ts/package.ts b/packages/ember-auto-import/ts/package.ts index f4c9f51a..0ce0fe3f 100644 --- a/packages/ember-auto-import/ts/package.ts +++ b/packages/ember-auto-import/ts/package.ts @@ -10,7 +10,7 @@ import { packageName as getPackageName, } from '@embroider/shared-internals'; import semver from 'semver'; -import type { TransformOptions } from '@babel/core'; +import type { PluginItem, TransformOptions } from '@babel/core'; import { MacrosConfig } from '@embroider/macros/src/node'; import minimatch from 'minimatch'; import { stripQuery } from './util'; @@ -141,6 +141,19 @@ export default class Package { this._hasBabelDetails = true; } + // this is used for two things: + // - when interoperating with older versions of ember-auto-import, it's used + // to configure the parser that we use to analyze source code. The parser + // cares about the user's babel config so it will support all the same + // syntax. (Newer EAI versions don't need to do this because they use the + // faster analyzer that happens inside the existing babel parse.) + // - when transpiling parts of the app itself that are configured with + // allowAppImports. It would be surprising if these didn't get transpiled + // with the same babel config that the rest of the app is getting. There is, + // however, one exception: if the user has added + // ember-auto-import/babel-plugin to get dynamic import support, we need to + // remove that because inside the natively webpack-owned area it's not + // needed and would actually break dynamic imports. get babelOptions(): TransformOptions { this._ensureBabelDetails(); return this._babelOptions; @@ -187,7 +200,11 @@ export default class Package { if (babelOptions.plugins) { babelOptions.plugins = babelOptions.plugins.filter( - (p: any) => !p._parallelBabel + // removing the weird "_parallelBabel" entry that's only used by + // broccoli-babel-transpiler and removing our own dynamic import babel + // plugin if it was added (because it's only correct to use it against + // the classic ember build, not the webpack-owned parts of the build. + (p: any) => !p._parallelBabel && !isEAIBabelPlugin(p) ); } @@ -674,3 +691,22 @@ function ensureTrailingSlash(url: string): string { } return url; } + +function isEAIBabelPlugin(item: PluginItem) { + let pluginPath: string | undefined; + if (typeof item === 'string') { + pluginPath = item; + } else if ( + Array.isArray(item) && + item.length > 0 && + typeof item[0] === 'string' + ) { + pluginPath = item[0]; + } + + if (pluginPath) { + return /ember-auto-import[\\/]babel-plugin/.test(pluginPath); + } + + return (item as any).baseDir?.() === __dirname; +} diff --git a/test-scenarios/dynamic-import-test.ts b/test-scenarios/dynamic-import-test.ts index e418796b..4659f922 100644 --- a/test-scenarios/dynamic-import-test.ts +++ b/test-scenarios/dynamic-import-test.ts @@ -61,6 +61,19 @@ appScenarios 'export default function() { return "example3 worked" }; export const NO_FIND = "string or not to string"', 'example4.js': 'export default function() { return "example4 worked" }; export const NO_FIND = "please look away"', + 'does-dynamic-import.js': ` + export async function getRelativeTarget() { + return (await import('./dynamic-target.js')).default; + } + export async function getPackageTarget() { + return (await import('dynamic-target')).default; + } + `, + 'dynamic-target.js': ` + export default function() { + return 'relative dynamic target loaded OK'; + } + `, }, templates: { 'dynamic-import.hbs': `
{{this.model.result}}
`, @@ -162,6 +175,7 @@ appScenarios 'allow-app-imports-test.js': ` import { module, test } from 'qunit'; import checkScripts from '../helpers/check-scripts'; + import { getRelativeTarget, getPackageTarget } from '@ef4/app-template/lib/does-dynamic-import'; module('Unit | allow-app-import', function () { test("check scripts smoke test", async function(assert) { @@ -206,6 +220,14 @@ appScenarios "expect to not find the 'example4 NO_FIND' in app-js because it's being consumed by webpack" ); }) + test("dynamic relative import from inside the allowAppImports dir", async function (assert) { + let fn = await getRelativeTarget(); + assert.equal(fn(), 'relative dynamic target loaded OK'); + }) + test("dynamic package import from inside the allowAppImports dir", async function (assert) { + let fn = await getPackageTarget(); + assert.equal(fn(), 'package dynamic target loaded OK'); + }) }); `, }, @@ -223,6 +245,15 @@ appScenarios }, }, }); + project.addDevDependency('dynamic-target', { + files: { + 'index.js': ` + export default function() { + return 'package dynamic target loaded OK'; + } + `, + }, + }); }) .forEachScenario(scenario => { Qmodule(scenario.name, function (hooks) {