Merge pull request #606 from embroider-build/allow-app-imports-dynamic

Fix dynamic import inside allowAppImports dirs

Author: ef4

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;
+}

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': `<div data-test="dynamic-import-result">{{this.model.result}}</div>`,
@@ -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) {