Improving the plugin API

[marvinhagemeister]

The current plugin API has served us well as a start, but it's not without its problems. Let's look at the common pitfalls with the current API:

1. Every plugin must be aware of prefixes, \0 and \b in paths. Just testing for an extension /\.js$/ is not enough. This breaks people's assumption about file paths. Examples of internal plugins where we need additional null byte checks:
   • aliases
   • htm
   • resolve-extension
   • sass
   • sucrase
   • unpkg
2. Plugins can't run before WMR's internal ones
3. Plugins can modify internal options directly. Can open a can of worms, see webpack plugins.

1) Path handling

I feel like 1) can be solved by reversing the relationship between our plugin container and plugins. Instead of putting the onus on each plugin to specify what it can't work with, it should specify what it can work with.

Current Plugin API:

{
  name: "foo",
  resolveId(id) {
    // Check for internal prefixes
    if (id.startsWith('\0') || id.startsWith('\b')) return;
    // Check if we can deal with that file type
    if (!/\.js$/.test(id)) return;
    // Wait, we need to check for potential custom prefixes too
    if (id.includes(':')) return
    // ...now we can actually start our work
  }
}

We could in theory eliminate the first \0 or \b check if we'd replace that with an internal: prefix. This eliminates two edge cases at once.

{
  name: "foo",
  resolveId(id) {
    // Check if we can deal with that file type
    if (!/\.js$/.test(id)) return;
    // Check for potential prefixes
    if (id.includes(':')) return;
    // ...now we can actually start our work
  }
}

The above is a good improvement and we could even export a nice helper to abstract prefix handling away:

{
  name: "foo",
  resolveId(id) {
    if (!matchId({ filter: /\.js$/ }, id)) return;
    // ...now we can start our work
  }
}

If we want to match for a particular prefix, we'd need to pass an additional property:

{
  name: "foo",
  resolveId(id) {
    // Only matches if both conditions are met.
    // For example: `url:foo/bar.js`
    if (!matchId({ filter: /\.js$/, prefix: 'url' }, id)) {
      return;
    }
    // ...now we can start our work
  }
}

The above is great step forward, but it still requires the user to be aware of prefixes and remember to add the safeguard themselves. They can still shoot themselves in the foot by forgetting that or opting to manually test against id.

Interestingly, esbuild has removed the chance of error by favoring a callback-style API:

plugin.onResolve({ filter: /\.js$/ }, (args) => {
  // ...
});

A benefit of the callback-style approach is that it is a way to get rid of the implicit this in our current plugins.

Current API:

function myPlugin() {
  return {
    name: "foo",
    async resolveId(id) {
      // Where is `this.resolve` coming from???
      const resolved = await this.resolve(id);
      return resolved.id;
    },
  };
}

vs callback-style API:

function myPlugin() {
  const plugin = createPlugin({ name: "foo" });

  plugin.onResolve({ filter: /.*/ }, async (args) => {
    // Obvious where `args.resolve` is coming from
    const resolved = await args.resolve(args.id);
    return resolved.id;
  });
}

// Or something in-between
function myPlugin() {
  return {
    name: "foo",
    setup(build) {
      build.onResolve({ filter: /.*/ }, async (args) => {
        // Obvious where `args.resolve` is coming from
        const resolved = await args.resolve(args.id);
        return resolved.id;
      });
    }
  }
}

It's definitely more to type, but would make it more explicit which functions are available and where they are coming from.

2) Plugin ordering

All plugins are currently called after built-in ones have run. This poses problems when a user plugin needs to intercept resolution before that or transform JSX before WMR has processed it. The current control flow roughly looks like this:

WMR Plugins
    ↓
User Plugins

Similar to vite and webpack we can add additional phases where plugins can run.

User Plugin Pre-Phase
    ↓
WMR Plugins
    ↓
User Plugins (Default Phase)
    ↓
User Plugins Post-Phase

