Navigation Menu

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

Plugin context function for pre-loading modules #4234

Merged
merged 9 commits into from Nov 12, 2021
Merged

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

This is the second and final addition to the plugin infrastructure I need to be able to handle circular imports in the commonjs plugin.

This feature allows you to trigger loading a module id before it is imported so that the load, transform and moduleParsed hooks run and you can inspect the module information. A pre-loaded module will not be loaded again when it is actually imported so there should be no performance overhead from using this function. My use case is to be able to detect and analyze CommonJS files in the resolveId hook before they are imported so that we can add a proxy module even for entry points if needed.

@yyx990803, @patak-js, @antfu, @marvinhagemeister this is a rather non-trivial extension of the plugin API, which is why I really value your feedback here if this can be added on your side, see documentation below (copied from the PR) for details. One thing to note is that I decided to let the function return the moduleInfo, which usually contains an AST. The CommonJS plugin would not require it, though, all it needs is access to meta. So if you can implement it but without providing an AST, that will be entirely fine for the purposes of the CommonJS plugin as long you document that restriction on your side.

In any case, I am not going to merge this before the corresponding PR for the CommonJS plugin is not ready so both can be tried out together. But this will take some time anyway.

Documentation: this.load

Type: ({id: string, moduleSideEffects?: boolean | 'no-treeshake' | null, syntheticNamedExports?: boolean | string | null, meta?: {[plugin: string]: any} | null}) => Promise<ModuleInfo>

Loads and parses the module corresponding to the given id, attaching additional meta information to the module if provided. This will trigger the same load, transform and moduleParsed hooks that would be triggered if the module were imported by another module.

This allows you to inspect the final content of modules before deciding how to resolve them in the resolveId hook and e.g. resolve to a proxy module instead. If the module becomes part of the graph later, there is no additional overhead from using this context function as the module will not be parsed again. The signature allows you to directly pass the return value of this.resolve to this function as long as it is neither null nor external.

Note that with regard to the moduleSideEffects, syntheticNamedExports and meta options, the same restrictions apply as for the resolveId hook: Their values only have an effect if the module has not been loaded yet. Thus, it is very important to use this.resolve first to find out if any plugins want to set special values for these options in their resolveId hook, and pass these options on to this.load if appropriate. The example below showcases how this can be handled to add a proxy module for modules containing a special code comment:

export default function addProxyPlugin() {
  return {
    async resolveId(source, importer, options) {
      // We make sure to pass on any resolveId options to this.resolve to get the module id
      const resolution = await this.resolve(source, importer, { skipSelf: true, ...options });
      // We can only pre-load existing and non-external ids
      if (!resolution?.external) {
        const moduleInfo = await this.load(resolution);
        if (moduleInfo.code.indexOf('/* use proxy */') >= 0) {
          return `${resolution.id}?proxy`;
        }
      }
      // As we already fully resolved the module, there is no reason to resolve it again
      return resolution;
    },
    load(id) {
      if (id.endsWith('?proxy')) {
        const importee = id.slice(0, -'?proxy'.length);
        return `console.log('proxy!); export * from '${importee}';`;
      }
      return null;
    }
  };
}

If the module was already loaded, this will just wait for the parsing to complete and then return its module information. If the module was not yet imported by another module, this will not automatically trigger loading other modules imported by this module. Instead, static and dynamic dependencies will only be loaded once this module has actually been imported at least once.

Be aware that you cannot await calling this.load for a module during that module's own load or transform hook as that would essentially wait for those hooks to complete first and lead to a deadlock.

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Thank you for your contribution! ❤️

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

npm install rollup/rollup#load-module

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

@lukastaegert lukastaegert changed the base branch from master to resolve-id-is-entry October 1, 2021 04:27
@codecov
Copy link

codecov bot commented Oct 1, 2021

Codecov Report

Merging #4234 (1c8f408) into master (2810269) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 1c8f408 differs from pull request most recent head 8b67cb2. Consider uploading reports for the commit 8b67cb2 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4234   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         204      204           
  Lines        7289     7306   +17     
  Branches     2081     2084    +3     
=======================================
+ Hits         7172     7189   +17     
  Misses         58       58           
  Partials       59       59           
Impacted Files Coverage Δ
src/Module.ts 100.00% <100.00%> (ø)
src/ModuleLoader.ts 100.00% <100.00%> (ø)
src/utils/PluginContext.ts 100.00% <100.00%> (ø)
src/utils/sanitizeFileName.ts 66.66% <0.00%> (-8.34%) ⬇️

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 2d85884...8b67cb2. Read the comment docs.

Base automatically changed from resolve-id-is-entry to master October 1, 2021 04:43
@marvinhagemeister
Copy link

I like this feature a lot! It's something I ran into in WMR a few weeks back and hacked around via a custom abstraction. So a big +1 from me 👍

@patak-dev
Copy link
Contributor

We discussed this extension with the Vite team today and it looks good to us. This could be implemented in the server plugin pipeline but we aren't using the commonjs plugin during dev, so at least for Vite we could delay implementing support until there are plugins in the ecosystem using this feature that needs to run in dev. Vite doesn't implement moduleParsed during dev (see https://vitejs.dev/guide/api-plugin.html#universal-hooks), so if this is implemented it will contain partial information as you suggested.

@lukastaegert
Copy link
Member Author

Published as pre-release rollup@2.59.0-0 for testing

@lukastaegert
Copy link
Member Author

After having tested it extensively in rollup/plugins#1038, the only change I added was to not have this.load wait for its imported ids to be resolved as well. i.e. after this.load, importedIds of the moduleInfo will be empty. This helps to avoid tricky deadlock situations when there are circular dependencies and makes it safe to use and wait for this.load in resolveId hooks. I also updated the documentation to describe this as well as potential deadlock situations one may encounter when using it in a load or transform hook.

This is now ready to be released as far as I can say and I would release it some time tomorrow to unblock rollup/plugins#1038 if there are no concerns.

I published one last pre-release 2.59.0-1 with the latest changes as rollup@beta.

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

3 participants