feat(commonjs): pass type of import to node-resolve (#611)

* refactor(commonjs): Refine logic for id wrapping, import transformation

* feat(commonjs): Pass type of import to node-resolve

Author: lukastaegert

package.json

@@ -26,7 +26,7 @@
     "@ava/babel": "^1.0.1",
     "@typescript-eslint/eslint-plugin": "^3.7.1",
     "@typescript-eslint/parser": "^3.7.1",
-    "ava": "^3.11.0",
+    "ava": "^3.13.0",
     "chalk": "^4.1.0",
     "codecov-lite": "^1.0.3",
     "del-cli": "^3.0.1",
@@ -36,7 +36,7 @@
     "husky": "^4.2.5",
     "lint-staged": "^10.2.11",
     "nyc": "^15.1.0",
-    "pnpm": "^5.4.6",
+    "pnpm": "^5.9.3",
     "prettier": "^2.0.5",
     "prettier-plugin-package": "^1.0.0",
     "ts-node": "^8.10.2",

packages/commonjs/README.md

@@ -34,9 +34,9 @@ export default {
   input: 'src/index.js',
   output: {
     dir: 'output',
-    format: 'cjs',
+    format: 'cjs'
   },
-  plugins: [commonjs()],
+  plugins: [commonjs()]
 };
 ```
 
@@ -66,8 +66,8 @@ commonjs({
     '!node_modules/logform/index.js',
     '!node_modules/logform/format.js',
     '!node_modules/logform/levels.js',
-    '!node_modules/logform/browser.js',
-  ],
+    '!node_modules/logform/browser.js'
+  ]
 });
 ```
 
@@ -143,7 +143,7 @@ You can also supply an array of ids to be treated as ES modules, or a function t
 
 ### `requireReturnsDefault`
 
-Type: `boolean | "auto" | "preferred" | ((id: string) => boolean | "auto" | "preferred")`<br>
+Type: `boolean | "namespace" | "auto" | "preferred" | ((id: string) => boolean | "auto" | "preferred")`<br>
 Default: `false`
 
 Controls what is returned when requiring an ES module from a CommonJS file. When using the `esmExternals` option, this will also apply to external modules. By default, this plugin will render those imports as namespace imports, i.e.
@@ -174,7 +174,7 @@ This is in line with how other bundlers handle this situation and is also the mo
 
   var dep$1 = /*#__PURE__*/ Object.freeze({
     __proto__: null,
-    default: dep,
+    default: dep
   });
 
   console.log(dep$1.default);
@@ -205,7 +205,7 @@ For these situations, you can change Rollup's behaviour either globally or per m
               enumerable: true,
               get: function () {
                 return n[k];
-              },
+              }
             }
       );
     });
@@ -287,9 +287,9 @@ export default {
   output: {
     file: 'bundle.js',
     format: 'iife',
-    name: 'MyModule',
+    name: 'MyModule'
   },
-  plugins: [resolve(), commonjs()],
+  plugins: [resolve(), commonjs()]
 };
 ```
 
@@ -299,7 +299,7 @@ Symlinks are common in monorepos and are also created by the `npm link` command.
 
 ```js
 commonjs({
-  include: /node_modules/,
+  include: /node_modules/
 });
 ```
 
@@ -322,11 +322,11 @@ function cjsDetectionPlugin() {
     moduleParsed({
       id,
       meta: {
-        commonjs: { isCommonJS },
-      },
+        commonjs: { isCommonJS }
+      }
     }) {
       console.log(`File ${id} is CommonJS: ${isCommonJS}`);
-    },
+    }
   };
 }
 ```

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

@@ -43,9 +43,7 @@ export function getDynamicPackagesEntryIntro(
   ).join('\n');
 
   if (dynamicRequireModuleDirPaths.length) {
-    dynamicImports += `require(${JSON.stringify(
-      DYNAMIC_REGISTER_PREFIX + DYNAMIC_PACKAGES_ID
-    )});`;
+    dynamicImports += `require(${JSON.stringify(DYNAMIC_REGISTER_PREFIX + DYNAMIC_PACKAGES_ID)});`;
   }
 
   return dynamicImports;

packages/commonjs/src/helpers.js

@@ -1,10 +1,10 @@
-export const PROXY_SUFFIX = '?commonjs-proxy';
-export const getProxyId = (id) => `\0${id}${PROXY_SUFFIX}`;
-export const getIdFromProxyId = (proxyId) => proxyId.slice(1, -PROXY_SUFFIX.length);
+export const isWrappedId = (id, suffix) => id.endsWith(suffix);
+export const wrapId = (id, suffix) => `\0${id}${suffix}`;
+export const unwrapId = (wrappedId, suffix) => wrappedId.slice(1, -suffix.length);
 
+export const PROXY_SUFFIX = '?commonjs-proxy';
+export const REQUIRE_SUFFIX = '?commonjs-require';
 export const EXTERNAL_SUFFIX = '?commonjs-external';
