fix: disable refresh for non-jsx without react import

[hi-ogawa]

Description

• Closes Switching to plugin-react-oxc triggers "$RefreshReg$ is not defined" errors vite-plugin-react#514

@vitejs/plugin-react has this check isJSX || code.includes(jsxImportRuntime) || code.includes(jsxImportDevRuntime) for fast refresh. Since the default filter allows non-jsx file, this additional check is necessary on oxc refresh transform side.

I added a test case in vitejs/vite-plugin-react#547 and confirmed it's fixed by pkg.pr.new

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vitejs/rolldown-vite/@vitejs/plugin-legacy@318

npm i https://pkg.pr.new/vitejs/rolldown-vite@318

commit: 898632a

[sapphi-red]

Hmm, this code feels too specific to the plugin to include in the core and feels hard to keep the condition in sync between the core and the plugin.
But if we add an option to configure this behavior, then it would be difficult to make it harder to keep the native transform plugin performant.

Do you have any alternative ideas? Or maybe this trade-off is just inevitable...

[hi-ogawa]

Is the coupling between core and plugin so much of concern? For example, I was assuming rolldown-vite's oxc.jsx.refresh: true should be only used by @vitejs/plugin-react-oxc and thus these are expected to be coupled. I don' think code syncing is an real problem here or I might be missing something.

[hi-ogawa]

OK, I think technically applying refresh transform to non-jsx is not an issue by itself. Probably @vitejs/plugin-react needed this logic for perf, but I don't think this behavior is something we need to preserve in the future. Other bundlers likely don't have this specific behavior as they normally don't care about jsx in non-jsx file. (I just tested Parcel and it injected $RefreshReg$ everywhere).

The original issue is about $RefreshReg$/$RefreshSig$ not being defined in worker, so we should solve that aspect eventually, but for now, I think having this fix on rolldown-vite js side seems worth it.

[sapphi-red]

I wouldn't say this is a major problem. It's just something that would be nice to avoid.

The issues I see with this approach are:

• Changing this condition would require a change in Vite core. Technically, this means plugin-react would need to update its peer dependency to keep things in sync. That said, this could be ignored if the breakage isn't significant.
• It makes using oxc.refresh: true difficult for other plugins. While oxc.jsx.refresh is somewhat of an internal option, it's still a public one and could be used by other plugins (e.g., maybe by React Router in the future?). It would be better to keep it generic if possible.
• The react-specific code lives in the core. It is harder to find the specific code as you normally assume the code lives in the plugin-react repo. It would be hard anyways as native related code will live in rolldown repo though.

I don’t have a better alternative at the moment, so I’ll go ahead and merge this unless you have any other suggestions.

[hi-ogawa]

I'm interested in trying out no-op $RefreshReg/Sig$ as fallback for worker, but I think landing this as compatibility fix is good. Intentionally changing the behavior from @vitejs/plugin-react can be done a separate step.