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 experimental react-refresh support #8582

Merged
merged 11 commits into from Apr 6, 2020

Conversation

@charrondev
Copy link
Contributor

charrondev commented Feb 29, 2020

Issue pmmmwh/react-refresh-webpack-plugin#7

Since react-refresh was released, I've been really excited to use it as a replacement for react-hot-loader & others.

@pmmmmwh/react-refresh-webpack-plugin is the farthest along webpack plugin right now. You can see it's progress over time here pmmmwh/react-refresh-webpack-plugin#1

Now as you might see as of Feb 28th 2020, not all of that issue is complete, however the remaining pieces don't really seem relevant as CRA doesn't really need advanced configuration options. If the maintainers here determine that those would be blockers to adoption in CRA, then maybe this PR will help get some additional eyes on the project.

Changes

  • Bring in the @pmmmmwh/react-refresh-webpack-plugin & react-refresh.
  • Create a new version of the webpack entry. I removed everything involving the error/warning overlay. (the webpack plugin provides it's own that looks a lot more like the react native one used for fast refresh).

Open Questions

  • Since the error overlay isn't being used here anymore should it be removed, or should it be kept around for some time? In any case I didn't do it because there may be other complications involved in that.
  • I tried just replacing the react-dev-utils webpack entry with one of the built in ones, but then the refresh didn't work anymore. It may be nice for that not to be necessary anymore in the future, but I don't think it necessarily has to be the work of this PR.
  • I removed the console logging and errors from the dev entry. Should that be left alone? Currently that other entry provides an overlay but doesn't seem to do any console logging. Maybe it would be best handled over in that plugin at this point in time? I'm open to any changes here.

Testing instructions

  • Clone the repo & checkout this branch.
  • Spin up a local CRA from this branch. yarn create-react-app test-react-refresh
  • cd test-react-refresh
  • yarn start

After some feedback, I've put this behind a ENV variable right now to enable, so add the following file to the root of your created project.

.env

# Enable React Refresh
FAST_REFRESH=true

Hot Reloading Hooks & State preservation

Hook state preservation is one the main improvements out of react-refresh vs it's predecessors, so the following should be a quick way to validate that it works.

  • Open App.js
  • Replace it's contents with the following:
    import React, { useState } from 'react';
    import logo from './logo.svg';
    import './App.css';
    
    function App() {
      const [inputValue, setInputValue] = useState("initial");
    
      return (
        <div className="App">
          <header className="App-header">
            <img src={logo} className="App-logo" alt="logo" />
            <input style={{ fontSize: 40 }} value={inputValue} onChange={e => setInputValue(e.target.value)} />
            <button type="button" onClick={() => {
              throw new Error("runtime error");
            }}>Throw Runtime error</button>
            <p>
              Edit <code>src/App.js</code> and save to reloadasdf.
            </p>
            <a
              className="App-link"
              href="https://reactjs.org"
              target="_blank"
              rel="noopener noreferrer"
            >
              Learn React
            </a>
          </header>
        </div>
      );
    }
    
    export default App;
  • Open the page and see the input.
  • Type some value into the input.
  • Go back to source code and change some other value.
  • See the updated value, and the hook state preserved.
  • Create a syntax error in App.js
  • See the syntax error appear in the page.
  • Fix the syntax error.
  • See the page go back to normal. The hook/input value should still be preserved.

Gifs make everything better.

react-refresh

@facebook-github-bot
Copy link

facebook-github-bot commented Feb 29, 2020

Hi @charrondev!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

facebook-github-bot commented Feb 29, 2020

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ianschmitz ianschmitz self-assigned this Feb 29, 2020
@ianschmitz ianschmitz added this to the 3.4.1 milestone Feb 29, 2020
@ianschmitz
Copy link
Collaborator

ianschmitz commented Feb 29, 2020

Awesome thanks for the PR @charrondev. I'll review this weekend.

@mrmckeb
Copy link
Collaborator

mrmckeb commented Feb 29, 2020

This looks great, my only concern here is that (and this is our fault) we don't have a huge range of test cases for this.

Perhaps we could release as a beta. What do you think @ianschmitz?

@ianschmitz
Copy link
Collaborator

ianschmitz commented Feb 29, 2020

Yeah we might want to put behind a flag and treat as experimental until those libs mature and iron out any issues.

@charrondev
Copy link
Contributor Author

charrondev commented Feb 29, 2020

@ianschmitz I can put a .env flag to use this for now. How does REACT_REFRESH sound?

I've added it right now behind this env flag.

@charrondev
Copy link
Contributor Author

charrondev commented Feb 29, 2020

One other thing I'm noticing as I test this is the actual cycle of

  • Make a change to CRA.
  • Spin up a new instance using that version.

Takes ~3-4 minutes on my machine. Have you guys considered making a couple private packages in the yarn workspace that are already wired up to the local packages?

I might just be doing it wrong though. I've been using yarn create-react-app to start up it up and using that, but then if I update any packages, I have start that from scratch again.

@ianschmitz
Copy link
Collaborator

ianschmitz commented Mar 1, 2020

@charrondev check out https://github.com/facebook/create-react-app/blob/master/CONTRIBUTING.md#setting-up-a-local-copy for assistance. You can use yarn start. Don't worry about testing the e2e flow.

@pmmmwh
Copy link

pmmmwh commented Mar 1, 2020

Hi! Thanks for putting in the work to integrate the plugin with CRA. I actually did some work to make the integration smoother/more seamless - it would be great if you guys can checkout pmmmwh/react-refresh-webpack-plugin#44 and give me some feedback so that I can be confident that the API fits the use case :)

@ianschmitz
Copy link
Collaborator

ianschmitz commented Mar 2, 2020

Thanks @pmmmwh - much appreciated. Would be good to get that figured out to understand the impact here as we are tightly integrated with our current overlay.

@charrondev
Copy link
Contributor Author

charrondev commented Mar 4, 2020

After running this for a few days, there's a few differences between the overlays, and it probably would be best not to handle

  • React refresh &
  • A new overlay

At the same time.

Once @pmmmwh's PR has landed, I'll update this PR to use the built in CRA overlay instead. Then other changes regarding the overlay could be regarded separately.

@ianschmitz
Copy link
Collaborator

ianschmitz commented Mar 5, 2020

Agreed. I think for first pass to get this in we should make this opt in (via env variable) to let folks experiment with this and iterate on any issues that arise. Once we're happy that it's stable and all of our existing functionality plays nice we can make it the default behavior.

This means that the existing overlay should continue to work as it does today.

@iansu iansu modified the milestones: 3.4.1, 3.4.2 Mar 20, 2020
charrondev added 5 commits Feb 29, 2020
@charrondev charrondev force-pushed the charrondev:feature/react-refresh-webpack branch from a52f847 to 670b60a Mar 31, 2020
@charrondev charrondev requested a review from amyrlam as a code owner Apr 1, 2020
@ianschmitz ianschmitz modified the milestone: 3.4.2 Apr 1, 2020
@ianschmitz
Copy link
Collaborator

ianschmitz commented Apr 1, 2020

@pmmmwh we had an interesting case here where we didn't end up needing module, but had to stub it out to keep the plugin happy. Thoughts?

@pmmmwh
Copy link

pmmmwh commented Apr 1, 2020

@pmmmwh we had an interesting case here where we didn't end up needing module, but had to stub it out to keep the plugin happy. Thoughts?

I will relax the requirements a little bit in the next beta. Initially I didn't think that using a custom entry without using a custom overlay module would make sense, because it would bring in the plugin's default overlay and the integration between the entry and the overlay is not trivial.

However, in this case I think the error is indeed correct if you guys want to preserve react-error-overlay instead of using the bundled one - the two required functions are being called on re-evaling the module to dismiss runtime errors, and the signature required don't align with react-error-overlay ootb. So the stub probably needs to act as a proxy to link up the two, or I will have to change the function names to align with react-error-overlay and then an actual path to it could be provided instead. I haven't tested with the branch yet, but I reckon that part of error recovery won't work.

For reference:

@ianschmitz ianschmitz changed the title Add react-refresh webpack plugin Add react-refresh support Apr 6, 2020
@ianschmitz ianschmitz changed the title Add react-refresh support Add experimental react-refresh support Apr 6, 2020
@ianschmitz ianschmitz merged commit c5b96c2 into facebook:master Apr 6, 2020
16 checks passed
16 checks passed
facebook.create-react-app Build #20200406.2 succeeded
Details
facebook.create-react-app (Behavior LinuxNode10) Behavior LinuxNode10 succeeded
Details
facebook.create-react-app (Behavior LinuxNode8) Behavior LinuxNode8 succeeded
Details
facebook.create-react-app (Behavior MacNode10) Behavior MacNode10 succeeded
Details
facebook.create-react-app (Behavior MacNode8) Behavior MacNode8 succeeded
Details
facebook.create-react-app (Installs LinuxNode10) Installs LinuxNode10 succeeded
Details
facebook.create-react-app (Installs LinuxNode8) Installs LinuxNode8 succeeded
Details
facebook.create-react-app (Kitchensink LinuxNode10) Kitchensink LinuxNode10 succeeded
Details
facebook.create-react-app (Kitchensink LinuxNode8) Kitchensink LinuxNode8 succeeded
Details
facebook.create-react-app (KitchensinkEject LinuxNode10) KitchensinkEject LinuxNode10 succeeded
Details
facebook.create-react-app (KitchensinkEject LinuxNode8) KitchensinkEject LinuxNode8 succeeded
Details
facebook.create-react-app (OldNode) OldNode succeeded
Details
facebook.create-react-app (Simple LinuxNode10) Simple LinuxNode10 succeeded
Details
facebook.create-react-app (Simple LinuxNode8) Simple LinuxNode8 succeeded
Details
facebook.create-react-app (UnsupportedNodeWithTypeScript) UnsupportedNodeWithTypeScript succeeded
Details
netlify/create-react-app/deploy-preview Docs deploy preview succeeded
Details
@ianschmitz
Copy link
Collaborator

ianschmitz commented Apr 6, 2020

Thanks @charrondev!

@lock lock bot locked and limited conversation to collaborators Apr 15, 2020
@ianschmitz ianschmitz modified the milestones: 3.5, 4.0 May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.