From ce6d1ce74527981b97264afa32884e7345f482fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Mon, 27 Sep 2021 12:20:04 +0100 Subject: [PATCH] fix: limit `nodeModulesWithDynamicImports` to unresolved imports (#671) * feat: limit nodeModulesWithDynamicImports to unresolved imports * chore: update test Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --- src/runtimes/node/dynamic_imports/plugin.js | 47 +++++++++---------- .../node-module-dynamic-import/function.js | 6 ++- .../node_modules/@org/test_2/files/one.json | 1 + .../node_modules/@org/test_2/index.js | 5 ++ .../node_modules/@org/test_2/package.json | 7 +++ .../node_modules/test-three/index.js | 5 ++ .../node_modules/test-three/one.json | 1 + .../node_modules/test-three/package.json | 4 ++ .../node-module-dynamic-import/package.json | 4 +- tests/main.js | 10 ++-- 10 files changed, 59 insertions(+), 31 deletions(-) create mode 100644 tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/files/one.json create mode 100644 tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/index.js create mode 100644 tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/package.json create mode 100644 tests/fixtures/node-module-dynamic-import/node_modules/test-three/index.js create mode 100644 tests/fixtures/node-module-dynamic-import/node_modules/test-three/one.json create mode 100644 tests/fixtures/node-module-dynamic-import/node_modules/test-three/package.json diff --git a/src/runtimes/node/dynamic_imports/plugin.js b/src/runtimes/node/dynamic_imports/plugin.js index ceff3fe8e..d16bd1b89 100644 --- a/src/runtimes/node/dynamic_imports/plugin.js +++ b/src/runtimes/node/dynamic_imports/plugin.js @@ -22,36 +22,35 @@ const getDynamicImportsPlugin = ({ basePath, includedPaths, moduleNames, process build.onDynamicImport({}, async (args) => { const { expression, resolveDir } = args - await registerModuleWithDynamicImports({ cache, moduleNames, resolveDir, srcDir }) - // Don't attempt to parse the expression if the base path isn't defined, // since we won't be able to generate the globs for the included paths. // Also don't parse the expression if we're not interested in processing // the dynamic import expressions. - if (!basePath || !processImports) { - return - } - - const { includedPathsGlob, type: expressionType } = parseExpression({ basePath, expression, resolveDir }) || {} - - if (!includedPathsGlob) { - return + if (basePath && processImports) { + const { includedPathsGlob, type: expressionType } = parseExpression({ basePath, expression, resolveDir }) || {} + + if (includedPathsGlob) { + // The parser has found a glob of paths that should be included in the + // bundle to make this import work, so we add it to `includedPaths`. + includedPaths.add(includedPathsGlob) + + // Create the shim that will handle the import at runtime. + const contents = getShimContents({ expressionType, resolveDir, srcDir }) + + // This is the only branch where we actually solve a dynamic import. + // eslint-disable-next-line max-depth + if (contents) { + return { + contents, + } + } + } } - // The parser has found a glob of paths that should be included in the - // bundle to make this import work, so we add it to `includedPaths`. - includedPaths.add(includedPathsGlob) - - // Create the shim that will handle the import at runtime. - const contents = getShimContents({ expressionType, resolveDir, srcDir }) - - if (!contents) { - return - } - - return { - contents, - } + // If we're here, it means we weren't able to solve the dynamic import. + // We add it to the list of modules with dynamic imports, which allows + // consumers like Netlify Build or CLI to advise users on how to proceed. + await registerModuleWithDynamicImports({ cache, moduleNames, resolveDir, srcDir }) }) }, }) diff --git a/tests/fixtures/node-module-dynamic-import/function.js b/tests/fixtures/node-module-dynamic-import/function.js index 41fa36930..42d9392a9 100644 --- a/tests/fixtures/node-module-dynamic-import/function.js +++ b/tests/fixtures/node-module-dynamic-import/function.js @@ -1 +1,5 @@ -module.exports = require('@org/test') +const test1 = require('@org/test') +const test2 = require('@org/test_2') +const test3 = require('test-three') + +module.exports = [test1, test2, test3] diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/files/one.json b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/files/one.json new file mode 100644 index 000000000..34204b5fc --- /dev/null +++ b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/files/one.json @@ -0,0 +1 @@ +{"bool": true} diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/index.js b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/index.js new file mode 100644 index 000000000..2907af3a7 --- /dev/null +++ b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/index.js @@ -0,0 +1,5 @@ +module.exports = (number) => { + const file = require(`./files/${number}.json`) + + return file.bool +} diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/package.json b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/package.json new file mode 100644 index 000000000..f29a23da0 --- /dev/null +++ b/tests/fixtures/node-module-dynamic-import/node_modules/@org/test_2/package.json @@ -0,0 +1,7 @@ +{ + "name": "@org/test_2", + "version": "1.0.0", + "dependencies": { + "test-two": "1.0.0" + } +} diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/test-three/index.js b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/index.js new file mode 100644 index 000000000..812557c5c --- /dev/null +++ b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/index.js @@ -0,0 +1,5 @@ +module.exports = () => { + const file = "./one.json" + + return require(file) +} diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/test-three/one.json b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/one.json new file mode 100644 index 000000000..34204b5fc --- /dev/null +++ b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/one.json @@ -0,0 +1 @@ +{"bool": true} diff --git a/tests/fixtures/node-module-dynamic-import/node_modules/test-three/package.json b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/package.json new file mode 100644 index 000000000..788fc2af4 --- /dev/null +++ b/tests/fixtures/node-module-dynamic-import/node_modules/test-three/package.json @@ -0,0 +1,4 @@ +{ + "name": "test-three", + "version": "1.0.0" +} diff --git a/tests/fixtures/node-module-dynamic-import/package.json b/tests/fixtures/node-module-dynamic-import/package.json index c0b4a81d3..600eeda12 100644 --- a/tests/fixtures/node-module-dynamic-import/package.json +++ b/tests/fixtures/node-module-dynamic-import/package.json @@ -2,6 +2,8 @@ "name": "node-module-dynamic-import", "version": "1.0.0", "dependencies": { - "@org/test": "1.0.0" + "@org/test": "1.0.0", + "@org/test_2": "1.0.0", + "test-three": "1.0.0" } } diff --git a/tests/main.js b/tests/main.js index 2bae8a025..7725a083c 100644 --- a/tests/main.js +++ b/tests/main.js @@ -1455,13 +1455,14 @@ test('Adds `type: "functionsBundling"` to esbuild bundling errors', async (t) => }) test('Returns a list of all modules with dynamic imports in a `nodeModulesWithDynamicImports` property', async (t) => { - const { files } = await zipNode(t, 'node-module-dynamic-import', { - opts: { config: { '*': { nodeBundler: ESBUILD } } }, + const fixtureName = 'node-module-dynamic-import' + const { files } = await zipNode(t, fixtureName, { + opts: { basePath: join(FIXTURES_DIR, fixtureName), config: { '*': { nodeBundler: ESBUILD } } }, }) t.is(files[0].nodeModulesWithDynamicImports.length, 2) - t.true(files[0].nodeModulesWithDynamicImports.includes('@org/test')) t.true(files[0].nodeModulesWithDynamicImports.includes('test-two')) + t.true(files[0].nodeModulesWithDynamicImports.includes('test-three')) }) test('Returns an empty list of modules with dynamic imports if the modules are missing a `package.json`', async (t) => { @@ -1508,8 +1509,7 @@ test('Adds a runtime shim and includes the files needed for dynamic imports usin // eslint-disable-next-line unicorn/new-for-builtins t.deepEqual(values, Array(expectedLength).fill(true)) t.throws(() => func('two')) - t.is(files[0].nodeModulesWithDynamicImports.length, 1) - t.true(files[0].nodeModulesWithDynamicImports.includes('@org/test')) + t.is(files[0].nodeModulesWithDynamicImports.length, 0) }) test('Leaves dynamic imports untouched when the files required to resolve the expression cannot be packaged at build time', async (t) => {