-
-
Notifications
You must be signed in to change notification settings - Fork 570
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): preserve moduleSideEffects when re-resolving files #1245
Conversation
e99e5bd
to
2aa3760
Compare
Thanks for putting all this work in so quickly! Certainly it would have taken me a lot longer, if ever. I'd still be interested to see if the nested resolveId behavior we discussed is intentional too (see my draft pr linked above): at the moment it seems a little off that it always defaults the last result properties: Basically at the moment adding a plugin that knows less breaks plugins that know more. I suppose it could also just forward the previous result if it's relevant in most cases. |
That is likely not possible to be fixed, at least not in the way you think. The problem is that Rollup cannot know after which resolve the module will be loaded, and what is "first" and "second" very much depends on the plugin observing it. The value of the attributes are set when the module is loaded, and it is either the attributes with which There would be yet another approach: If node-resolve also takes over loading a module and adds the attributes at that point in time. |
Thanks! |
Note also this should mostly resolve this troubleshooting section on the site: https://rollupjs.org/guide/en/#tree-shaking-doesnt-seem-to-be-working |
@lukastaegert if you're good to merge this, please do |
Awesome, was just waiting for a go as I am not code-owner here |
BREAKING CHANGES: Requires Rollup>=2.78.0
2aa3760
to
1940a13
Compare
Thanks everybody: so far it's looking really nice! I did get a weird issue with it complaining about a missing export from a commonjs module that it wasn't before, but deleting the rollup cache cleared that up, and I'm using the rollup API, so I'm probably just doing something wrong. Perhaps bumping to rollup 3 would have fixed it too... |
The Dependabot PR which tried to bump this package to the version including this change broke nearform/github-snooze-chrome-extension#144. This suggests that there is a breaking change and I'm not sure what's the way to fix it. |
Seems similar to #1258, you should track that issue |
Working on updating from 13 to 14, and encountering this error that seems to related to this change. Should I raise a new issue for this?
We're using yarn 3 w/pnp and the following versions:
|
@michaelfaith please do not cross post in multiple issues/pull requests. if you feel you have a valid bug, please open a new issue and follow the requirements of the bug template. |
Understood. I wasn't sure if that issue would still be monitored, since it was closed. Apologies. |
The first line of the Description section might be relevant to you:
But see the immediately above linked issue, it seems all together too easy to miss this, despite the fact the package manager should be checking peer deps. |
I see. The main README still shows lower version support. So I wasn't looking deeper there. Sorry for wasting you guys' time. |
Actually, that sounds like a bug, thanks for reporting it! If noone else beats me to it I'll make a PR to fix it. |
Rollup Plugin Name:
node-resolve
This PR contains:
Are tests included?
Breaking Changes?
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
Breaking change: The plugin now requires at least rollup@2.78.0 to run due to its use of the new
order
plugin hook ordering feature.This fixes a really important bug that basically rendered
moduleSideEffects
useless when used together with the commonjs plugin. The problem is that as part of its functionality,node-resolve
"re-resolves" the id in the end by callingthis.resolve
again with askipSelf
flag so that the call skips the node-resolve plugin.A feature of
skipSelf
is, that if another plugin usesthis.resolve
again in response to the call with the same source and importer, then this call is also not forwarded tonode-resolve
.This is what the
commonjs
plugin does. Then the commonjs plugin takes the result and passes it tothis.load
to inspect the code of the module before resolving. However as the result was not processed bynode-resolve
, it does not contain themoduleSideEffects
information. Whenthis.load
is called for an id, the value ofmoduleSideEffects
can no longer be changed by any otherresolveId
hook and thusmoduleSideEffects
gets ignored.Solution
node-resolve
no longer usesskipSelf
. Instead, it stores the resolve information in a custom resolve attribute and directly responds with this information when the attribute is present, thus givingcommonjs
the correctmoduleSideEffects
attribute.This fixes the bug, but it breaks some tests that rely on the
skipSelf
attribute. This is not an issue if thenode-resolve
plugin is the last plugin. To emulate this, I added anorder: "post"
attribute to theresolveId
hook, which is a new feature from rollup@2.78.0, hence the breaking change. It basically ensures the hook runs last, which makes a lot of sense for a "catch-all" resolver like node-resolve and might solve other issues as well.Alternatively, I considered fixing this in Rollup core, but allowing later
resolveId
hooks to changemoduleSideEffects
breaks other things and is not a solution.Another alternative approach could be for
node-resolve
to itself usethis.load
itself to access the mutablemoduleSideEffects
of the actual module in question. If you think this would be a better solution, then we can also go there.This is a really important bug that should have been fixed long ago, but apparently it took some time to be noticed.
cc @simonbuchan