Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from May 2, 2022

Conversation

lukastaegert
Copy link
Member

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.

// Allow other plugins to take over resolution. Rollup core will not
// change the id if it corresponds to an existing file
if (resolvedResolved.id !== resolved.id) {
return resolvedResolved;
Copy link
Member

@tjenkinson tjenkinson May 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tjenkinson tjenkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@lukastaegert lukastaegert force-pushed the node-resolve/plugin-resolution branch from fa64906 to 6a214fa Compare May 2, 2022 04:24
@lukastaegert
Copy link
Member Author

@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
Copy link
Collaborator

@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.

@shellscape shellscape merged commit 9bb05ae into master May 2, 2022
@shellscape shellscape deleted the node-resolve/plugin-resolution branch May 2, 2022 12:11
@silkfire
Copy link

When is this planned to be released?

@shellscape
Copy link
Collaborator

@silkfire
Copy link

@shellscape My mistake, thanks 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants