feat(node-resolve): dedupe preferBuiltins protocol

[bluwy]

Rollup Plugin Name: node-resolve

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

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

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

When bundling a library that uses both node:fs and fs, it can be deduped as a single node:fs or fs import instead, slightly reducing the bundle size. This can often happen when using third-party libraries.

To enable this optimization, preferBuiltins now also accepts prefer-protocol or prefer-no-protocol to dedupe to either node:fs or fs, depending on the users node target version that supports it.

[tjenkinson]

Thanks for the PR!

I’m wondering if this really should be part of this plug-in though or a separate plug-in?

Also I reckon one of the reasons for the new prefix was to prevent name collisions between node built-ins and real packages, so I’m not sure we should provide an option to go in the direction from prefix to no prefix?

[bluwy]

I’m wondering if this really should be part of this plug-in though or a separate plug-in?

I thought about it too, but if it's a separate plugin, the only place it'll be useful are in projects that externalize all deps and builtin modules (bundle source code only). But in that case, the source code would've been written with only node: or not in the first place, not needing the plugin.

Deduping protocols would mostly be handy when bundling third-party packages which node-resolve handles, so it may be nice to have this as a first-class feature.

Also I reckon one of the reasons for the new prefix was to prevent name collisions between node built-ins and real packages, so I’m not sure we should provide an option to go in the direction from prefix to no prefix?

Good point. It would a problem if the project/package used node:url and the url package at the same time, but that sounds like an edge case, and they should be avoiding both prefer-no-protocol and prefer-protocol (as both is destructive). But for most projects I think it would be a handy addition.

[tjenkinson]

Hmm. Yeh going either way is potentially destructive.

I’m still not sure it needs to be in this plug-in vs another one. I get that the prefix is a node thing which is why it could fall in here, but also it could easily be an isolated plug-in. Will see what other maintainers think

[tjenkinson]

A bit related, we should probably always have this plug-in mark node:* as external, regardless of the preferBuiltins option

[bluwy]

A bit related, we should probably always have this plug-in mark node:* as external, regardless of the preferBuiltins option

It looks like it's already handled as node:* resolves to null when calling resolveImportSpecifiers, so it won't match a local package that would trigger a warning.

[shellscape]

I’m still not sure it needs to be in this plug-in vs another one. I get that the prefix is a node thing which is why it could fall in here, but also it could easily be an isolated plug-in.

I agree with this

[bluwy]

I'll make this as a separate plugin then, thanks for the feedback!

[shellscape]

Thanks. Please make sure to add it to rollup/awesome so others can discover it