fix(node-resolve): forward meta-information from other plugins

[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:
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).

[tjenkinson]

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?

[tjenkinson]

🙈

[lukastaegert]

So without the this.resolve the other resolve id hooks would be called from somewhere else

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 this.resolve here because it allowed people to mark ids as external that had been resolved by node-resolve. Another solution would have been to place the other plugin always before node-resolve, but these ordering-solutions are tricky. Yet another solution is for the plugin to use the options hook to add a secondary plugin that is placed first.
But as we decided to go for this solution, respecting the meta data added by other plugins besides the external flag seemed like a small step.
The only things that are NOT respected are the id and moduleSideEffects. moduleSideEffects is necessary, because node-resolve determines this value and we have no way to figure out if other plugins respected that value or not. The reason we also ignore id changes was linked for me: If I change the id, moduleSideEffects may be completely wrong as it was based on the position of the original module in the file system.

[tjenkinson]

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!

[tjenkinson]

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?

[lukastaegert]

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.

[tjenkinson]

I mean

Suggested change

	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?

[lukastaegert]

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.

[lukastaegert]

Done, please have another look

[tjenkinson]

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

[lukastaegert]

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?

[shellscape]

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

[tjenkinson]

Lgtm