fix(node-resolve): Respect if other plugins resolve the resolution to a different id

[lukastaegert]

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:
Solves an issue mentioned in #1169 (comment)

Description

Currently, node-resolve is very eager to take over resolution meaning that any resolver placed after the node-resolve plugin may at most resolve ids starting with \0. That is mostly fine except when plugins really want to do some proxying as mentioned later in #1169.

This modifies the logic so that when node-resolve "re-resolves" the resolved id, it not only checks if a plugin marks the id as external, but also checks if a plugin would change the id. That is something that rollup core should never do if the file exists and has an extension.

In that case, node-resolve returns the different resolution, also not messing with e.g. moduleSideEffects as now the resolution effectively refers to a different file.

That should allow plugins to replace or proxy any file even when they are placed after node-resolve.

[tjenkinson]

Could we always just return this? Would it ever be different to the line below (when the id is the same)?

[lukastaegert]

Yes, it would. The most important difference is the moduleSideEffects flag that would be overridden. You will immediately see all tests with regard to sideEffects fail. While I also considered merging in the value of this flag, I think it makes more sense that way: If the id does not change, then we are still talking about the same file and since we cannot find out if a plugin or node-resolve did the resolving, we assume node-resolve is the source of truth (but still merge in meta because node-resolve does not set meta information).
If the id changes, then it is a different file and we are sure if was another plugin interfering. Therefore, we assume the other plugin is right and our side effect information may also be wrong as it referred to the original file.

[tjenkinson]

Ah right. This sounds like something that might make sense in rollup core? So a plug-in could return a result and then have that resolved result run though all the other plugins, and do the necessary merging of the first plug-in result to a later plug-in result (I.e rules like if the first plug-in says there are side effects and a future one doesn’t set the side effects flag use the result from the first one)

Or if not in the core maybe a helper function that could wrap this hook? E.g something like withDelegationToOtherPlugins(resolveId)?

[lukastaegert]

How would you decide what the correct logic is if you do not know what the plugins actually want to do? Using the id to decide whether to use the this.resolve result over the own result IS very specific to node-resolve IMO because for node-resolve, ids are tied to actual files on the disk via package.json. For other plugins, the decision about side effects may be tied to very different criteria like extension, filters, meta data, content inspection, who knows.
I do not feel good about moving it to core. And for a shared helper, we would first need at least one other plugin that does what node-resolve does. I think node-resolve is special as it is a plugin that tries to resolve nearly everything while most other plugins just resolve some special ids.

[tjenkinson]

How would you decide what the correct logic is if you do not know what the plugins actually want to do?

I was wondering if there was a way it could be generic but maybe that doesn’t make sense

Was thinking if a later plug-in didn’t explicitly return a value for a property then that could signal to use the value from the initial one

I was wondering if the id check wouldn’t actually be needed then too

If a later plug-in did change the id but not provide side effect info then maybe using the side effect result from the first plug-in could not work well

[lukastaegert]

If a later plug-in did change the id but not provide side effect info then maybe using the side effect result from the first plug-in could not work well

Exactly my thinking considering side effects in node-resolve are tied to files on the disk.

[tjenkinson]

Lgtm

[lukastaegert]

@shellscape It seems that pnpm@7 is the new latest version now, which breaks several pipelines that just blindly npm install -g pnpm. To fix pipelines in this PR, I manually replaced that with npm install -g pnpm@6, but a better solution would certainly be to fix this in a central location, either by pinning to v6 for now as I did or make the pipelines compatible.

[shellscape]

@lukastaegert yeah that dropped over the weekend and there are some breaking changes in how things are filtered. I use pnpm for all of my work-work pipelines and workflows too and we're seeing the same there - I have some work to do today :/

That's a fine solution for now.

[silkfire]

When is this planned to be released?

[shellscape]

@silkfire https://github.com/rollup/plugins/blob/master/packages/node-resolve/CHANGELOG.md

[silkfire]

@shellscape My mistake, thanks 😅