fix(commonjs)!: dynamic require root check was broken in some cases (#1461)

* fix(commonjs): dynamic require root check was broken in some cases

* fix(commonjs): fixed a commonjs unit test based on OS paths

* test(commonjs): attempt 2 at having tests not fail in CI

Using the normalized path in _all_ the properties of the error

* test: 1st try at the test

* test(commonjs): the dynamic root must ignore conflicting backslashes

* test(commonjs): added a test specifically for Windows

Author: Elarcis

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,18 +137,19 @@ export default function commonjs(options = {}) {
     }
 
     const needsRequireWrapper =
-      !isEsModule &&
-      (dynamicRequireModules.has(normalizePathSlashes(id)) || strictRequiresFilter(id));
+      !isEsModule && (dynamicRequireModules.has(normalizedId) || strictRequiresFilter(id));
 
     const checkDynamicRequire = (position) => {
-      if (id.indexOf(dynamicRequireRoot) !== 0) {
+      const normalizedRequireRoot = normalizePathSlashes(dynamicRequireRoot);
+
+      if (normalizedId.indexOf(normalizedRequireRoot) !== 0) {
         this.error(
           {
             code: 'DYNAMIC_REQUIRE_OUTSIDE_ROOT',
-            id,
+            normalizedId,
             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

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

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));
@@ -715,9 +720,16 @@ 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 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')
+  );
+
   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',
@@ -726,6 +738,52 @@ test('throws when there is a dynamic require from outside dynamicRequireRoot', a
   });
 });
 
+test('does not throw when a dynamic require uses different slashes than dynamicRequireRoot', 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);
+});
+
+// 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',