fix(commonjs): Warn when plugins do not pass options to resolveId (#1038

)

packages/commonjs/README.md

@@ -68,6 +68,8 @@ You can also provide a [minimatch pattern](https://github.com/isaacs/minimatch),
 Type: `string | string[]`<br>
 Default: `[]`
 
+_Note: In previous versions, this option would spin up a rather comprehensive mock environment that was capable of handling modules that manipulate `require.cache`. This is no longer supported. If you rely on this e.g. when using request-promise-native, use version 21 of this plugin._
+
 Some modules contain dynamic `require` calls, or require modules that contain circular dependencies, which are not handled well by static imports.
 Including those modules as `dynamicRequireTargets` will simulate a CommonJS (NodeJS-like) environment for them with support for dynamic dependencies. It also enables `strictRequires` for those modules, see above.
 

packages/commonjs/src/index.js

@@ -99,7 +99,7 @@ export default function commonjs(options = {}) {
     };
   };
 
-  const resolveId = getResolveId(extensions);
+  const { currentlyResolving, resolveId } = getResolveId(extensions);
 
   const sourceMap = options.sourceMap !== false;
 
@@ -204,7 +204,11 @@ export default function commonjs(options = {}) {
           'The namedExports option from "@rollup/plugin-commonjs" is deprecated. Named exports are now handled automatically.'
         );
       }
