feat(node-resolve)!: Mark built-ins as external (#627)

* 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

* treat builtins as externals (depending on useBuiltins option)

* update readme sections on builtins

* fix readme doc about `jail`

* update all other snapshots

* use exists() instead of try/catch

* refactor resolveImportSpecifiers to use async/await

* use async `realpath`

* remove duplicate lines

* remove readme update

* fix doc

* add note that `preferBuiltins` should be `false` for stubbed builtins

packages/node-resolve/README.md

@@ -112,7 +112,7 @@ Specifies the extensions of files that the plugin will operate on.
 Type: `String`<br>
 Default: `'/'`
 
-Locks the module search within specified path (e.g. chroot). Modules defined outside this path will be marked as external.
+Locks the module search within specified path (e.g. chroot). Modules defined outside this path will be ignored by this plugin.
 
 ### `mainFields`
 
@@ -129,7 +129,6 @@ DEPRECATED: use "resolveOnly" instead
 ### `preferBuiltins`
 
 Type: `Boolean`<br>
-Default: `true`
 
 If `true`, the plugin will prefer built-in modules (e.g. `fs`, `path`). If `false`, the plugin will look for locally installed modules of the same name.
 
@@ -183,20 +182,11 @@ export default {
 
 ## Resolving Built-Ins (like `fs`)
 
-This plugin won't resolve any builtins (e.g. `fs`). If you need to resolve builtins you can install local modules and set `preferBuiltins` to `false`, or install a plugin like [rollup-plugin-node-polyfills](https://github.com/ionic-team/rollup-plugin-node-polyfills) which provides stubbed versions of these methods.
+By default this plugin will prefer built-ins over local modules, marking them as external.
 
-If you want to silence warnings about builtins, you can add the list of builtins to the `externals` option; like so:
+See [`preferBuiltins`](#preferbuiltins).
 
-```js
-import resolve from '@rollup/plugin-node-resolve';
-import builtins from 'builtin-modules'
-export default ({
-  input: ...,
-  plugins: [resolve()],
-  external: builtins,
-  output: ...
-})
-```
+Use a plugin like [rollup-plugin-node-polyfills](https://github.com/ionic-team/rollup-plugin-node-polyfills) to provide stubbed versions of built-ins, and set `preferBuiltins` to `false`.
 
 ## Resolving require statements
 

packages/node-resolve/src/index.js

@@ -259,14 +259,14 @@ export function nodeResolve(opts = {}) {
 
       if (hasPackageEntry) {
         if (builtins.has(resolved) && preferBuiltins && isPreferBuiltinsSet) {
-          return null;
+          return false;
         } 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;
+          return false;
         } else if (jail && resolved.indexOf(normalize(jail.trim(sep))) !== 0) {
           return null;
         }

packages/node-resolve/src/resolveImportSpecifiers.js

@@ -160,38 +160,30 @@ async function resolveId(importPath, options, exportConditions, warn) {
 
 // Resolve module specifiers in order. Promise resolves to the first module that resolves
 // successfully, or the error that resulted from the last attempted module resolution.
-export function resolveImportSpecifiers(
+export async function resolveImportSpecifiers(
   importSpecifierList,
   resolveOptions,
   exportConditions,
   warn
 ) {
-  let promise = Promise.resolve();
-
   for (let i = 0; i < importSpecifierList.length; i++) {
-    // 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;
-      }
-
+    try {
+      // eslint-disable-next-line no-await-in-loop
       let result = await resolveId(importSpecifierList[i], resolveOptions, exportConditions, warn);
       if (!resolveOptions.preserveSymlinks) {
+        // eslint-disable-next-line no-await-in-loop
         if (await exists(result)) {
+          // eslint-disable-next-line no-await-in-loop
           result = await realpath(result);
         }
       }
       return result;
-    });
-
-    // swallow MODULE_NOT_FOUND errors
-    promise = promise.catch((error) => {
+    } catch (error) {
+      // swallow MODULE_NOT_FOUND errors
       if (error.code !== 'MODULE_NOT_FOUND') {
         throw error;
       }
-    });
+    }
   }
-
-  return promise;
+  return null;
 }

packages/node-resolve/test/prefer-builtins.js

@@ -9,7 +9,7 @@ const { nodeResolve } = require('..');
 
 process.chdir(join(__dirname, 'fixtures'));
 
-test('warns when importing builtins', async (t) => {
+test('handles importing builtins', async (t) => {
   const warnings = [];
   const bundle = await rollup({
     input: 'builtins.js',
@@ -24,8 +24,7 @@ test('warns when importing builtins', async (t) => {
 
   const { module } = await testBundle(t, bundle);
 
-  t.is(warnings.length, 1);
-  t.snapshot(warnings);
+  t.is(warnings.length, 0);
   // eslint-disable-next-line global-require
   t.is(module.exports, require('path').sep);
 });
@@ -66,8 +65,7 @@ test('true allows preferring a builtin to a local module of the same name', asyn
 
   const imports = await getImports(bundle);
 
-  t.is(warnings.length, 1);
-  t.snapshot(warnings);
+  t.is(warnings.length, 0);
   t.deepEqual(imports, ['events']);
 });
 

packages/node-resolve/test/snapshots/dedupe-custom.js.md

@@ -2,7 +2,7 @@
 
 The actual snapshot is saved in `dedupe-custom.js.snap`.
 
-Generated by [AVA](https://ava.li).
+Generated by [AVA](https://avajs.dev).
 
 ## can deduplicate custom module directory
 

packages/node-resolve/test/snapshots/dedupe.js.md

@@ -2,7 +2,7 @@
 
 The actual snapshot is saved in `dedupe.js.snap`.
 
-Generated by [AVA](https://ava.li).
+Generated by [AVA](https://avajs.dev).
 
 ## dedupes deep imports by package name if dedupe is set
 

packages/node-resolve/test/snapshots/jail.js.md

@@ -2,7 +2,7 @@
 
 The actual snapshot is saved in `jail.js.snap`.
 
-Generated by [AVA](https://ava.li).
+Generated by [AVA](https://avajs.dev).
 
 ## mark module outside the jail as external
 

packages/node-resolve/test/snapshots/only.js.md

@@ -2,7 +2,7 @@
 
 The actual snapshot is saved in `only.js.snap`.
 
-Generated by [AVA](https://ava.li).
+Generated by [AVA](https://avajs.dev).
 
 ## deprecated: regex
 
@@ -30,19 +30,19 @@ Generated by [AVA](https://ava.li).
       },
     ]
 
-## regex
+## handles nested entry modules
 
 > Snapshot 1
 
     []
 
-## specify the only packages to resolve
+## regex
 
 > Snapshot 1
 
     []
 
-## handles nested entry modules
+## specify the only packages to resolve
 
 > Snapshot 1
 

packages/node-resolve/test/snapshots/prefer-builtins.js.md

@@ -2,7 +2,7 @@
 
 The actual snapshot is saved in `prefer-builtins.js.snap`.
 
-Generated by [AVA](https://ava.li).
+Generated by [AVA](https://avajs.dev).
 
 ## false allows resolving a local module with the same name as a builtin module
 
@@ -16,33 +16,3 @@ Generated by [AVA](https://ava.li).
         toString: Function {},
       },
     ]
-
-## true allows preferring a builtin to a local module of the same name
-
-> Snapshot 1
-
-    [
-      {
-        code: 'UNRESOLVED_IMPORT',
-        importer: 'prefer-builtin.js',
-        message: '\'events\' is imported by prefer-builtin.js, but could not be resolved – treating it as an external dependency',
-        source: 'events',
-        toString: Function {},
-        url: 'https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency',
-      },
-    ]
-
-## warns when importing builtins
-
-> Snapshot 1
-
-    [
-      {
-        code: 'UNRESOLVED_IMPORT',
-        importer: 'builtins.js',
-        message: '\'path\' is imported by builtins.js, but could not be resolved – treating it as an external dependency',
-        source: 'path',
-        toString: Function {},
-        url: 'https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency',
-      },
-    ]

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

@@ -2,13 +2,7 @@
 
 The actual snapshot is saved in `root-dir.js.snap`.
 
-Generated by [AVA](https://ava.li).
-
-## deduplicated from the given root directory
-
-> Snapshot 1
-
-    'Package A React: react imported from root | package B react: react imported from root'
+Generated by [AVA](https://avajs.dev).
 
 ## deduplicates modules from the given root directory
 

packages/node-resolve/test/snapshots/test.js.md

@@ -2,7 +2,7 @@
 
 The actual snapshot is saved in `test.js.snap`.
 
-Generated by [AVA](https://ava.li).
+Generated by [AVA](https://avajs.dev).
 
 ## handles package side-effects