search

gaearon / react-hot-loader

Tweak React components in real time. (Deprecated: use Fast Refresh instead.)
http://gaearon.github.io/react-hot-loader/
MIT License
12.26k stars 801 forks source link

Reconciliation fails when using lodash/memoize #984

Open baldurh opened 6 years ago

baldurh commented 6 years ago

Description

Hot reloading fails for files using lodash/memoize in 4.2.0

Removing all calls to the memoize function fixes the issue for that file. It doesn’t make sense that this problem has anything to do with the memoize function itself but it happens to be the problematic function in this case.

It was only being used at the top level of the file.

Expected behavior

Should hot reload the file.

Actual behavior

Hits an error: reconcilation failed due to error ReferenceError: resolver is not defined

resolver is the second argument passed to memoize

Environment

React Hot Loader version: 4.2.0

Run these commands in the project folder and fill in their results:

  1. node -v: 9.5.0
  2. npm -v: 5.6.0
  3. yarn -v: 1.6.0

Then, specify:

  1. Operating system: MacOS 10.13.4
  2. Browser and version: Chrome 66.0.3359.170

Reproducible Demo

N/A

theKashey commented 6 years ago

Please provide code to reproduce. As far I know - you can not get some modules updated, but reconcilation should not fail. If you are unable to provide some code - could you try to trace the bug by yourself? I need just a call stack, to find how calling to a render function could break hot update. There is 2 possibilities - something wrong with render, something wrong with class update, and RHL breaking your components. Both sounds 💩, and lets try to fix them!

baldurh commented 6 years ago

If I move all the memoized methods to another file this seems to work alright. I’m gonna try to create a simple setup that reproduces the problem.

theKashey commented 6 years ago

fixed in v4.3.0

baldurh commented 6 years ago

I cannot confirm this is fixed. 4.3.0 seems to have introduced new issues. Now, even if I remove the memoized methods from the class the reconciliation fails. That was enough to get everything working on 4.2.0. Now I don’t get this particular error but still nothing gets hot-reloaded.

Sorry I haven’t had time to create the test setup yet.

theKashey commented 6 years ago

🤷‍♂️😭 Please share any code to reproduce a bug. Difference between 4.2 and 4.3 is quite narrow - 4.2 is updating all functions, while 4.3 - only with “this” inside. It should not reintroduce the problem.

theKashey commented 6 years ago

https://github.com/gaearon/react-hot-loader/issues/995#issuecomment-395059652 🤷‍♂️

baldurh commented 6 years ago

I think I might have found what is wrong on my end though I don’t understand it completely 🙈

First, my comment above is wrong. I looked more into this and now I see that with 4.3.0 I no longer get the error reported in this issue: reconcilation failed due to error ReferenceError: resolver is not defined. So this issue is resolved 👍🏻

What I believe is the issue on my end is the memoized function is being used as a prop to a child component which had the changed component lower in the tree. So it seems the child component was still getting the same function passed as prop after the hot reload which then effectively stopped the re-rendering since it was a pure component.

Perhaps there is something else in play here as I have not been able to reproduce this in a simple app created from create-react-app (but that uses webpack 3.8 but we have webpack 4.7). We have a few HOCs and a custom made async component loader etc which might be disrupting the process. I’ve worked around the problem by refactoring the function so it no longer needs to be memoized. If this continues to be a problem for us I will try to create a simplified version of our own app to pinpoint this. I tried to do that but gave up after a while because of our complex setup.

theKashey commented 6 years ago

I am undestanding what you are talking about. Currently hot loader could not bypass any memoization, as long it's goal is to preserve values. There are some ideas how to fix it, add componentDidHotUpdate for example, but they all does not exists yet.

Currently you can chose what you want to get - component not updated, or state loss. Probably, in your case, state loss is preferred.

Use new cold method from react-hot-loader to disable RHL for a specific component. That will fix "not updating" issue, but will lead to tree branch unmount/mount on HRM event.