fix(commonjs): Always sort node-resolve plugin after commonjs if necessary

Author: lukastaegert

packages/commonjs/README.md

@@ -42,6 +42,8 @@ export default {
 
 Then call `rollup` either via the [CLI](https://www.rollupjs.org/guide/en/#command-line-reference) or the [API](https://www.rollupjs.org/guide/en/#javascript-api).
 
+When used together with the node-resolve plugin
+
 ## Options
 
 ### `strictRequires`
@@ -378,10 +380,12 @@ export default {
     format: 'iife',
     name: 'MyModule'
   },
-  plugins: [resolve(), commonjs()]
+  plugins: [commonjs(), resolve()]
 };
 ```
 
+Note that this plugin needs to be placed _before_ the node-resolve plugin. If that is not the case, it will automatically fix this by adjusting the plugins array and moving the node-resolve plugin directly behind the commonjs plugin during startup.
+
 ## Usage with symlinks
 
 Symlinks are common in monorepos and are also created by the `npm link` command. Rollup with `@rollup/plugin-node-resolve` resolves modules to their real paths by default. So `include` and `exclude` paths should handle real paths rather than symlinked paths (e.g. `../common/node_modules/**` instead of `node_modules/**`). You may also use a regular expression for `include` that works regardless of base path. Try this:

packages/commonjs/src/dynamic-modules.js

@@ -1,10 +1,63 @@
+import { existsSync, readFileSync, statSync } from 'fs';
+import { join, resolve } from 'path';
+
+import glob from 'glob';
+
 import { getVirtualPathForDynamicRequirePath, normalizePathSlashes } from './utils';
 
+function getPackageEntryPoint(dirPath) {
+  let entryPoint = 'index.js';
+
+  try {
+    if (existsSync(join(dirPath, 'package.json'))) {
+      entryPoint =
+        JSON.parse(readFileSync(join(dirPath, 'package.json'), { encoding: 'utf8' })).main ||
+        entryPoint;
+    }
+  } catch (ignored) {
+    // ignored
+  }
+
+  return entryPoint;
+}
+
+function isDirectory(path) {
+  try {
+    if (statSync(path).isDirectory()) return true;
+  } catch (ignored) {
+    // Nothing to do here
+  }
+  return false;
+}
+
+export function getDynamicRequireModules(patterns) {
+  const dynamicRequireModules = new Map();
+  for (const pattern of !patterns || Array.isArray(patterns) ? patterns || [] : [patterns]) {
+    const isNegated = pattern.startsWith('!');
+    const modifyMap = (targetPath, resolvedPath) =>
+      isNegated
+        ? dynamicRequireModules.delete(targetPath)
+        : dynamicRequireModules.set(targetPath, resolvedPath);
+    for (const path of glob.sync(isNegated ? pattern.substr(1) : pattern)) {
+      const resolvedPath = resolve(path);
+      const requirePath = normalizePathSlashes(resolvedPath);
+      if (isDirectory(resolvedPath)) {
+        const modulePath = normalizePathSlashes(resolve(join(path, getPackageEntryPoint(path))));
+        modifyMap(requirePath, modulePath);
+        modifyMap(modulePath, modulePath);
+      } else {
+        modifyMap(requirePath, requirePath);
+      }
+    }
+  }
+  return dynamicRequireModules;
+}
+
 const FAILED_REQUIRE_ERROR = `throw new Error('Could not dynamically require "' + path + '". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.');`;
 
-export function getDynamicRequireModules(
+export function getDynamicModuleRegistry(
   isDynamicRequireModulesEnabled,
-  dynamicRequireModuleSet,
+  dynamicRequireModules,
   commonDir,
   ignoreDynamicRequires
 ) {
@@ -13,16 +66,15 @@ export function getDynamicRequireModules(
 	${FAILED_REQUIRE_ERROR}
 }`;
   }
-  const dynamicModuleIds = [...dynamicRequireModuleSet];
-  const dynamicModuleImports = dynamicModuleIds
+  const dynamicModuleImports = [...dynamicRequireModules.values()]
     .map(
       (id, index) =>
         `import ${
           id.endsWith('.json') ? `json${index}` : `{ __require as require${index} }`
         } from ${JSON.stringify(id)};`
     )
     .join('\n');
-  const dynamicModuleProps = dynamicModuleIds
+  const dynamicModuleProps = [...dynamicRequireModules.keys()]
     .map(
       (id, index) =>
         `\t\t${JSON.stringify(

packages/commonjs/src/dynamic-require-paths.js

@@ -68,11 +68,11 @@ export function getRequireStringArg(node) {
     : node.arguments[0].quasis[0].value.cooked;
 }
 
-export function hasDynamicModuleForPath(source, id, dynamicRequireModuleSet) {
+export function hasDynamicModuleForPath(source, id, dynamicRequireModules) {
   if (!/^(?:\.{0,2}[/\\]|[A-Za-z]:[/\\])/.test(source)) {
     try {
       const resolvedPath = normalizePathSlashes(nodeResolveSync(source, { basedir: dirname(id) }));
-      if (dynamicRequireModuleSet.has(resolvedPath)) {
+      if (dynamicRequireModules.has(resolvedPath)) {
         return true;
       }
     } catch (ex) {
@@ -85,7 +85,7 @@ export function hasDynamicModuleForPath(source, id, dynamicRequireModuleSet) {
 
   for (const attemptExt of ['', '.js', '.json']) {
     const resolvedPath = normalizePathSlashes(resolve(dirname(id), source + attemptExt));
-    if (dynamicRequireModuleSet.has(resolvedPath)) {
+    if (dynamicRequireModules.has(resolvedPath)) {
       return true;
     }
   }

packages/commonjs/src/generate-imports.js

@@ -6,9 +6,8 @@ import getCommonDir from 'commondir';
 import { peerDependencies } from '../package.json';
 
 import analyzeTopLevelStatements from './analyze-top-level-statements';
-import { getDynamicRequireModules } from './dynamic-modules';
+import { getDynamicModuleRegistry, getDynamicRequireModules } from './dynamic-modules';
 
-import getDynamicRequireModuleSet from './dynamic-require-paths';
 import {
   DYNAMIC_MODULES_ID,
   ES_IMPORT_SUFFIX,
@@ -61,10 +60,11 @@ export default function commonjs(options = {}) {
     getWrappedIds,
     isRequiredId
   } = getResolveRequireSourcesAndGetMeta(extensions, detectCycles);
-  const dynamicRequireModuleSet = getDynamicRequireModuleSet(options.dynamicRequireTargets);
-  const isDynamicRequireModulesEnabled = dynamicRequireModuleSet.size > 0;
+  const dynamicRequireModules = getDynamicRequireModules(options.dynamicRequireTargets);
+  const isDynamicRequireModulesEnabled = dynamicRequireModules.size > 0;
+  // TODO Lukas do we need the CWD?
   const commonDir = isDynamicRequireModulesEnabled
-    ? getCommonDir(null, Array.from(dynamicRequireModuleSet).concat(process.cwd()))
+    ? getCommonDir(null, Array.from(dynamicRequireModules.keys()).concat(process.cwd()))
     : null;
 
   const esModulesWithDefaultExport = new Set();
@@ -111,7 +111,7 @@ export default function commonjs(options = {}) {
     }
 
     if (
-      !dynamicRequireModuleSet.has(normalizePathSlashes(id)) &&
+      !dynamicRequireModules.has(normalizePathSlashes(id)) &&
       (!(hasCjsKeywords(code, ignoreGlobal) || isRequiredId(id)) ||
         (isEsModule && !options.transformMixedEsModules))
     ) {
@@ -120,7 +120,7 @@ export default function commonjs(options = {}) {
 
     const needsRequireWrapper =
       !isEsModule &&
-      (dynamicRequireModuleSet.has(normalizePathSlashes(id)) || strictRequiresFilter(id));
+      (dynamicRequireModules.has(normalizePathSlashes(id)) || strictRequiresFilter(id));
 
     return transformCommonjs(
       this.parse,
@@ -133,7 +133,7 @@ export default function commonjs(options = {}) {
       getIgnoreTryCatchRequireStatementMode,
       sourceMap,
       isDynamicRequireModulesEnabled,
-      dynamicRequireModuleSet,
+      dynamicRequireModules,
       commonDir,
       ast,
       defaultIsModuleExports,
@@ -146,10 +146,9 @@ export default function commonjs(options = {}) {
   return {
     name: 'commonjs',
 
-    options(options) {
+    options({ plugins }) {
       // Always sort the node-resolve plugin after the commonjs plugin as otherwise CommonJS entries
       // will not work with strictRequires: true
-      const { plugins } = options;
       if (Array.isArray(plugins)) {
         const cjsIndex = plugins.findIndex((plugin) => plugin.name === 'commonjs');
         const nodeResolveIndex = plugins.findIndex((plugin) => plugin.name === 'node-resolve');
@@ -228,9 +227,9 @@ export default function commonjs(options = {}) {
       }
 
       if (id === DYNAMIC_MODULES_ID) {
-        return getDynamicRequireModules(
+        return getDynamicModuleRegistry(
           isDynamicRequireModulesEnabled,
-          dynamicRequireModuleSet,
+          dynamicRequireModules,
           commonDir,
           ignoreDynamicRequires
         );

packages/commonjs/src/index.js

@@ -46,7 +46,7 @@ export default async function transformCommonjs(
   getIgnoreTryCatchRequireStatementMode,
   sourceMap,
   isDynamicRequireModulesEnabled,
-  dynamicRequireModuleSet,
+  dynamicRequireModules,
   commonDir,
   astCache,
   defaultIsModuleExports,
@@ -195,7 +195,7 @@ export default async function transformCommonjs(
             node.callee.object &&
             node.callee.object.name === 'require' &&
             node.callee.property.name === 'resolve' &&
-            hasDynamicModuleForPath(id, '/', dynamicRequireModuleSet)
+            hasDynamicModuleForPath(id, '/', dynamicRequireModules)
           ) {
             // TODO Lukas reimplement?
             uses.require = true;
@@ -288,7 +288,7 @@ export default async function transformCommonjs(
               uses.require = true;
               if (isNodeRequirePropertyAccess(parent)) {
                 // TODO Lukas reimplement?
-                if (hasDynamicModuleForPath(id, '/', dynamicRequireModuleSet)) {
+                if (hasDynamicModuleForPath(id, '/', dynamicRequireModules)) {
                   if (parent.property.name === 'cache') {
                     magicString.overwrite(node.start, node.end, dynamicRequireName, {
                       storeName: true

packages/commonjs/src/transform-commonjs.js

@@ -5624,7 +5624,7 @@ Generated by [AVA](https://avajs.dev).
       `,
     }
 
-## strict-requires-entry
+## strict-requires-entry-node-resolve
 
 > Snapshot 1
 
@@ -5883,40 +5883,6 @@ Generated by [AVA](https://avajs.dev).
       `,
     }
 
-## strict-requires-magic-string
-
-> Snapshot 1
-
-    {
-      'main.js': `'use strict';␊
-      ␊
-      Object.defineProperty(exports, '__esModule', { value: true });␊
-      ␊
-      var main = {};␊
-      ␊
-      var hasRequiredMain;␊
-      ␊
-      function requireMain () {␊
-      	if (hasRequiredMain) return main;␊
-      	hasRequiredMain = 1;␊
-      	console.log('hey');␊
-      	// [email protected]␊
-      	const m = new MagicString('0123456789');␊
-      	console.log(␊
-      	  m.prependRight(0, 'W').prependLeft(3, 'AB').appendRight(9, 'XY').remove(6, 8).toString()␊
-      	);␊
-      	const bundle = new MagicString.Bundle();␊
-      	bundle.addSource({ filename: 'foo.txt', content: m });␊
-      	const map = bundle.generateMap({ file: 'bundle.txt', includeContent: true, hires: true });␊
-      	console.log(JSON.stringify(map));␊
-      	main.foo = 'foo';␊
-      	return main;␊
-      }␊
-      ␊
-      exports.__require = requireMain;␊
-      `,
-    }
-
 ## strict-requires-multiple-entry
 
 > Snapshot 1
@@ -6807,27 +6773,3 @@ Generated by [AVA](https://avajs.dev).
       module.exports = main;␊
       `,
     }
-
-## strict-requires-entry-node-resolve
-
-> Snapshot 1
-
-    {
-      'main.js': `'use strict';␊
-      ␊
-      var main = {};␊
-      ␊
-      var hasRequiredMain;␊
-      ␊
-      function requireMain () {␊
-      	if (hasRequiredMain) return main;␊
-      	hasRequiredMain = 1;␊
-      	main.foo = 'foo';␊
-      	return main;␊
-      }␊
-      ␊
-      var mainExports = requireMain();␊
-      ␊
-      module.exports = mainExports;␊
-      `,
-    }