To specify during which phase a plugin should run, we can add an enforce property, similar to vite.

{
  name: "my-plugin",
  enforce: 'pre' // 'pre' | 'normal' | 'post' (Default: 'normal')
};

Seeing that both the webpack and vite ecosystem don't have known issues with plugin ordering that I'm aware of this seems to be a good solution.

3) Config hooks instead of mutating options

The third issue of users being able to mutate options and somewhat internal state directly can be solved by adding an addition config() hook. The return value of that will be deeply merged into the existing config.

{
  name: "foo",
  config() {
    return {
      features: { preact: true }
    }
  }
}

To avoid a tree-like structure of plugins, returning plugins from config() is disallowed.

{
  name: "foo",
  config() {
    return {
      // Will throw
      plugins: [otherPlugin()]
    }
  }
}

Instead, multiple plugins can return an array of plugins to allow for preset-style plugins. The returned array will be flattened into options.plugins internally:

function PreactPreset() {
  return [prefresh(), devtools()]
}

Related issues

• Userland plugin doesn't work if it uses the the same file extension as a built-in plugin, with a different import prefix #449 Plugin ordering

Related PRs

• Improve plugin API #438

[marvinhagemeister]

Chatted a bit with @matias-capeletto from vite today and one of their learnings is that they should've made the default plugin phase to run before vite's plugins. The main reason for that is that users most commonly want to intercept stuff at that time. So it seems like the majority of plugins is using enforce: pre.

EDIT: Relevant issue for that on the vite issue tracker: vitejs/vite#1815

Another thought that came up is that prefixes could be converted to search paramters. Search paramters are valid by default in URLs and thereby making them a valid import specifier. This is another alternative of doing prefix we should think about.

// with prefix
import foo from "url:./bar/image.jpg";

// vs with search param
import foo from "./bar/image.jpg?url";

[patak-dev]

About the need of adding enforce: pre, only two of the official plugins need that: https://vite-rollup-plugins.patak.dev/, so it is not the majority, but I think it is nice to have.

It may be a bit late to modify this in Vite now though, as it would be a breaking change, we discussed that in the linked issue. In Vite we can not place the user plugins directly at the start, as some core plugins are needed first. I was proposing that they will go before the feature core plugins (like json support). Evan pointed out issues with this idea though vitejs/vite#1815 (comment). I don't know if there is a clear strategy without having extra options in rollup hooks (like being able to bail out in non-first hooks)

[piotr-cz]

As for Plugin ordering: to set the enforce mode for Rollup plugin, I'd suggest passing it as an argument.
The reason is to separate plugin from build configuration and keep possiblity to use existing Rollup plugins.

Something like this:

config.pushPlugin(someRollupPlugin(), 'pre')

[marvinhagemeister]

@piotr-cz With #438 we're moving away from plugins manually pushing stuff into wmr's internal state. Instead, the return value of plugins will be deeply merged into our state object. Before wmr does its job we currently presort the plugins array based on the enforce property. If a plugin doesn't have it, we'll treat it as enforce: "normal". In other words rollup plugins will continue to work as usual. No breakage there.

I'm unsure how the proposed pushPlugin() method would improve on that. But maybe I'm lacking a bit of context. Can you elaborate on your thought process on pushPlugin()?

[patak-dev]

Rollup Plugins can still be used when enforce or apply is needed in Vite. This is the pattern used in Vite

import eslint from "@rollup/plugin-eslint"
          
export default {
  plugins: [
    {
      ...eslint({ include: '**/*.+(vue|js|jsx|ts|tsx)' }),
      enforce: 'pre',
      apply: 'build',
    },
  ]
}

[piotr-cz]

@marvinhagemeister
Thanks, I missed that PR - I think the README.md could be updated to reflect new updates in API.

@matias-capeletto
Thanks, I forgot that the return from rollup plugin is just plain object.

This answers my concern