-      requireResolver = getRequireResolver(extensions, detectCyclesAndConditional);
+      requireResolver = getRequireResolver(
+        extensions,
+        detectCyclesAndConditional,
+        currentlyResolving
+      );
     },
 
     buildEnd() {
@@ -260,15 +264,13 @@ export default function commonjs(options = {}) {
 
       // entry suffix is just appended to not mess up relative external resolution
       if (id.endsWith(ENTRY_SUFFIX)) {
-        return getEntryProxy(
-          id.slice(0, -ENTRY_SUFFIX.length),
-          defaultIsModuleExports,
-          this.getModuleInfo
-        );
+        const acutalId = id.slice(0, -ENTRY_SUFFIX.length);
+        return getEntryProxy(acutalId, getDefaultIsModuleExports(acutalId), this.getModuleInfo);
       }
 
       if (isWrappedId(id, ES_IMPORT_SUFFIX)) {
-        return getEsImportProxy(unwrapId(id, ES_IMPORT_SUFFIX), defaultIsModuleExports);
+        const actualId = unwrapId(id, ES_IMPORT_SUFFIX);
+        return getEsImportProxy(actualId, getDefaultIsModuleExports(actualId));
       }
 
       if (id === DYNAMIC_MODULES_ID) {

packages/commonjs/src/resolve-id.js

@@ -50,79 +50,114 @@ export function resolveExtensions(importee, importer, extensions) {
 }
 
 export default function getResolveId(extensions) {
-  return async function resolveId(importee, importer, resolveOptions) {
-    // We assume that all requires are pre-resolved
-    const customOptions = resolveOptions.custom;
-    if (customOptions && customOptions['node-resolve'] && customOptions['node-resolve'].isRequire) {
-      return null;
-    }
-    if (isWrappedId(importee, WRAPPED_SUFFIX)) {
-      return unwrapId(importee, WRAPPED_SUFFIX);
-    }
+  const currentlyResolving = new Map();
 
-    if (
-      importee.endsWith(ENTRY_SUFFIX) ||
-      isWrappedId(importee, MODULE_SUFFIX) ||
-      isWrappedId(importee, EXPORTS_SUFFIX) ||
-      isWrappedId(importee, PROXY_SUFFIX) ||
-      isWrappedId(importee, ES_IMPORT_SUFFIX) ||
-      isWrappedId(importee, EXTERNAL_SUFFIX) ||
-      importee.startsWith(HELPERS_ID) ||
-      importee === DYNAMIC_MODULES_ID
-    ) {
-      return importee;
-    }
+  return {
+    /**
+     * This is a Maps of importers to Sets of require sources being resolved at
+     * the moment by resolveRequireSourcesAndUpdateMeta
+     */
+    currentlyResolving,
+    async resolveId(importee, importer, resolveOptions) {
+      const customOptions = resolveOptions.custom;
+      // All logic below is specific to ES imports.
+      // Also, if we do not skip this logic for requires that are resolved while
+      // transforming a commonjs file, it can easily lead to deadlocks.
+      if (
+        customOptions &&
+        customOptions['node-resolve'] &&
+        customOptions['node-resolve'].isRequire
+      ) {
+        return null;
+      }
+      const currentlyResolvingForParent = currentlyResolving.get(importer);
+      if (currentlyResolvingForParent && currentlyResolvingForParent.has(importee)) {
+        this.warn({
+          code: 'THIS_RESOLVE_WITHOUT_OPTIONS',
+          message:
+            'It appears a plugin has implemented a "resolveId" hook that uses "this.resolve" without forwarding the third "options" parameter of "resolveId". This is problematic as it can lead to wrong module resolutions especially for the node-resolve plugin and in certain cases cause early exit errors for the commonjs plugin.\nIn rare cases, this warning can appear if the same file is both imported and required from the same mixed ES/CommonJS module, in which case it can be ignored.',
+          url: 'https://rollupjs.org/guide/en/#resolveid'
+        });
+        return null;
+      }
+
+      if (isWrappedId(importee, WRAPPED_SUFFIX)) {
+        return unwrapId(importee, WRAPPED_SUFFIX);
+      }
 
-    if (importer) {
       if (
-        importer === DYNAMIC_MODULES_ID ||
-        // Proxies are only importing resolved ids, no need to resolve again
-        isWrappedId(importer, PROXY_SUFFIX) ||
-        isWrappedId(importer, ES_IMPORT_SUFFIX) ||
-        importer.endsWith(ENTRY_SUFFIX)
+        importee.endsWith(ENTRY_SUFFIX) ||
+        isWrappedId(importee, MODULE_SUFFIX) ||
+        isWrappedId(importee, EXPORTS_SUFFIX) ||
+        isWrappedId(importee, PROXY_SUFFIX) ||
+        isWrappedId(importee, ES_IMPORT_SUFFIX) ||
+        isWrappedId(importee, EXTERNAL_SUFFIX) ||
+        importee.startsWith(HELPERS_ID) ||
+        importee === DYNAMIC_MODULES_ID
       ) {
         return importee;
       }
-      if (isWrappedId(importer, EXTERNAL_SUFFIX)) {
-        // We need to return null for unresolved imports so that the proper warning is shown
-        if (!(await this.resolve(importee, importer, { skipSelf: true }))) {
-          return null;
+
+      if (importer) {
+        if (
+          importer === DYNAMIC_MODULES_ID ||
+          // Proxies are only importing resolved ids, no need to resolve again
+          isWrappedId(importer, PROXY_SUFFIX) ||
+          isWrappedId(importer, ES_IMPORT_SUFFIX) ||
+          importer.endsWith(ENTRY_SUFFIX)
+        ) {
+          return importee;
+        }
+        if (isWrappedId(importer, EXTERNAL_SUFFIX)) {
+          // We need to return null for unresolved imports so that the proper warning is shown
+          if (
+            !(await this.resolve(
+              importee,
+              importer,
+              Object.assign({ skipSelf: true }, resolveOptions)
+            ))
+          ) {
+            return null;
+          }
+          // For other external imports, we need to make sure they are handled as external
+          return { id: importee, external: true };
         }
-        // For other external imports, we need to make sure they are handled as external
-        return { id: importee, external: true };
       }
-    }
 
-    if (importee.startsWith('\0')) {
-      return null;
-    }
+      if (importee.startsWith('\0')) {
+        return null;
+      }
 
-    // If this is an entry point or ESM import, we need to figure out if the importee is wrapped and
-    // if that is the case, we need to add a proxy.
-    const resolved =
-      (await this.resolve(importee, importer, Object.assign({ skipSelf: true }, resolveOptions))) ||
-      resolveExtensions(importee, importer, extensions);
-    // Make sure that even if other plugins resolve again, we ignore our own proxies
-    if (
-      !resolved ||
-      resolved.external ||
-      resolved.id.endsWith(ENTRY_SUFFIX) ||
-      isWrappedId(resolved.id, ES_IMPORT_SUFFIX)
-    ) {
+      // If this is an entry point or ESM import, we need to figure out if the importee is wrapped and
+      // if that is the case, we need to add a proxy.
+      const resolved =
+        (await this.resolve(
+          importee,
+          importer,
+          Object.assign({ skipSelf: true }, resolveOptions)
+        )) || resolveExtensions(importee, importer, extensions);
+      // Make sure that even if other plugins resolve again, we ignore our own proxies
+      if (
+        !resolved ||
+        resolved.external ||
+        resolved.id.endsWith(ENTRY_SUFFIX) ||
+        isWrappedId(resolved.id, ES_IMPORT_SUFFIX)
+      ) {
+        return resolved;
+      }
+      const moduleInfo = await this.load(resolved);
+      if (resolveOptions.isEntry) {
+        moduleInfo.moduleSideEffects = true;
+        // We must not precede entry proxies with a `\0` as that will mess up relative external resolution
+        return resolved.id + ENTRY_SUFFIX;
+      }
+      const {
+        meta: { commonjs: commonjsMeta }
+      } = moduleInfo;
+      if (commonjsMeta && commonjsMeta.isCommonJS === IS_WRAPPED_COMMONJS) {
+        return { id: wrapId(resolved.id, ES_IMPORT_SUFFIX), meta: { commonjs: { resolved } } };
+      }
       return resolved;
     }
-    const moduleInfo = await this.load(resolved);
-    if (resolveOptions.isEntry) {
-      moduleInfo.moduleSideEffects = true;
-      // We must not precede entry proxies with a `\0` as that will mess up relative external resolution
-      return resolved.id + ENTRY_SUFFIX;
-    }
-    const {
-      meta: { commonjs: commonjsMeta }
-    } = moduleInfo;
-    if (commonjsMeta && commonjsMeta.isCommonJS === IS_WRAPPED_COMMONJS) {
-      return { id: wrapId(resolved.id, ES_IMPORT_SUFFIX), meta: { commonjs: { resolved } } };
-    }
-    return resolved;
   };
 }

packages/commonjs/src/resolve-require-sources.js

@@ -9,7 +9,7 @@ import {
 } from './helpers';
 import { resolveExtensions } from './resolve-id';
 
-export function getRequireResolver(extensions, detectCyclesAndConditional) {
+export function getRequireResolver(extensions, detectCyclesAndConditional, currentlyResolving) {
   const knownCjsModuleTypes = Object.create(null);
   const requiredIds = Object.create(null);
   const unconditionallyRequiredIds = Object.create(null);
@@ -161,16 +161,20 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) {
       parentMeta.requires = [];
       parentMeta.isRequiredCommonJS = Object.create(null);
       setInitialParentType(parentId, isParentCommonJS);
+      const currentlyResolvingForParent = currentlyResolving.get(parentId) || new Set();
+      currentlyResolving.set(parentId, currentlyResolvingForParent);
       const requireTargets = await Promise.all(
         sources.map(async ({ source, isConditional }) => {
           // Never analyze or proxy internal modules
           if (source.startsWith('\0')) {
             return { id: source, allowProxy: false };
           }
+          currentlyResolvingForParent.add(source);
           const resolved =
             (await rollupContext.resolve(source, parentId, {
               custom: { 'node-resolve': { isRequire: true } }
             })) || resolveExtensions(source, parentId, extensions);
+          currentlyResolvingForParent.delete(source);
           if (!resolved) {
             return { id: wrapId(source, EXTERNAL_SUFFIX), allowProxy: false };
           }
@@ -201,6 +205,10 @@ export function getRequireResolver(extensions, detectCyclesAndConditional) {
           isCommonJS
         };
       });
+    },
+    isCurrentlyResolving(source, parentId) {
+      const currentlyResolvingForParent = currentlyResolving.get(parentId);
+      return currentlyResolvingForParent && currentlyResolvingForParent.has(source);
     }
   };
 }

packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/_config.js

@@ -0,0 +1,29 @@
+const assert = require('assert');
+
+const warnings = [];
+
+module.exports = {
+  description: 'Warns when another plugin uses this.resolve without forwarding options',
+  options: {
+    onwarn(warning) {
+      warnings.push(warning);
+    },
+    plugins: [
+      {
+        name: 'test',
+        resolveId(source, importer) {
+          return this.resolve(source, importer, { skipSelf: true });
+        },
+        buildEnd() {
+          assert.strictEqual(warnings.length, 1);
+          assert.strictEqual(
+            warnings[0].message,
+            'It appears a plugin has implemented a "resolveId" hook that uses "this.resolve" without forwarding the third "options" parameter of "resolveId". This is problematic as it can lead to wrong module resolutions especially for the node-resolve plugin and in certain cases cause early exit errors for the commonjs plugin.\nIn rare cases, this warning can appear if the same file is both imported and required from the same mixed ES/CommonJS module, in which case it can be ignored.'
+          );
+          assert.strictEqual(warnings[0].pluginCode, 'THIS_RESOLVE_WITHOUT_OPTIONS');
+          assert.strictEqual(warnings[0].url, 'https://rollupjs.org/guide/en/#resolveid');
+        }
+      }
+    ]
+  }
+};

packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/foo.js

@@ -0,0 +1 @@
+module.exports = 21;

packages/commonjs/test/fixtures/function/warn-this-resolve-without-options/main.js

@@ -0,0 +1,3 @@
+const foo = require('./foo');
+
+module.exports = foo * 2;

packages/commonjs/test/snapshots/function.js.md

@@ -7713,3 +7713,20 @@ Generated by [AVA](https://avajs.dev).
       module.exports = main;␊
       `,
     }
+
+## warn-this-resolve-without-options
+
+> Snapshot 1
+
+    {
+      'main.js': `'use strict';␊
+      ␊
+      var foo$1 = 21;␊
+      ␊
+      const foo = foo$1;␊
+      ␊
+      var main = foo * 2;␊
+      ␊
+      module.exports = main;␊
+      `,
+    }