-
Notifications
You must be signed in to change notification settings - Fork 31
Conversation
Codecov Report
@@ Coverage Diff @@
## master #550 +/- ##
=========================================
+ Coverage 97.93% 98.03% +0.1%
=========================================
Files 38 39 +1
Lines 967 968 +1
Branches 213 214 +1
=========================================
+ Hits 947 949 +2
+ Misses 19 18 -1
Partials 1 1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you look to see what's going on here?
STR:
- Load http://localhost:3000/en-US/browse/494431/versions/1532144/
- Click on the root folder to expand it
- Change line 98 so that it says
LOL
instead of a line number (or any edit like this).
The component updates but the tree collapses. It looks like it's replaying all Redux actions starting with initialState
. It's not reloading the page, so that's good, but resetting the state defeats the purpose of HMR. Any idea what's going on?
I'll try to fix it but I bet this is: reduxjs/redux#1940 |
Hmm, that issue suggest that it is unfixable, no? |
It seems it should just work, but it does not. We could maybe backup the state before hot reloading. |
I suppose that will work. Creating a root reducer with a predefined state is well supported. I guess it's all still a workaround for facebook/create-react-app#6503 but that's OK. |
Actually, I think this is an issue with our app and not with the HMR setup. |
Also, we can't really backup the state before applying the HMR patch. Redux does not allow that. |
Can you file it with STR so we don't forget to fix it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+, ship it. Thanks!
I tested component changes, style changes, and reducer changes. It all seems to work but none of our components can handle a re-mount with existing state 😂 so I'm not totally sure if HMR is behaving how we want it to.
It's somewhat surprising to me that HMR re-mounts components that are already on the screen. I thought it was only supposed to update them. In any case, that's fine because we can deal with it. We can fix each component to reflect Redux state correctly.
I didn't think we needed all components to reflect Redux state for HMR support but that makes sense now that I think about it. I knew we would need it for SSR but we don't need SSR so we've been lax about reflecting Redux state.
I filed #551 |
Fixes #275
I started from scratch, trying to understand whether CRA officially
supports HMR (hint: it is not as clear as it should be). In the end, I
added some code to enable HMR for React [1] and Redux [2]. It's similar
to what I've done for addons-frontend in the past, albeit much simpler.
Creating
src/reducers/index.tsx
is required so that we can trackchanges on all reducers at once.
[1]: I tested by live editing a React component, which is updated in FF
without reloading the page. It's hot patched instead
[2]: I tested by adding a new prop to a reducer and it works too (it can
be monitored in the redux devtools)