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

Add fast refresh to default babel if packages exist #3220

Conversation

justin808
Copy link
Contributor

@justin808 justin808 commented Nov 9, 2021

With this change, it's extremely easy to make React work with HMR.

Here's a PR in my example project demonstrating the improvement:

shakacode/react_on_rails_demo_ssr_hmr#20

Since we already check for React in the bable config, let's do the right
thing if the package for fast-refresh is installed.
@justin808 justin808 force-pushed the justin808/add-fast-refresh-to-default-babel branch from 5df9305 to 7dc5cf4 Compare November 9, 2021 00:56
@guillaumebriday
Copy link
Member

I'm not sure this should be in the code because it seems to be really tight to React and I think Webpacker should not add framework related code in its own codebase.

What do you think?

@justin808
Copy link
Contributor Author

I added because we have other React code:

moduleExists('@babel/preset-react') && [

So we might as well support HMR if we're going to support React.

Or we should remove React.

I think it's fine to support React by checking if the packages are installed.

"It just works"

@guillaumebriday
Copy link
Member

As discussed, we should close this PR for #3224 right? 👍

@guillaumebriday
Copy link
Member

@justin808 Should be close this PR?

@justin808
Copy link
Contributor Author

Sure. Let's close it. I think we should merge #3224.

@justin808 justin808 closed this Dec 5, 2021
@justin808 justin808 deleted the justin808/add-fast-refresh-to-default-babel branch April 6, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants