Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dynamic import inside allowAppImports dirs #606

Merged
merged 1 commit into from
Dec 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions packages/ember-auto-import/ts/package.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
);
}

Expand Down Expand Up @@ -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;
}
31 changes: 31 additions & 0 deletions test-scenarios/dynamic-import-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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': `<div data-test="dynamic-import-result">{{this.model.result}}</div>`,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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');
})
});
`,
},
Expand All @@ -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) {
Expand Down
Loading