fix(commonjs): short-circuit to actual module entry point when using circular ref through a different entry (#888)

Author: danielgindi

packages/commonjs/src/dynamic-packages-manager.js

@@ -21,15 +21,13 @@ export function getPackageEntryPoint(dirPath) {
 }
 
 export function getDynamicPackagesModule(dynamicRequireModuleDirPaths, commonDir) {
-  let code = `const commonjsRegister = require('${HELPERS_ID}?commonjsRegister');`;
+  let code = `const commonjsRegisterOrShort = require('${HELPERS_ID}?commonjsRegisterOrShort');`;
   for (const dir of dynamicRequireModuleDirPaths) {
     const entryPoint = getPackageEntryPoint(dir);
 
-    code += `\ncommonjsRegister(${JSON.stringify(
+    code += `\ncommonjsRegisterOrShort(${JSON.stringify(
       getVirtualPathForDynamicRequirePath(dir, commonDir)
-    )}, function (module, exports) {
-  module.exports = require(${JSON.stringify(normalizePathSlashes(join(dir, entryPoint)))});
-});`;
+    )}, ${JSON.stringify(getVirtualPathForDynamicRequirePath(join(dir, entryPoint), commonDir))});`;
   }
   return code;
 }

packages/commonjs/src/helpers.js

@@ -73,8 +73,18 @@ export function commonjsRegister (path, loader) {
 	DYNAMIC_REQUIRE_LOADERS[path] = loader;
 }
 
+export function commonjsRegisterOrShort (path, to) {
+	const resolvedPath = commonjsResolveImpl(path, null, true);
+	if (resolvedPath !== null && DYNAMIC_REQUIRE_CACHE[resolvedPath]) {
+	  DYNAMIC_REQUIRE_CACHE[path] = DYNAMIC_REQUIRE_CACHE[resolvedPath];
+	} else {
+	  DYNAMIC_REQUIRE_SHORTS[path] = to;
+	}
+}
+
 const DYNAMIC_REQUIRE_LOADERS = Object.create(null);
 const DYNAMIC_REQUIRE_CACHE = Object.create(null);
+const DYNAMIC_REQUIRE_SHORTS = Object.create(null);
 const DEFAULT_PARENT_MODULE = {
 	id: '<' + 'rollup>', exports: {}, parent: undefined, filename: null, loaded: false, children: [], paths: []
 };
@@ -179,10 +189,13 @@ export function commonjsResolveImpl (path, originalModuleDir, testCache) {
 			const resolvedPath = relPath + CHECKED_EXTENSIONS[extensionIndex];
 			if (DYNAMIC_REQUIRE_CACHE[resolvedPath]) {
 				return resolvedPath;
-			};
+			}
+			if (DYNAMIC_REQUIRE_SHORTS[resolvedPath]) {
+			  return resolvedPath;
+			}
 			if (DYNAMIC_REQUIRE_LOADERS[resolvedPath]) {
 				return resolvedPath;
-			};
+			}
 		}
 		if (!shouldTryNodeModules) break;
 		const nextDir = normalize(originalModuleDir + '/..');
@@ -201,10 +214,17 @@ export function commonjsResolve (path, originalModuleDir) {
 }
 
 export function commonjsRequire (path, originalModuleDir) {
-	const resolvedPath = commonjsResolveImpl(path, originalModuleDir, true);
+	let resolvedPath = commonjsResolveImpl(path, originalModuleDir, true);
 	if (resolvedPath !== null) {
     let cachedModule = DYNAMIC_REQUIRE_CACHE[resolvedPath];
     if (cachedModule) return cachedModule.exports;
+    let shortTo = DYNAMIC_REQUIRE_SHORTS[resolvedPath];
+    if (shortTo) {
+      cachedModule = DYNAMIC_REQUIRE_CACHE[shortTo];
+      if (cachedModule)
+        return cachedModule.exports;
+      resolvedPath = commonjsResolveImpl(shortTo, null, true);
+    }
     const loader = DYNAMIC_REQUIRE_LOADERS[resolvedPath];
     if (loader) {
       DYNAMIC_REQUIRE_CACHE[resolvedPath] = cachedModule = {

packages/commonjs/test/fixtures/function/dynamic-require-root-circular/_config.js

@@ -0,0 +1,13 @@
+const { nodeResolve } = require('@rollup/plugin-node-resolve');
+
+module.exports = {
+  description: 'resolves circular references through indirect access (../ to module\'s root)',
+  options: {
+    plugins: [nodeResolve()]
+  },
+  pluginOptions: {
+    dynamicRequireTargets: [
+      'fixtures/function/dynamic-require-root-circular/node_modules/custom-module{,/**/*.js}'
+    ]
+  }
+};

packages/commonjs/test/fixtures/function/dynamic-require-root-circular/main.js

@@ -0,0 +1,7 @@
+/* eslint-disable import/no-dynamic-require, global-require */
+
+const custom = require('custom-module');
+
+t.is(custom.get1(), 'all good');
+t.is(custom.get2(), 'indirect ref');
+t.is(custom.get3(), custom.get1());