feat(plugin-react): check for api.reactBabel on other plugins

[aleclarson]

⚠️ The original description no longer matches this PR's implementation.
See the discussion below and this comment for more details.

Description

Allow Vite plugins to define their own Babel presets/plugins.

// TypeScript modules need this import for the extended Plugin interface.
import "@vitejs/plugin-react"
import * as vite from "vite"

export const myVitePlugin: vite.Plugin = {
  // The `enforce` property is respected. When it equals "pre", the Babel plugins 
  // and presets will be applied *before* user-defined plugins/presets.
  enforce: "pre",
  babel: {
    // Inline plugins and presets are possible.
    plugins: [{ visitor: {...} }],
    // Vite plugins must use `require.resolve` when passing a file path for a preset 
    // or plugin, or else it can't be found by Babel. Another option is to import the
    // preset and pass its module manually.
    presets: [require.resolve('babel-preset-env')],
  }
}

Additional context

This PR is needed for vite-react-css to work.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[aleclarson]

Not sure why CI fails. I tested locally (with updated lockfile) and it builds fine.

[bluwy]

What about the api pattern instead? vite-plugin-svelte used to define a custom property like babel too but changed to api to follow Rollup's recommendation.

[Shinigami92]

[...] with updated lockfile [...]

Maybe this is exactly the point.
But I updated some deps already, maybe a rebase is worth it?

[aleclarson]

@bluwy I don't see Rollup ever adding a babel property to its Plugin interface, so maybe this is fine?

maybe a rebase is worth it?

I already tried that :\

[patak-dev]

This would be a new concept, and it wont work without the react plugin. I also think that we should use api if the dx is similar

If not, this should go in vite config, and modified by plugins in the config hook

[aleclarson]

If not, this should go in vite config, and modified by plugins in the config hook

Would that allow the end user to configure Babel through config.babel rather than reactPlugin({ babel })?

Probably want to avoid having two ways for the end user to configure Babel.

I also think that we should use api if the dx is similar

It's a clunkier solution, imo.

Editable plugins

Here's an idea... We could add "editable plugins" 👇

export default function viteReact(options) {
  const babelOptions = options.babel || {}
  babelOptions.plugins ??= []

  return {
    name: 'vite:react-babel',
    // Objects added to `editable` may be edited by other plugins.
    editable: {
      babel: babelOptions,
    }
  }
}

// Other plugins define an `editPlugins` hook…
editPlugins(edit) {
  // This warns if `vite:react-babel` does not exist
  edit('vite:react-babel', (state) => {
    // Accessing `state.babel` will throw if never defined 
    // in `editable` of the targeted plugin.
    state.babel.plugins.push(require.resolve('@foo/babel-plugin'))

    // Or you can return an object to be merged recursively.
    return { babel: { plugins: ['@foo/babel-plugin']} }
  })
}

[bluwy]

I think api would also work fine though. Here's what I have in mind:

Code 1

export default function viteReact(options) {
  const babelOptions = options.babel || {}
  babelOptions.plugins ??= []

  return {
    name: 'vite:react-babel',
    api: {
      updateBabel(fn) {
        fn(babelOptions)
      }
    }
  }
}

function viteReactCss() {
  return {
    name: 'vite-react-css',
    config(c) {
      const viteReactApi = c.plugins.find(plugin => plugin.name === 'vite:react')?.api
      viteReactApi?.updateBabel((babel) => {
        babel.plugins.push(require.resolve('@foo/babel-plugin'))
      })
    }
  }
}

or we can invert the responsibility:

Code 2

export default function viteReact(options) {
  const babelOptions = options.babel || {}
  babelOptions.plugins ??= []

  return {
    name: 'vite:react-babel',
    configResolved(c) {
      c.plugins.forEach(plugin => {
        plugin.api?.reactBabel?.(babelOptions)
      }
    }
  }
}

function viteReactCss() {
  return {
    name: 'vite-react-css',
    api: {
      reactBabel(babel) {
        babel.plugins.push(require.resolve('@foo/babel-plugin'))
      }
    }
  }
}

Windicss and svelte follows the latter pattern. windicss code, svelte code.

[patak-dev]

The second one is a very interesting pattern, it looks a lot cleaner to me. If the ecosystem is going to start doing this, we should document that this is supported so Vite doesn't do things like freezing the ConfigResolved object in the future (if that is even possible). But it is still important to define that plugin can modify the config in configResolved. I'll bring this to the next team meeting.

From the current docs: https://vitejs.dev/guide/api-plugin.html#configresolved

Called after the Vite config is resolved. Use this hook to read and store the final resolved config. It is also useful when the plugin needs to do something different based the command is being run.

[aleclarson]

The 2nd pattern is good, except that there's no warning if vite:react-babel is missing. Of course, the dependent plugin could use peerDependencies, but that doesn't ensure that it's actually included in the user's Vite config (but maybe that's good enough for now?).

Another option: We could formalize the declaration of a cross-plugin dependency like so:

// Dependent plugins would add @vitejs/plugin-react to devDependencies
// so the `api` object could be statically typed.
import type { ReactBabel } from '@vitejs/plugin-react'

// …within vite-react-css plugin
config({ getPlugin }) {
  // `getPlugin` should probably return the plugin object (instead of only its `api` object)
  // since its other hooks could be useful. For example, vite-plugin-mdx uses the `transform`
  // hook of @vitejs/plugin-react-refresh to enable Fast Refresh in MDX files.
  const reactBabel = getPlugin<ReactBabel>('vite:react-babel', true /* required */).api

  reactBabel.mergeConfig({ plugins: […], presets: […] })
}

The getPlugin helper would throw if the required argument is truthy, otherwise it would return undefined if the specified plugin is missing.

[bluwy]

The 2nd pattern is good, except that there's no warning if vite:react-babel is missing. Of course, the dependent plugin could use peerDependencies, but that doesn't ensure that it's actually included in the user's Vite config (but maybe that's good enough for now?).

The dependent plugin can manually issue a warning if the babel plugin is a must, by checking for the plugin like in the 1st pattern using config.plugins.find.

Another option: We could formalize the declaration of a cross-plugin dependency like so:

This seems to be going back to the 1st pattern, and I'm not sure why we need a blessed way of searching for plugins instead of config.plugins.find, unless I'm missing something here 😅

[yyx990803]

My vote is on the 2nd style by @bluwy.

[patak-dev]

Something else we discussed in the meeting is that the worry in my comment here #5454 (comment) isn't really an issue. The ResolvedConfig object is not being modified in the 2nd style, what changes is the internal plugin config so it is totally fair to do so. There is no need to update the docs.

[aleclarson]

I've updated this PR. The vite:react-babel plugin now looks for other plugins with api.reactBabel defined. Those plugins receive the Babel options (with plugins, presets, and parserOpts.plugins guaranteed to exist) and the resolved Vite config.