From 3a7c55fe8079eedc1268a5f2f0be131c53dd7172 Mon Sep 17 00:00:00 2001 From: Elarcis Date: Tue, 21 Mar 2023 17:41:30 +0100 Subject: [PATCH 1/6] fix(commonjs): dynamic require root check was broken in some cases --- packages/commonjs/src/index.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js index a3288c427..d45a0b74f 100644 --- a/packages/commonjs/src/index.js +++ b/packages/commonjs/src/index.js @@ -140,14 +140,17 @@ export default function commonjs(options = {}) { (dynamicRequireModules.has(normalizePathSlashes(id)) || strictRequiresFilter(id)); const checkDynamicRequire = (position) => { - if (id.indexOf(dynamicRequireRoot) !== 0) { + const normalizedId = normalizePathSlashes(id); + const normalizedRequireRoot = normalizePathSlashes(dynamicRequireRoot); + + if (normalizedId.indexOf(normalizedRequireRoot) !== 0) { this.error( { code: 'DYNAMIC_REQUIRE_OUTSIDE_ROOT', id, dynamicRequireRoot, - message: `"${id}" contains dynamic require statements but it is not within the current dynamicRequireRoot "${dynamicRequireRoot}". You should set dynamicRequireRoot to "${dirname( - id + message: `"${normalizedId}" contains dynamic require statements but it is not within the current dynamicRequireRoot "${normalizedRequireRoot}". You should set dynamicRequireRoot to "${dirname( + normalizedId )}" or one of its parent directories.` }, position From 1461d815dea1ee512a11f63e4a98714665144ee3 Mon Sep 17 00:00:00 2001 From: Elarcis Date: Mon, 27 Mar 2023 10:46:35 +0200 Subject: [PATCH 2/6] fix(commonjs): fixed a commonjs unit test based on OS paths --- packages/commonjs/test/helpers/util.js | 5 +++++ packages/commonjs/test/test.js | 16 +++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/commonjs/test/helpers/util.js b/packages/commonjs/test/helpers/util.js index 060233d71..7ab558988 100644 --- a/packages/commonjs/test/helpers/util.js +++ b/packages/commonjs/test/helpers/util.js @@ -7,6 +7,10 @@ function commonjs(options) { return commonjsPlugin(options); } +function normalizePathSlashes(path) { + return path.replace(/\\/g, '/'); +} + function requireWithContext(code, context) { const module = { exports: {} }; const contextWithExports = { ...context, module, exports: module.exports }; @@ -90,5 +94,6 @@ module.exports = { executeBundle, getCodeFromBundle, getCodeMapFromBundle, + normalizePathSlashes, runCodeSplitTest }; diff --git a/packages/commonjs/test/test.js b/packages/commonjs/test/test.js index 44234545b..a4585cf61 100644 --- a/packages/commonjs/test/test.js +++ b/packages/commonjs/test/test.js @@ -13,7 +13,12 @@ const { testBundle } = require('../../../util/test'); const { peerDependencies } = require('../package.json'); -const { commonjs, executeBundle, getCodeFromBundle } = require('./helpers/util.js'); +const { + commonjs, + executeBundle, + getCodeFromBundle, + normalizePathSlashes +} = require('./helpers/util.js'); install(); test.beforeEach(() => process.chdir(__dirname)); @@ -716,8 +721,13 @@ test('throws when there is a dynamic require from outside dynamicRequireRoot', a const cwd = process.cwd(); const id = path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/main.js'); - const dynamicRequireRoot = path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/nested'); - const minimalDynamicRequireRoot = path.join(cwd, 'fixtures/samples/dynamic-require-outside-root'); + const dynamicRequireRoot = normalizePathSlashes( + path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/nested') + ); + const minimalDynamicRequireRoot = normalizePathSlashes( + path.join(cwd, 'fixtures/samples/dynamic-require-outside-root') + ); + t.like(error, { message: `"${id}" contains dynamic require statements but it is not within the current dynamicRequireRoot "${dynamicRequireRoot}". You should set dynamicRequireRoot to "${minimalDynamicRequireRoot}" or one of its parent directories.`, pluginCode: 'DYNAMIC_REQUIRE_OUTSIDE_ROOT', From 1e22af10a70fdabdc2e4b9e2a5165b33dc494b8d Mon Sep 17 00:00:00 2001 From: Elarcis Date: Thu, 6 Apr 2023 12:50:25 +0200 Subject: [PATCH 3/6] test(commonjs): attempt 2 at having tests not fail in CI Using the normalized path in _all_ the properties of the error --- packages/commonjs/src/index.js | 2 +- packages/commonjs/test/test.js | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js index d45a0b74f..c898199af 100644 --- a/packages/commonjs/src/index.js +++ b/packages/commonjs/src/index.js @@ -147,7 +147,7 @@ export default function commonjs(options = {}) { this.error( { code: 'DYNAMIC_REQUIRE_OUTSIDE_ROOT', - id, + normalizedId, dynamicRequireRoot, message: `"${normalizedId}" contains dynamic require statements but it is not within the current dynamicRequireRoot "${normalizedRequireRoot}". You should set dynamicRequireRoot to "${dirname( normalizedId diff --git a/packages/commonjs/test/test.js b/packages/commonjs/test/test.js index a4585cf61..10526f71c 100644 --- a/packages/commonjs/test/test.js +++ b/packages/commonjs/test/test.js @@ -720,7 +720,9 @@ test('throws when there is a dynamic require from outside dynamicRequireRoot', a } const cwd = process.cwd(); - const id = path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/main.js'); + const id = normalizePathSlashes( + path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/main.js') + ); const dynamicRequireRoot = normalizePathSlashes( path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/nested') ); From 7be87e6935c6b7ec38c9d9ac987727886dfaa149 Mon Sep 17 00:00:00 2001 From: Elarcis Date: Thu, 6 Apr 2023 13:23:43 +0200 Subject: [PATCH 4/6] test: 1st try at the test --- packages/commonjs/test/test.js | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/packages/commonjs/test/test.js b/packages/commonjs/test/test.js index 10526f71c..77d785e3c 100644 --- a/packages/commonjs/test/test.js +++ b/packages/commonjs/test/test.js @@ -738,6 +738,40 @@ test('throws when there is a dynamic require from outside dynamicRequireRoot', a }); }); +test('does not mistakely says that a dynamic require is outside of dynamicRequireRoot when `path` uses backslashes', async (t) => { + const plugin = commonjs({ + dynamicRequireRoot: 'fixtures/samples/dynamic-require-outside-root/nested', + dynamicRequireTargets: ['fixtures/samples/dynamic-require-outside-root/nested/target.js'] + }); + + let error = null; + const cwd = process.cwd(); + const id = normalizePathSlashes( + path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/main.js') + ); + const dynamicRequireRoot = normalizePathSlashes( + path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/nested') + ); + const minimalDynamicRequireRoot = normalizePathSlashes( + path.join(cwd, 'fixtures/samples/dynamic-require-outside-root') + ); + + const code = (await import('fixtures/samples/dynamic-require-outside-root/main.js')).default; + + try { + plugin.transform(code, id.replace('/', '\\')); + } catch (e) { + error = e; + } + + t.like(error, { + message: `"${id}" contains dynamic require statements but it is not within the current dynamicRequireRoot "${dynamicRequireRoot}". You should set dynamicRequireRoot to "${minimalDynamicRequireRoot}" or one of its parent directories.`, + pluginCode: 'DYNAMIC_REQUIRE_OUTSIDE_ROOT', + id, + dynamicRequireRoot + }); +}); + test('does not transform typeof exports for mixed modules', async (t) => { const bundle = await rollup({ input: 'fixtures/samples/mixed-module-typeof-exports/main.js', From 926d381a60b236c76ee62b1b0325f5c1b116a29f Mon Sep 17 00:00:00 2001 From: Elarcis Date: Thu, 6 Apr 2023 14:26:17 +0200 Subject: [PATCH 5/6] test(commonjs): the dynamic root must ignore conflicting backslashes --- packages/commonjs/src/index.js | 7 +++--- packages/commonjs/test/test.js | 43 ++++++++++++---------------------- 2 files changed, 18 insertions(+), 32 deletions(-) diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js index c898199af..f2590befe 100644 --- a/packages/commonjs/src/index.js +++ b/packages/commonjs/src/index.js @@ -112,6 +112,7 @@ export default function commonjs(options = {}) { let requireResolver; function transformAndCheckExports(code, id) { + const normalizedId = normalizePathSlashes(id); const { isEsModule, hasDefaultExport, hasNamedExports, ast } = analyzeTopLevelStatements( this.parse, code, @@ -127,7 +128,7 @@ export default function commonjs(options = {}) { } if ( - !dynamicRequireModules.has(normalizePathSlashes(id)) && + !dynamicRequireModules.has(normalizedId) && (!(hasCjsKeywords(code, ignoreGlobal) || requireResolver.isRequiredId(id)) || (isEsModule && !options.transformMixedEsModules)) ) { @@ -136,11 +137,9 @@ export default function commonjs(options = {}) { } const needsRequireWrapper = - !isEsModule && - (dynamicRequireModules.has(normalizePathSlashes(id)) || strictRequiresFilter(id)); + !isEsModule && (dynamicRequireModules.has(normalizedId) || strictRequiresFilter(id)); const checkDynamicRequire = (position) => { - const normalizedId = normalizePathSlashes(id); const normalizedRequireRoot = normalizePathSlashes(dynamicRequireRoot); if (normalizedId.indexOf(normalizedRequireRoot) !== 0) { diff --git a/packages/commonjs/test/test.js b/packages/commonjs/test/test.js index 77d785e3c..4558d24f9 100644 --- a/packages/commonjs/test/test.js +++ b/packages/commonjs/test/test.js @@ -738,38 +738,25 @@ test('throws when there is a dynamic require from outside dynamicRequireRoot', a }); }); -test('does not mistakely says that a dynamic require is outside of dynamicRequireRoot when `path` uses backslashes', async (t) => { - const plugin = commonjs({ - dynamicRequireRoot: 'fixtures/samples/dynamic-require-outside-root/nested', - dynamicRequireTargets: ['fixtures/samples/dynamic-require-outside-root/nested/target.js'] - }); - +test('does not throw when a dynamic require uses different slashes than dynamicRequireRoot', async (t) => { let error = null; - const cwd = process.cwd(); - const id = normalizePathSlashes( - path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/main.js') - ); - const dynamicRequireRoot = normalizePathSlashes( - path.join(cwd, 'fixtures/samples/dynamic-require-outside-root/nested') - ); - const minimalDynamicRequireRoot = normalizePathSlashes( - path.join(cwd, 'fixtures/samples/dynamic-require-outside-root') - ); - - const code = (await import('fixtures/samples/dynamic-require-outside-root/main.js')).default; - try { - plugin.transform(code, id.replace('/', '\\')); - } catch (e) { - error = e; + await rollup({ + input: 'fixtures/samples/dynamic-require-outside-root/main.js', + plugins: [ + commonjs({ + dynamicRequireRoot: 'fixtures\\samples\\dynamic-require-outside-root', + dynamicRequireTargets: [ + 'fixtures\\samples\\dynamic-require-outside-root\\nested\\target.js' + ] + }) + ] + }); + } catch (err) { + error = err; } - t.like(error, { - message: `"${id}" contains dynamic require statements but it is not within the current dynamicRequireRoot "${dynamicRequireRoot}". You should set dynamicRequireRoot to "${minimalDynamicRequireRoot}" or one of its parent directories.`, - pluginCode: 'DYNAMIC_REQUIRE_OUTSIDE_ROOT', - id, - dynamicRequireRoot - }); + t.is(error, null); }); test('does not transform typeof exports for mixed modules', async (t) => { From f30b5cb73777f4d19a6e9262d3d3b6cee0aa8dc6 Mon Sep 17 00:00:00 2001 From: Elarcis Date: Thu, 6 Apr 2023 14:53:10 +0200 Subject: [PATCH 6/6] test(commonjs): added a test specifically for Windows --- packages/commonjs/test/test.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/packages/commonjs/test/test.js b/packages/commonjs/test/test.js index 4558d24f9..948151c0e 100644 --- a/packages/commonjs/test/test.js +++ b/packages/commonjs/test/test.js @@ -759,6 +759,31 @@ test('does not throw when a dynamic require uses different slashes than dynamicR t.is(error, null); }); +// On Windows, avoid a false error about a module not being in the dynamic require root due to +// incoherent slashes/backslashes in the paths. +if (os.platform() === 'win32') { + test('correctly asserts dynamicRequireRoot on Windows', async (t) => { + let error = null; + try { + await rollup({ + input: 'fixtures/samples/dynamic-require-outside-root/main.js', + plugins: [ + commonjs({ + dynamicRequireRoot: 'fixtures/samples/dynamic-require-outside-root', + dynamicRequireTargets: [ + 'fixtures/samples/dynamic-require-outside-root/nested/target.js' + ] + }) + ] + }); + } catch (err) { + error = err; + } + + t.is(error, null); + }); +} + test('does not transform typeof exports for mixed modules', async (t) => { const bundle = await rollup({ input: 'fixtures/samples/mixed-module-typeof-exports/main.js',