fix(node-resolve): forward meta-information from other plugins#1062
fix(node-resolve): forward meta-information from other plugins#1062shellscape merged 1 commit intomasterfrom
Conversation
tjenkinson
left a comment
There was a problem hiding this comment.
Is the reason this fix is needed because the resolve id hooks are only ever called once?
So without the this.resolve the other resolve id hooks would be called from somewhere else, where the returned metadata was respected there?
packages/node-resolve/test/test.js
Outdated
| ]); | ||
| }); | ||
|
|
||
| test.only('passes on custom options', async (t) => { |
Actually, they would not be called at all for plugins that are sorted after the node-resolve plugin, because, as you stated, resolution only happens once. We needed to add |
tjenkinson
left a comment
There was a problem hiding this comment.
thanks for the explanation. I think you might have missed overriding the id based on what you said?
I'm not sure what the best answer is to my other comment. Not defaulting to passing all props through feels safer and more controlled, but maybe it's fine. You have a much better understanding of the API than I do :)
LGTM!
packages/node-resolve/src/index.js
Outdated
| return false; | ||
| } | ||
| // Pass on meta information added by other plugins | ||
| return { ...resolvedResolved, moduleSideEffects: resolved.moduleSideEffects }; |
There was a problem hiding this comment.
given what you said before
The only things that are NOT respected are the id and moduleSideEffects
shouldn't you be forcing resolved.id as the id here too?
There was a problem hiding this comment.
No, that is what I was trying to say:
- node-resolves figures out that /foo/foo.js has no side-effects based on some package.json file
- another plugin changes the id to /bar.js, which is completely different.
Then our value for moduleSideEffects is probably wrong or irrelevant.
That is why I thought to prohibit changing the id.
On the other hand, one could also check if the id has changed and only override the moduleSideEffects value if it is still the same id.
There was a problem hiding this comment.
I mean
| return { ...resolvedResolved, moduleSideEffects: resolved.moduleSideEffects }; | |
| return { ...resolvedResolved, id: resolved.id, moduleSideEffects: resolved.moduleSideEffects }; |
?
right now a change to bar.js would make it though and it shouldn’t?
There was a problem hiding this comment.
Ah, then you are indeed right, should read my own code ^^
I will change it to just pick the meta property, that is probably best for now.
There was a problem hiding this comment.
Done, please have another look
| if (resolvedResolved.external) { | ||
| return false; | ||
| } | ||
| // Pass on meta information added by other plugins |
There was a problem hiding this comment.
I wonder if we should really just target property holding the metadata instead of allowing everything through by default except moduleSideEffects? Don't think we'd be doing any object merging then
There was a problem hiding this comment.
Admittedly, the last remaining property is syntheticNamedExports. Which we may want to keep IF we also keep the id. So maybe a better solution is: Keep everything from the second resolve, but override moduleSideEffects if the id did not change?
|
@lukastaegert @tjenkinson please give me a heads up on when you both are satisfied with the state of conversation, so I know when to merge this one. |
78e0f57 to
97a2883
Compare
Rollup Plugin Name:
node-resolveThis 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:
Resolves #1060
Description
While node-resolve was already calling other resolvers to allow them to mark modules as external, meta-information was ignored so far. This fix will make sure that all meta information added by other plugins is preserved (except moduleSideEffects that is).