Node-resolve: refactor handling builtins, do not log warning if no local version #637

tjenkinson · 2020-11-08T11:46:10Z

Rollup Plugin Name: `node-resolve`

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

Description

This slightly simplifies how the preferBuiltins default value is handled. Now isPreferBuiltinsSet is only used for determining when to show the log message and not in some of the other decisions.

Currently the plugin uses builtin-modules package to determine if a module is a builtin, but the resolve package uses is-core-module so there is a bit of a mismatch and room for errors here (e.g. we don't think the id is a builtin, but resolve does. I think then we would let it through even if the user set preferBuiltins: false. Or we think it's a builtin, but resolve disagrees and finds nothing, so we end up bailing out early) . I think it would be better if we could tell the resolve plugin to not resolve builtins, and then we would have the source of truth for builtins. Will open with an PR for an option for this in the resolve repo

…resolve-refactor-builtins

tjenkinson · 2020-11-08T11:58:22Z

packages/node-resolve/src/index.js

@@ -258,9 +258,20 @@ export function nodeResolve(opts = {}) {
      idToPackageInfo.set(resolved, packageInfo);

      if (hasPackageEntry) {
-        if (builtins.has(resolved) && preferBuiltins && isPreferBuiltinsSet) {


Pretty sure resolved === importee here because we were depending on resolve() agreeing on it being a builtin and resolving to the same thing (given we didn't add importSpecifierList.push(`${importee}`)). Therefore builtins.has(resolved) is the same as importeeIsBuiltin.

^ @lukastaegert

tjenkinson · 2020-11-08T12:05:01Z

Actually I'm not sure if this is going to warn for all builtins even when there isn't a local version available. Moving this back to a draft

tjenkinson · 2020-11-08T12:15:10Z

Actually I'm not sure if this is going to warn for all builtins even when there isn't a local version available. Moving this back to a draft

This wasn't handled before either, so now this includes a fix for that too. We no longer log a warning for a builtin if there was no local version.

ljharb · 2020-11-09T06:15:36Z

https://www.npmjs.com/package/browser-resolve exists, and has been used by browserify since it invented the concept of a bundler - why not use that?

shellscape · 2020-11-09T14:16:56Z

Personally, I'm not a fan of depending upon another bundler's implementation of a thing. @ljharb your comment seems better suited to a Modification ticket to discuss wholesale removal of node-resolve in favor of another solution. Since node-resolve isn't going anywhere anytime soon, let's allow this PR to proceed without tertiary discussion.

lukastaegert · 2020-11-15T06:29:23Z

why not use that?

@ljharb While we have been happily using your resolve package already, browser-resolve appears to be targeted at resolving the use of the browser field, which does not appear the core issue to me.

We had a custom logic for this for a long time and I suspect the reason is that it allows greater control over when to use the browser field, i.e. I am talking about the interaction with the mainFields option. While I think it could still be made to work with the browser-resolve package by manually patching up package files in the packageFilter option (something I think we already do for other purposes), and while this could even make the plugin simpler, it would be a major refactoring which I do not think should be part of this.

I would not completely dismiss this in general, though. Your resolve package has shown that it is very well-cacheable, giving this plugin a nice speed-boost over vanilla Rollup, so there could be some upsides from using browser-resolve as well. I do not know why it was not used originally, though, that was before my time.

But @shellscape is right, this should be another issue, but not of the magnitude of replacing node-resolve but rather changing the core-resolve from https://www.npmjs.com/package/resolve to https://www.npmjs.com/package/browser-resolve

lukastaegert

Looks good to me. I assume not merging this is blocking progress on #627?

lukastaegert · 2020-11-15T06:40:20Z

packages/node-resolve/README.md

@@ -129,7 +129,7 @@ DEPRECATED: use "resolveOnly" instead
 ### `preferBuiltins`

 Type: `Boolean`<br>
-Default: `true`
+Default: `true` (with warnings if a builtin module is used over a local version. Set to `true` to disable warning.)


lukastaegert · 2020-11-15T06:42:58Z

packages/node-resolve/src/index.js

@@ -201,7 +201,7 @@ export function nodeResolve(opts = {}) {

      const importeeIsBuiltin = builtins.has(importee);

-      if (importeeIsBuiltin && (!preferBuiltins || !isPreferBuiltinsSet)) {
+      if (importeeIsBuiltin) {
        // The `resolve` library will not resolve packages with the same


Wouldn't the logic here become superfluous by using the new option in the updated resolve package?

Yep! Am looking into that in a different branch though.

lukastaegert · 2020-11-15T06:44:17Z

packages/node-resolve/src/index.js

@@ -258,9 +258,20 @@ export function nodeResolve(opts = {}) {
      idToPackageInfo.set(resolved, packageInfo);

      if (hasPackageEntry) {
-        if (builtins.has(resolved) && preferBuiltins && isPreferBuiltinsSet) {


tjenkinson · 2020-11-15T09:20:26Z

I assume not merging this is blocking progress on #627?

Nope. I don't think there's anything left to do address there right now.

shellscape · 2020-11-28T02:35:50Z

@tjenkinson I merged #627 a little bit ago and it caused a few conflicts. Please have a look

tjenkinson · 2020-11-28T10:09:37Z

@shellscape done

tjenkinson added 2 commits November 8, 2020 11:36

refactor handling builtins

dc2deca

Merge branch 'master' of https://github.com/rollup/plugins into node-…

74f76ec

…resolve-refactor-builtins

tjenkinson commented Nov 8, 2020

View reviewed changes

remove duplicate code block

b19fb3e

tjenkinson marked this pull request as ready for review November 8, 2020 12:00

tjenkinson mentioned this pull request Nov 8, 2020

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

Merged

9 tasks

tjenkinson marked this pull request as draft November 8, 2020 12:05

do not warn when using a builtin when no local version

15302c6

tjenkinson changed the title ~~Node-resolve: refactor handling builtins~~ Node-resolve: refactor handling builtins, do not log warning if no local version Nov 8, 2020

tjenkinson marked this pull request as ready for review November 8, 2020 12:15

add not about warnings to readme

44d0625

tjenkinson mentioned this pull request Nov 8, 2020

add 'includeCoreModules' option browserify/resolve#233

Merged

shellscape added pr⁴ 🔧 fix 🔌 plugin-node-resolve labels Nov 9, 2020

This comment has been minimized.

Sign in to view

lukastaegert approved these changes Nov 15, 2020

View reviewed changes

tjenkinson mentioned this pull request Nov 21, 2020

node-resolve: simplify builtins and remove customResolveOptions #656

Merged

9 tasks

Merge branch 'master' into node-resolve-refactor-builtins

fbf8807

Merge branch 'master' into node-resolve-refactor-builtins

e818131

shellscape merged commit c30b2c0 into rollup:master Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node-resolve: refactor handling builtins, do not log warning if no local version #637

Node-resolve: refactor handling builtins, do not log warning if no local version #637

tjenkinson commented Nov 8, 2020 •

edited

tjenkinson Nov 8, 2020

shellscape Nov 9, 2020

lukastaegert Nov 15, 2020

tjenkinson commented Nov 8, 2020

tjenkinson commented Nov 8, 2020

ljharb commented Nov 9, 2020

shellscape commented Nov 9, 2020

This comment has been minimized.

lukastaegert commented Nov 15, 2020 •

edited

lukastaegert left a comment

lukastaegert Nov 15, 2020

lukastaegert Nov 15, 2020

tjenkinson Nov 15, 2020

lukastaegert Nov 15, 2020

tjenkinson commented Nov 15, 2020

shellscape commented Nov 28, 2020

tjenkinson commented Nov 28, 2020

Node-resolve: refactor handling builtins, do not log warning if no local version #637

Node-resolve: refactor handling builtins, do not log warning if no local version #637

Conversation

tjenkinson commented Nov 8, 2020 • edited

Rollup Plugin Name: node-resolve

Description

tjenkinson Nov 8, 2020

Choose a reason for hiding this comment

shellscape Nov 9, 2020

Choose a reason for hiding this comment

lukastaegert Nov 15, 2020

Choose a reason for hiding this comment

tjenkinson commented Nov 8, 2020

tjenkinson commented Nov 8, 2020

ljharb commented Nov 9, 2020

shellscape commented Nov 9, 2020

This comment has been minimized.

lukastaegert commented Nov 15, 2020 • edited

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert Nov 15, 2020

Choose a reason for hiding this comment

lukastaegert Nov 15, 2020

Choose a reason for hiding this comment

tjenkinson Nov 15, 2020

Choose a reason for hiding this comment

lukastaegert Nov 15, 2020

Choose a reason for hiding this comment

tjenkinson commented Nov 15, 2020

shellscape commented Nov 28, 2020

tjenkinson commented Nov 28, 2020

tjenkinson commented Nov 8, 2020 •

edited

Rollup Plugin Name: `node-resolve`

lukastaegert commented Nov 15, 2020 •

edited