Expand "@vitejs/plugin-react-refresh" to allow class components and non-component exports

[bjufre]

Clear and concise description of the problem

Recently at work we hit a point that was something of a wake-up call for us in terms of bundle size and tree-shaking capabilities and DX while working on our huge React codebase.

We decided that it might be time to try and look elsewhere for another solution that would improve our happines while working on the project as well as not sacrificing any of the existing features and final assets.

That's why we looked at Vite, it looked interesting, exciting as well as the tool that we were looking for. So, we started migrating and ditching Webpack in favor of Vite. It has been an interesting few weeks, mobilizing some scripts, components, etc. But we think it has been worth the time.

Despite all of that, we soon hit one paint-point that was "kind of a deal-breaker", and that was the HMR experience in our codebase. Due to the fact, that we realy heavily in class components and Redux and its well know hoc connect, we were seeing that the browser was being refreshed all the time. Of course, that not not an issue with Vite per-se, but it's a limitation of react-refresh itself, see here).

That's why we would like to submit some code, get it reviewed and improved and allow developers not migrating to Vite due to class components, to make that final jump.

Suggested solution

The solution that we first tried was to try to implement a Runtime workaround, by expanding the react-refresh/runtime itself with a function explained in this discussion. But the fix relies on the assumption that one can "inspect" the current and previous exports of "the module". But as of now, that's not the case with Vite, so we have to look for some other solution.

That why we expanded the plugin in our local environment to handle class components with HMR. The approach has been to modify the "boundary check function" to include class components and to return the different boundaries that we've guessed from the file in question. Then we modify the final code to include the necessary $RefreshReg$ calls so that we can tie everything together with react-refresh.

At the same time, to allow for some customization and flexibility we've added a new option to the plugin named excludeExports that allows the developer to specify "named" exports that must be ignored when "guessing" those boundaries; e.g:

// vite.config.js
import reactRefresh from '@vitejs/plugin-react-refresh'

export default {
  plugins: [reactRefresh({
	excludeExports: ['mapStateToProps', 'mapDispatchToProps']
  })]
}

This helps colocating exports such as the ones named in that example within the component (for testing purposes) while having the HMR working as expected.

NOTE: This is really something that can be kept out of the this implementation/solution, as for most devs, this might not be as much of a deal-breaker and they might be ok with moving them to separate files. But it's very nice for migrating projects like ours.

Alternative

Another solution that can be implemented is to expand Vite and allow the inspection of module exports and rely upon the react-refresh/runtime itself to handle the logic for knowing whether an export is a component or not (this would make the excludeExports option not really something worth doing.). That of course, would be more "safe" and might allow Vite to delegate most of the "burden/guesswork" to the runtime.

This would implement the runtime "function check" (see the discussion linked above) in the preambleCode of the plugin and then is just a matter of calling the function with the module exports, something like: isReactRefreshBoundary(import.meta.hot.module).

[aleclarson]

I think it's unlikely we'll want to complicate the Fast Refresh integration just to support an outdated class component architecture. If you think I'm overestimating the maintenance burden, feel free to open a PR and I'll see what the team thinks. Another option is for you to publish and maintain a vite-react-refresh-legacy plugin. WDYT?

[bjufre]

@aleclarson that makes sense, and I kinda figured that it might be a little out of scope for the current plugin. I think that having a small little plugin for the legacy components as a transition bridge until the architecture is migrated, sounds alright.

I will take a stab at it in the upcoming days, thank you very much for your time and for taking the time to answer!

[JesusTheHun]

I think it's unlikely we'll want to complicate the Fast Refresh integration just to support an outdated class component architecture.

Calling class components outdated is a pretty bold statement.
React clearly stated that they do not plan to drop the support of it. So, not outdated.

[jenslind]

Kinda running into a similar issue here migrating a webpack project to vite. I know little about how HMR works but whats the reason for checking that all exports are components?

// Exporting a variable breaks HMR
export const variable = ''
export const Component = () => {}

This works fine with our current setup using react-refresh-webpack-plugin

[aleclarson]

@jenslind See this explanation:

[Aeolun]

I wish my class components were reloading the entire page. Instead nothing happens at all. If I modify my file so that it's exporting a functional component that wraps the class component, everything magically starts working again.

Is that related to this issue? Maybe a change was made to prevent the full page reload, but now it results in no reload at all?

E.g.

// NO GOOD
export Component = connect(mapStateToProps)(MyClassComponent)

// HAPPY
const IntermediateComponent = connect(mapStateToProps)(MyClassComponent)
export Component = function() {
  return <IntermediateComponent />
}

I'm baffled by this.

[DouglasDev]

@Aeolun I was thinking of switching from snowpack to vite because snowpack has the exact same bug (FredKSchott/snowpack#3323) and it seems unlikely that it will be fixed. Disappointing that there is no good alternative when most React projects more than a couple of years old are largely written using class components.

[bluwy]

Is this still an issue with @vitejs/plugin-react?

[dickeylth]

Is this still an issue with @vitejs/plugin-react?

I think it should be, class components are still necessary in many scenarios.

[IanVS]

Potentially related: vitejs/vite#9869, depending on if/how it is solved.

[ArnaudBarre]

Similar to #11, this issue will be fixed by #79. Waiting for Vite 4.1 to enable this change