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

Implement new APIs for inter-plugin communication #3807

Merged
merged 20 commits into from Oct 8, 2020

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Oct 2, 2020

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:
Resolves #2662

Description

This is in response to various issues with the node-resolve and commonjs plugin and will provide plugins with a way to pass additional information to resolve as well as a way to annotate modules with custom meta-informaiton. See the attached documentation update, for reference:

Inter-plugin communication

At some point when using many dedicated plugins, there may be the need for unrelated plugins to be able to exchange information during the build. There are several mechanisms through which Rollup makes this possible.

Custom resolver options

Assume you have a plugin that should resolve a module to different ids depending on how it is imported. One way to achieve this would be to use special proxy ids when importing this module, e.g. a transpiled import via require("foo") could be denoted with an id foo?require=true so that a resolver plugin knows this.

The problem here, however, is that this proxy id may or may not cause unintended side-effects when passed to other resolvers. Moreover, if the id is created by plugin A and the resolution happens in plugin B, it creates a dependency between these plugins so that one A is not usable without B.

Custom resolver option offer a solution here by allowing to pass additional options for plugins when manually resolving a module. This happens without changing the id and thus without impairing the ability for other plugins to resolve the module correctly if the intended target plugin is not present.

function requestingPlugin() {
  return {
    name: 'requesting',
    async buildStart() {
      const resolution = await this.resolve('foo', undefined, {
        custom: {resolving: {specialResolution: true}}
      });
      console.log(resolution.id); // "special"
    }
  }
}

function resolvingPlugin() {
  return {
    name: 'resolving',
    resolveId(id, importer, { custom }) {
      if (custom.resolving?.specialResolution) {
        return 'special';
      }
      return null;
    }
  }
}

Note the convention that custom options should be added using a property corresponding to the plugin name of the resolving plugin. It is responsibility of the resolving plugin to specify which options it respects.

Custom module meta-data

Plugins can annotate modules with custom meta-data which can be accessed by themselves and other plugins via the resolveId, load, and transform hooks. This meta-data should always be JSON.stringifyable and will be persisted in the cache e.g. in watch mode.

function annotatingPlugin() {
  return {
    name: 'annotating',
    transform(code, id) {
      if (thisModuleIsSpecial(code, id)) {
        return {meta: {annotating: {special: true}}}
      }
    }
  }
}

function readingPlugin() {
  let parentApi;
  return {
    name: 'reading',
    buildEnd() {
      const specialModules = Array.from(this.getModuleIds())
        .filter(id => this.getModuleInfo(id).meta.annotating?.special);
      // do something with this list
    }
  }
}

Note the convention that plugins that add or modify data should use a property corresponding to the plugin name, in this case annotating. On the other hand, any plugin can read all meta-data from other plugins via this.getModuleInfo.

If several plugins add meta-data or meta-data is added in different hooks, then these meta objects will be merged shallowly. That means if plugin first adds {meta: {first: {resolved: "first"}}} in the resolveId hook and {meta: {first: {loaded: "first"}}} in the load hook wile plugin second adds {meta: {second: {transformed: "second"}}} in the transform hook, then the resulting meta object will be {first: {loaded: "first"}, second: {transformed: "second"}}. Here the result of the resolveId hook will be overwritten by the result of the load hook as the plugin was both storing them under its first top-level property. The transform data of the other plugin on the other hand will be placed next to it.

Direct plugin communication

For any other kind of inter-plugin communication, we recommend the pattern below. Note that api will never conflict with any upcoming plugin hooks.

function parentPlugin() {
  return {
    name: 'parent',
    api: {
      //...methods and properties exposed for other plugins
      doSomething(...args) {
        // do something interesting
      }
    }
    // ...plugin hooks
  }
}

function dependentPlugin() {
  let parentApi;
  return {
    name: 'dependent',
    buildStart({ plugins }) {
      const parentName = 'parent';
      const parentPlugin = options.plugins
        .find(plugin => plugin.name === parentName);
      if (!parentPlugin) {
        // or handle this silently if it is optional
        throw new Error(`This plugin depends on the "${parentName}" plugin.`);
      }
      // now you can access the API methods in subsequent hooks
      parentApi = parentPlugin.api;
    }
    transform(code, id) {
      if (thereIsAReasonToDoSomething(id)) {
        parentApi.doSomething(id);
      }
    }
  }
}

@rollup-bot
Copy link
Collaborator

rollup-bot commented Oct 2, 2020

Thank you for your contribution! ❤️

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

npm install rollup/rollup#resolve-id-params

or load it into the REPL:
https://rollupjs.org/repl/?circleci=13149

@tivac
Copy link
Contributor

tivac commented Oct 2, 2020

Excited to see this so there's finally a blessed and much-safer approach to solving #2662!

@lukastaegert
Copy link
Member Author

