fix(node-resolve): do not ignore exceptions (#564)

* do not ignore errors

* do not error when import specifier does not map to file

* always swallow MODULE_NOT_FOUND errors

as not being able to resolve the id is a valid output

* return null from resolveId early if can't resolve

* strip null byte from importer if there is one

* fix root-dir test

this was passing before because the real error was being ignored

* remove try/catch

* use async exists() and realpath()

like we do in index.js

* pretend importer is `undefined` if it starts will \0

* return null if file not found with modulesOnly option

Author: tjenkinson

packages/node-resolve/src/index.js

@@ -100,6 +100,10 @@ export function nodeResolve(opts = {}) {
       // ignore IDs with null character, these belong to other plugins
       if (/\0/.test(importee)) return null;
 
+      if (/\0/.test(importer)) {
+        importer = undefined;
+      }
+
       // strip query params from import
       const [importPath, params] = importee.split('?');
       const importSuffix = `${params ? `?${params}` : ''}`;
@@ -218,62 +222,61 @@ export function nodeResolve(opts = {}) {
       importSpecifierList.push(importee);
       resolveOptions = Object.assign(resolveOptions, customResolveOptions);
 
-      try {
-        let resolved = await resolveImportSpecifiers(importSpecifierList, resolveOptions);
+      let resolved = await resolveImportSpecifiers(importSpecifierList, resolveOptions);
+      if (!resolved) {
+        return null;
+      }
 
-        if (resolved && packageBrowserField) {
-          if (Object.prototype.hasOwnProperty.call(packageBrowserField, resolved)) {
-            if (!packageBrowserField[resolved]) {
-              browserMapCache.set(resolved, packageBrowserField);
-              return ES6_BROWSER_EMPTY;
-            }
-            resolved = packageBrowserField[resolved];
+      if (packageBrowserField) {
+        if (Object.prototype.hasOwnProperty.call(packageBrowserField, resolved)) {
+          if (!packageBrowserField[resolved]) {
+            browserMapCache.set(resolved, packageBrowserField);
+            return ES6_BROWSER_EMPTY;
           }
-          browserMapCache.set(resolved, packageBrowserField);
+          resolved = packageBrowserField[resolved];
         }
+        browserMapCache.set(resolved, packageBrowserField);
+      }
 
-        if (hasPackageEntry && !preserveSymlinks && resolved) {
-          const fileExists = await exists(resolved);
-          if (fileExists) {
-            resolved = await realpath(resolved);
-          }
+      if (hasPackageEntry && !preserveSymlinks) {
+        const fileExists = await exists(resolved);
+        if (fileExists) {
+          resolved = await realpath(resolved);
         }
+      }
 
-        idToPackageInfo.set(resolved, packageInfo);
-
-        if (hasPackageEntry) {
-          if (builtins.has(resolved) && preferBuiltins && isPreferBuiltinsSet) {
-            return null;
-          } else if (importeeIsBuiltin && preferBuiltins) {
-            if (!isPreferBuiltinsSet) {
-              this.warn(
-                `preferring built-in module '${importee}' over local alternative at '${resolved}', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning`
-              );
-            }
-            return null;
-          } else if (jail && resolved.indexOf(normalize(jail.trim(sep))) !== 0) {
-            return null;
-          }
-        }
+      idToPackageInfo.set(resolved, packageInfo);
 
-        if (resolved && options.modulesOnly) {
-          const code = await readFile(resolved, 'utf-8');
-          if (isModule(code)) {
-            return {
-              id: `${resolved}${importSuffix}`,
-              moduleSideEffects: hasModuleSideEffects(resolved)
-            };
+      if (hasPackageEntry) {
+        if (builtins.has(resolved) && preferBuiltins && isPreferBuiltinsSet) {
+          return null;
+        } else if (importeeIsBuiltin && preferBuiltins) {
+          if (!isPreferBuiltinsSet) {
+            this.warn(
+              `preferring built-in module '${importee}' over local alternative at '${resolved}', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning`
+            );
           }
           return null;
+        } else if (jail && resolved.indexOf(normalize(jail.trim(sep))) !== 0) {
+          return null;
+        }
+      }
+
+      if (options.modulesOnly && (await exists(resolved))) {
+        const code = await readFile(resolved, 'utf-8');
+        if (isModule(code)) {
+          return {
+            id: `${resolved}${importSuffix}`,
+            moduleSideEffects: hasModuleSideEffects(resolved)
+          };
         }
-        const result = {
-          id: `${resolved}${importSuffix}`,
-          moduleSideEffects: hasModuleSideEffects(resolved)
-        };
-        return result;
-      } catch (error) {
         return null;
       }
+      const result = {
+        id: `${resolved}${importSuffix}`,
+        moduleSideEffects: hasModuleSideEffects(resolved)
+      };
+      return result;
     },
 
     load(importee) {

packages/node-resolve/src/util.js

@@ -5,7 +5,7 @@ import { createFilter } from '@rollup/pluginutils';
 
 import resolveModule from 'resolve';
 
-import { realpathSync } from './fs';
+import { exists, realpath, realpathSync } from './fs';
 
 const resolveId = promisify(resolveModule);
 
@@ -165,28 +165,28 @@ export function resolveImportSpecifiers(importSpecifierList, resolveOptions) {
   let promise = Promise.resolve();
 
   for (let i = 0; i < importSpecifierList.length; i++) {
-    promise = promise.then((value) => {
+    // eslint-disable-next-line no-loop-func
+    promise = promise.then(async (value) => {
       // if we've already resolved to something, just return it.
       if (value) {
         return value;
       }
 
-      return resolveId(importSpecifierList[i], resolveOptions).then((result) => {
-        if (!resolveOptions.preserveSymlinks) {
-          result = realpathSync(result);
+      let result = await resolveId(importSpecifierList[i], resolveOptions);
+      if (!resolveOptions.preserveSymlinks) {
+        if (await exists(result)) {
+          result = await realpath(result);
         }
-        return result;
-      });
+      }
+      return result;
     });
 
-    if (i < importSpecifierList.length - 1) {
-      // swallow MODULE_NOT_FOUND errors from all but the last resolution
-      promise = promise.catch((error) => {
-        if (error.code !== 'MODULE_NOT_FOUND') {
-          throw error;
-        }
-      });
-    }
+    // swallow MODULE_NOT_FOUND errors
+    promise = promise.catch((error) => {
+      if (error.code !== 'MODULE_NOT_FOUND') {
+        throw error;
+      }
+    });
   }
 
   return promise;

packages/node-resolve/test/root-dir.js

@@ -7,11 +7,9 @@ const { testBundle } = require('../../../util/test');
 
 const { nodeResolve } = require('..');
 
-process.chdir(join(__dirname, 'fixtures', 'monorepo-dedupe', 'packages', 'package-a'));
-
 test('deduplicates modules from the given root directory', async (t) => {
   const bundle = await rollup({
-    input: 'index.js',
+    input: './packages/package-a/index.js',
     plugins: [
       nodeResolve({
         dedupe: ['react'],