Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
bjufre opened this issue Jun 4, 2021 · 11 comments · Fixed by #79
Closed
Labels
bug Something isn't working feat: hmr

Comments

@bjufre
Copy link

bjufre commented Jun 4, 2021

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
Copy link
Member

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
Copy link
Author

bjufre commented Jun 5, 2021

@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
Copy link

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
Copy link

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
Copy link
Member

@jenslind See this explanation:
image

@Aeolun
Copy link

Aeolun commented Dec 14, 2021

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
Copy link

DouglasDev commented Feb 8, 2022

@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
Copy link
Member

bluwy commented Mar 11, 2022

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

@dickeylth
Copy link

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

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

@IanVS
Copy link

IanVS commented Aug 31, 2022

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

@patak-dev patak-dev transferred this issue from vitejs/vite Dec 3, 2022
@ArnaudBarre
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feat: hmr
Projects
None yet