So can I mark this as closing #2662?

@lukastaegert
Copy link
Member Author

One hope I have is to use this in the commonjs plugin for two things:

  • Pass the kind of import to the node-resolve plugin so that exports based resolution can correctly use either the require or the import condition in the upcoming exports handling feat(node-resolve): support package entry points plugins#540
  • Use module meta data to store which module is commonjs to make it available to downstream consumers such as Vite (cc @underfin) but also to finally make this plugin cacheable via disk cache. I will try to compile a PR for this to see if this actually works.

@lukastaegert
Copy link
Member Author

BTW if someone has suggestions for how to improve documentation, this is more than welcome. This PR includes a HUGE docs update that would really benefit from some reviewing.

@codecov
Copy link

codecov bot commented Oct 3, 2020

Codecov Report

Merging #3807 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3807      +/-   ##
==========================================
+ Coverage   97.03%   97.05%   +0.01%     
==========================================
  Files         185      185              
  Lines        6477     6482       +5     
  Branches     1876     1877       +1     
==========================================
+ Hits         6285     6291       +6     
  Misses        101      101              
+ Partials       91       90       -1     
Impacted Files Coverage Δ
src/utils/options/normalizeInputOptions.ts 100.00% <ø> (ø)
src/ExternalModule.ts 100.00% <100.00%> (ø)
src/Graph.ts 100.00% <100.00%> (ø)
src/Module.ts 100.00% <100.00%> (ø)
src/ModuleLoader.ts 100.00% <100.00%> (ø)
src/utils/PluginContext.ts 100.00% <100.00%> (ø)
src/utils/blank.ts 100.00% <100.00%> (ø)
src/utils/error.ts 100.00% <100.00%> (ø)
src/utils/resolveId.ts 94.73% <100.00%> (ø)
src/utils/transform.ts 100.00% <100.00%> (+1.56%) ⬆️

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 74e5081...97fffab. Read the comment docs.

}
```

When triggering this hook from a plugin via [`this.resolve(source, importer, options)`](guide/en/#thisresolvesource-string-importer-string-options-skipself-boolean--promiseid-string-external-boolean--null), it is possible to pass a custom options object to this hook. While this object will be passed unmodified, plugins should follow the convention of adding a `custom` property with an object where the keys correspond to the names of the plugins that the options are intended for. For details see **TODO: Section about custom parameter passing**.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we show an example of how this custom property would look like?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would place this section more to the bottom, so that first the hook return values are explained.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we show an example of how this custom property would look like

With the proper link here, maybe the example is not needed?

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is some top quality API work, great to see this finally!

The only thing worth thinking about a little might be how this affects caching. Ideally we should encourage that the metadata is cached by default in the cache model, I didn't check if that is handled here or not though.

@lukastaegert
Copy link
Member Author

lukastaegert commented Oct 6, 2020

Thanks! Caching is indeed an important point, otherwise we might run into trouble once transform hooks are skipped that add essential meta data. Therefore, the metadata is properly cached and restored, which is also tested.

I was hoping to utilize this to make the commonjs plugin cacheable at last, but as it turns out, this will still require another hook that is run for each module, cached or not, once Rollup has finished parsing them and all meta data is in their final state. I am actually working on this in another upcoming PR because I think this might be useful to others as well.

@lukastaegert lukastaegert merged commit 1ad8289 into master Oct 8, 2020
@lukastaegert lukastaegert deleted the resolve-id-params branch October 8, 2020 04:07
@kzc
Copy link
Contributor

kzc commented Dec 3, 2020

@lukastaegert I was trying to add an unrelated new feature to latest master but found that npm test in rollup is now a hundred times slower. Because of this npm test not usable on my machine - macos 10.9.5 with 8GB RAM and SSD. I tested with both node 12 and node 14. The node version doesn't seem to matter. Bisected the testing slowness to 1ad8289. I don't know whether rollup itself is slower in general or just the testing. Any ideas?

@lukastaegert
Copy link
Member Author

Not really, because for me, I do no see a change (also a Mac, though with macos 10.15). I remember seeing something like this once when my battery was nearly dead. Could not reproduce ever since, though. If this is reproducible for you, it would be interesting to gain more insights. Like attaching a browser for debugging (node --inspect-brk node_modules/.bin/mocha) and profiling where the time is spent. Maybe memory consumption also changed? I am sorry I cannot help more here.

@kzc
Copy link
Contributor

kzc commented Dec 3, 2020

Battery is not an issue. The commit immediately preceding this one is fast. All subsequent commits are slow.

@kzc
Copy link
Contributor

kzc commented Dec 3, 2020

It was a stale node_modules issue. Once reinstalled the speed is fast once again. This commit must have exercised a perf bug in a dependency for the first time that was subsequently fixed.

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.

Plugin message passing/data-sharing
6 participants