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

Add importedIdResolutions and dynamicallyImportedIdResolutions to moduleInfo #4354

Merged
merged 1 commit into from Jan 21, 2022

Conversation

lukastaegert
Copy link
Member

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

List any relevant issue numbers:

Description

I am currently working on stream-lining the process of scanning dependency trees via this.load and the use of this.load in general. One problem I stumbled upon is this small detail that is easy to get wrong:

  • If you pass meta (or syntheticNamedExports or moduleSideEffects) to this.load, then it will be ignored UNLESS this is the first time we try to load this module.

The reason these flags exist on this.load in the first place, and why they are ignored for subsequent calls, is that they simulate the resolveId hook. This hooks can attach meta information to a module, but this will only happen on first resolution. This is important e.g. for the node-resolve plugin.

Ignoring subsequent calls makes sure that information from a load or transform hook is not accidentally overwritten by information from a resolveId hook, or information passed via this.load. I.e. meta information is always attached in this order:

  • resolveId hook or this.load call
  • load hook
  • transform hooks in order of plugins

However, that means you have to get these flags right when loading a module via this.load that may not have been loaded another way.

When scanning a dependency tree by looking at the importedIds on moduleInfo, you currently only find out the imported ids, but there is no way of figuring out the additional meta information (you can try running this.resolve, but as you do not know exactly how the imports were created, you will have a hard time getting it right when e.g. the commonjs plugin is involved).

This PR fixes this by not only exposing the imported ids, but also the resolution objects created by the resolveId hooks so that those can be passed to this.load.

There are two new properties on ModuleInfo (that Rollup implements as getters):

  • importedIdResolutions: ResolvedId[]
  • dynamicallyImportedIdResolutions: ResolvedId[]

The latter corresponds to dynamic imports, but only those that can be statically resolved (others are missing entirely).

@yyx990803, @patak-dev, @antfu, @marvinhagemeister also looking forward to your comments, I hope this would be easy to implement in Vite and WMR. There will be another two PRs shortly to improve other aspects of the use of this.load, though.

@github-actions
Copy link

github-actions bot commented Jan 18, 2022

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#imported-id-resolutions

or load it into the REPL:
https://rollupjs.org/repl/?pr=4354

@codecov
Copy link

codecov bot commented Jan 18, 2022

Codecov Report

Merging #4354 (c917565) into master (2256dcd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4354   +/-   ##
=======================================
  Coverage   98.68%   98.69%           
=======================================
  Files         205      205           
  Lines        7328     7331    +3     
  Branches     2084     2084           
=======================================
+ Hits         7232     7235    +3     
  Misses         36       36           
  Partials       60       60           
Impacted Files Coverage Δ
src/ExternalModule.ts 100.00% <ø> (ø)
src/Module.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2256dcd...c917565. Read the comment docs.

@lukastaegert lukastaegert merged commit 61adedf into master Jan 21, 2022
@lukastaegert lukastaegert deleted the imported-id-resolutions branch January 21, 2022 05:57
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

1 participant