feat: add plugin-react-oxc

[sapphi-red]

Description

A new plugin for react that leverages Oxc built into Rolldown.

How the test works

For each playground with __tests__/oxc directory, vitestGlobalSetup.ts copies the original playground to a separate directory for the oxc plugin (this is similar to what we have with variant playgrounds in Vite core repo, for example playground/css/__tests__/lightningcss). After that vitestGlobalSetup.ts replaces node_modules/vite and node_modules/@vitejs/plugin-react with a symlink that points to rolldown-vite and @vitejs/plugin-react-oxc.

For each playground, vitestGlobalSetup.ts adds _vite-proxy.js that re-exports vite (export * from 'vite'). This is to import vite resolved from each playground directory. In vitestSetup.ts, vite is imported via _vite-proxy.js so that rolldown-vite is used for oxc variant playgrounds.

Making it work with ecosystem-ci

@vitejs/plugin-react-oxc has "vite": "catalog:rolldown-vite" as a dev dependency. This catalog item points to npm:rolldown-vite@^6.3.0-beta.5. This is to allow ecosystem-ci to keep that dep as-is.
Because ecosystem-ci adds an override for vite, it also changes the vite devDep for plugin-react-oxc. To avoid that from happening, in the config for this repo for ecosystem-ci, I added '@vitejs/plugin-react-oxc>vite': 'catalog:rolldown-vite'. This way the devDep will keep pointing the rolldown-vite package.

[sapphi-red]

I'm wondering how to make this work without overrides.

[sapphi-red]

OK. I managed to make the tests working. Now I'm going to check if this works ok with ecosystem-ci.

[hi-ogawa]

Looks good to me!

For the testing, I was thinking about using Vitest workspace to split babel and oxc then setting up alias for each, but I realized we still need to split temp dir to avoid mixing hmr and also @vitejs/plugin-react needs to be aliased for vite config's esbuild to understand. Directly swapping node_modules seems like a reliable approach 👍

One question, it looks like fast-refresh.ts and refreshUtils.js are almost identical to the ones in packages/plugin-react. Should we try to dedupe them? I think copyRefreshUtils.ts script can handle just exact copy and fast-refresh.ts can be also replaced with a weird direct import of ../../plugin-react/src/fast-refresh.

[ArnaudBarre]

What do you think about using a setup closer to the SWC plugin for fast refresh utils?
It makes the code cleaner and doesn't change a lot (I just checked there was not anything change upstream since 01/2023)

[sapphi-red]

Sure. Let me dedupe and refactor the fast refresh handling in a separate PR first.

[sapphi-red]

Made a PR for #439 (comment) here: #443

[sapphi-red]

I've merged the main branch. The addRefreshWrapper is a bit complex because there's no sourcemap to modify in oxc plugin, but the overall complexity should be now reduced!

[ArnaudBarre]

Did you try generated an empty source map that will then only be ';'.repeat(17) + newMap.mappings?
Not a big deal but cleaner when you inspect the pipeline changes

[sapphi-red]

It's because newMap doesn't exist here.
https://github.com/vitejs/vite-plugin-react/pull/439/files#diff-57c3aa7ebcc7d479870d1da730fee916a15a41215f2a6af821383235f62f3b59R114-R129
We can generate it by new MagicString(code).generateMap({ hires: 'boundary' }), but I guess that has a bigger overhead.

[ArnaudBarre]

Does rollup allows to get the current sourceMap of the transform pipeline?
So we can edit it instead of generating a new one and having Vite merge them after

[sapphi-red]

It has a way to get the current combined source map (this.getCombinedSourcemap). But there isn't a way to tell rollup that the returned sourcemap is a combined one.

[ArnaudBarre]

I did some experiment but using an empty sourceMappings where only ; is apprend breaks the concatenation somewhere because the output doesn't contain any source map (without logs)

So I tried this:

        const { code: newCode, map } = addRefreshWrapper(
          code,
          {
            version: 3,
            file: undefined,
            sources: [''],
            sourcesContent: undefined,
            names: [],
            mappings: 'AAAA;'.repeat(code.split('\n').length - 1),
          },
          '@vitejs/plugin-react-oxc',
          id,
        )
        return { code: newCode, map }

Inspired by what magic-string does as a default and once again was surprised out slow unpredictable JS perf is.

This test also made me find that the ';'.repeat should be 16 and not 17 because there are 17 lines but 16 line breaks.

With the change I have this kind of output.

For Babel I found that's it's a bit broken.

For SWC it map a bit more (onClick) but fails the children is off

I'm still ok for going the full first line inline way, it feels hacky for me but probably only people like me look at generated code, the same way normal people never look at the URL 😅

[sapphi-red]

'AAAA;'.repeat(code.split('\n').length - 1),

I think this works similarly to hires: false and probably that's the reason the sourcemap is a bit broken when combined with babel.

I prefer to go with the current way for now.

[ArnaudBarre]

Nope the soucemap is perfectly valid, it just says the line didn't change which is true. When combined it works well with the OXC sourcemap
On the babel plugin the sourcemap is not working great but that's nothing to do with 'AAAA;'.repeat(code.split('\n').length - 1) which I added only on the OXC plugin

[sapphi-red]

Nope the soucemap is perfectly valid, it just says the line didn't change which is true.

'AAAA;'.repeat(code.split('\n').length - 1) only maps the first character of each lines (which is the same behavior with hires: false). If I remember correctly, the position of console.log will be at the first character of the line rather than the position next to console.log. This is not that important as we have multiple places doing similar to it (it's better if we can avoid them though). But in addition to that, it causes troubles when combined with other sourcemaps.
https://x.com/bluwyoo/status/1674036158714769408 (I didn't find any examples, only found this post 😅)

[sapphi-red]

What we want is a range mapping (a proposal exists: https://github.com/tc39/ecma426/blob/main/proposals/range-mappings.md), but currently sourcemaps only supports character-by-character mapping.

[ArnaudBarre]

Ok thanks I see and I understand now why people wants a new source map spec 😅
That's sad that such a common usecase is not easy to do 😢

[ArnaudBarre]

Note related to this PR because the issue also exist in the babel plugin but this is not working in dev.
I not sure yet what is the best way to handle this (I though about adding src/refresh-runtime.js to gitignore and having a copy step in the dev script, but not 100% convinced)

[sapphi-red]

I'll take a look if I can fix this in a separate PR 👍