-export const getExternalProxyId = (id) => `\0${id}${EXTERNAL_SUFFIX}`;
-export const getIdFromExternalProxyId = (proxyId) => proxyId.slice(1, -EXTERNAL_SUFFIX.length);
 
 export const VIRTUAL_PATH_BASE = '/$$rollup_base$$';
 export const getVirtualPathForDynamicRequirePath = (path, commonDir) => {

packages/commonjs/src/index.js

@@ -16,10 +16,10 @@ import {
   DYNAMIC_PACKAGES_ID,
   EXTERNAL_SUFFIX,
   getHelpersModule,
-  getIdFromExternalProxyId,
-  getIdFromProxyId,
   HELPERS_ID,
-  PROXY_SUFFIX
+  isWrappedId,
+  PROXY_SUFFIX,
+  unwrapId
 } from './helpers';
 import { setIsCjsPromise } from './is-cjs';
 import {
@@ -81,10 +81,8 @@ export default function commonjs(options = {}) {
 
   function transformAndCheckExports(code, id) {
     if (isDynamicRequireModulesEnabled && this.getModuleInfo(id).isEntry) {
-      code = getDynamicPackagesEntryIntro(
-        dynamicRequireModuleDirPaths,
-        dynamicRequireModuleSet
-      ) + code;
+      code =
+        getDynamicPackagesEntryIntro(dynamicRequireModuleDirPaths, dynamicRequireModuleSet) + code;
     }
 
     const { isEsModule, hasDefaultExport, hasNamedExports, ast } = checkEsModule(
@@ -156,8 +154,8 @@ export default function commonjs(options = {}) {
         return getSpecificHelperProxy(id);
       }
 
-      if (id.endsWith(EXTERNAL_SUFFIX)) {
-        const actualId = getIdFromExternalProxyId(id);
+      if (isWrappedId(id, EXTERNAL_SUFFIX)) {
+        const actualId = unwrapId(id, EXTERNAL_SUFFIX);
         return getUnknownRequireProxy(
           actualId,
           isEsmExternal(actualId) ? getRequireReturnsDefault(actualId) : true
@@ -176,8 +174,8 @@ export default function commonjs(options = {}) {
         return getDynamicRequireProxy(normalizePathSlashes(id), commonDir);
       }
 
-      if (id.endsWith(PROXY_SUFFIX)) {
-        const actualId = getIdFromProxyId(id);
+      if (isWrappedId(id, PROXY_SUFFIX)) {
+        const actualId = unwrapId(id, PROXY_SUFFIX);
         return getStaticRequireProxy(
           actualId,
           getRequireReturnsDefault(actualId),

packages/commonjs/src/resolve-id.js

@@ -6,11 +6,13 @@ import { dirname, resolve, sep } from 'path';
 import {
   DYNAMIC_JSON_PREFIX,
   DYNAMIC_PACKAGES_ID,
-  getExternalProxyId,
-  getIdFromProxyId,
-  getProxyId,
+  EXTERNAL_SUFFIX,
   HELPERS_ID,
-  PROXY_SUFFIX
+  isWrappedId,
+  PROXY_SUFFIX,
+  REQUIRE_SUFFIX,
+  unwrapId,
+  wrapId
 } from './helpers';
 
 function getCandidatesForExtension(resolved, extension) {
@@ -44,10 +46,18 @@ export default function getResolveId(extensions) {
     return undefined;
   }
 
-  function resolveId(importee, importer) {
-    const isProxyModule = importee.endsWith(PROXY_SUFFIX);
+  return function resolveId(importee, importer) {
+    // Proxies are only importing resolved ids, no need to resolve again
+    if (importer && isWrappedId(importer, PROXY_SUFFIX)) {
+      return importee;
+    }
+
+    const isProxyModule = isWrappedId(importee, PROXY_SUFFIX);
+    const isRequiredModule = isWrappedId(importee, REQUIRE_SUFFIX);
     if (isProxyModule) {
-      importee = getIdFromProxyId(importee);
+      importee = unwrapId(importee, PROXY_SUFFIX);
+    } else if (isRequiredModule) {
+      importee = unwrapId(importee, REQUIRE_SUFFIX);
     }
     if (importee.startsWith('\0')) {
       if (
@@ -57,30 +67,23 @@ export default function getResolveId(extensions) {
       ) {
         return importee;
       }
-      if (!isProxyModule) {
-        return null;
-      }
+      return null;
     }
 
-    if (importer && importer.endsWith(PROXY_SUFFIX)) {
-      importer = getIdFromProxyId(importer);
-    }
-
-    return this.resolve(importee, importer, { skipSelf: true }).then((resolved) => {
+    return this.resolve(importee, importer, {
+      skipSelf: true,
+      custom: { 'node-resolve': { isRequire: isProxyModule || isRequiredModule } }
+    }).then((resolved) => {
       if (!resolved) {
         resolved = resolveExtensions(importee, importer);
       }
-      if (isProxyModule) {
-        if (!resolved) {
-          return { id: getExternalProxyId(importee), external: false };
-        }
-        resolved.id = (resolved.external ? getExternalProxyId : getProxyId)(resolved.id);
+      if (resolved && isProxyModule) {
+        resolved.id = wrapId(resolved.id, resolved.external ? EXTERNAL_SUFFIX : PROXY_SUFFIX);
         resolved.external = false;
-        return resolved;
+      } else if (!resolved && (isProxyModule || isRequiredModule)) {
+        return { id: wrapId(importee, EXTERNAL_SUFFIX), external: false };
       }
       return resolved;
     });
-  }
-
-  return resolveId;
+  };
 }

packages/commonjs/src/transform.js

@@ -11,9 +11,11 @@ import { flatten, isFalsy, isReference, isTruthy } from './ast-utils';
 import {
   DYNAMIC_JSON_PREFIX,
   DYNAMIC_REGISTER_PREFIX,
-  getProxyId,
   getVirtualPathForDynamicRequirePath,
-  HELPERS_ID
+  HELPERS_ID,
+  PROXY_SUFFIX,
+  REQUIRE_SUFFIX,
+  wrapId
 } from './helpers';
 import { getName } from './utils';
 
@@ -138,7 +140,8 @@ export function transformCommonjs(
   const required = {};
   // Because objects have no guaranteed ordering, yet we need it,
   // we need to keep track of the order in a array
-  const sources = [];
+  const requiredSources = [];
+  const dynamicRegisterSources = [];
 
   let uid = 0;
 
@@ -229,8 +232,7 @@ export function transformCommonjs(
     }
 
     const existing = required[sourceId];
-    // eslint-disable-next-line no-undefined
-    if (existing === undefined) {
+    if (!existing) {
       const isDynamic = hasDynamicModuleForPath(sourceId);
 
       if (!name) {
@@ -240,12 +242,15 @@ export function transformCommonjs(
         } while (scope.contains(name));
       }
 
-      if (isDynamicRegister && sourceId.endsWith('.json')) {
-        sourceId = DYNAMIC_JSON_PREFIX + sourceId;
+      if (isDynamicRegister) {
+        if (sourceId.endsWith('.json')) {
+          sourceId = DYNAMIC_JSON_PREFIX + sourceId;
+        }
+        dynamicRegisterSources.push(sourceId);
       }
 
-      if (isDynamicRegister || !isDynamic || sourceId.endsWith('.json')) {
-        sources.push([sourceId, isDynamicRegister]);
+      if (!isDynamic || sourceId.endsWith('.json')) {
+        requiredSources.push(sourceId);
       }
 
       required[sourceId] = { source: sourceId, name, importsDefault: false, isDynamic };
@@ -553,7 +558,8 @@ export function transformCommonjs(
   usesCommonjsHelpers = usesCommonjsHelpers || shouldWrap;
 
   if (
-    !sources.length &&
+    !requiredSources.length &&
+    !dynamicRegisterSources.length &&
     !uses.module &&
     !uses.exports &&
     !uses.require &&
@@ -568,25 +574,22 @@ export function transformCommonjs(
     : []
   )
     .concat(
-      // dynamic registers first (`commonjsRegister(,,,)`), as the may be required in the other modules
-      sources
-        .filter(([, isDynamicRegister]) => isDynamicRegister)
-        .map(([source]) => `import '${source}';`),
-
-      // now the solid modules, non-commonjsRegister
-      sources
-        .filter(([, isDynamicRegister]) => !isDynamicRegister)
-        .map(([source]) => `import '${source}';`),
-
-      // now the proxies for solid modules (non-commonjsRegister)
-      sources
-        .filter(([, isDynamicRegister]) => !isDynamicRegister)
-        .map(([source]) => {
-          const { name, importsDefault } = required[source];
-          return `import ${importsDefault ? `${name} from ` : ``}'${
-            source.startsWith('\0') ? source : getProxyId(source)
-          }';`;
-        })
+      // dynamic registers first, as the may be required in the other modules
+      dynamicRegisterSources.map((source) => `import '${source}';`),
+
+      // now the actual modules so that they are analyzed before creating the proxies;
+      // no need to do this for virtual modules as we never proxy them
+      requiredSources
+        .filter((source) => !source.startsWith('\0'))
+        .map((source) => `import '${wrapId(source, REQUIRE_SUFFIX)}';`),
+
+      // now the proxy modules
+      requiredSources.map((source) => {
+        const { name, importsDefault } = required[source];
+        return `import ${importsDefault ? `${name} from ` : ``}'${
+          source.startsWith('\0') ? source : wrapId(source, PROXY_SUFFIX)
+        }';`;
+      })
     )
     .join('\n')}\n\n`;
 

packages/commonjs/test/fixtures/form/constant-template-literal/output.js

@@ -1,4 +1,4 @@
-import 'tape';
+import '_tape?commonjs-require';
 import foo from '_tape?commonjs-proxy';
 
 console.log(foo);

packages/commonjs/test/fixtures/form/ignore-ids-function/output.js

@@ -1,4 +1,4 @@
-import 'bar';
+import '_bar?commonjs-require';
 import bar from '_bar?commonjs-proxy';
 
 var foo = require( 'foo' );

packages/commonjs/test/fixtures/form/ignore-ids/output.js

@@ -1,4 +1,4 @@
-import 'bar';
+import '_bar?commonjs-require';
 import bar from '_bar?commonjs-proxy';
 
 var foo = require( 'foo' );