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

Reconciliation fails when using lodash/memoize #984

Open
baldurh opened this issue May 18, 2018 · 8 comments
Open

Reconciliation fails when using lodash/memoize #984

baldurh opened this issue May 18, 2018 · 8 comments

Comments

@baldurh
Copy link

baldurh commented May 18, 2018

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

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

baldurh commented May 19, 2018

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

fixed in v4.3.0

@baldurh
Copy link
Author

baldurh commented Jun 6, 2018

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

🤷‍♂️😭
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 theKashey reopened this Jun 6, 2018
@theKashey
Copy link
Collaborator

#995 (comment) 🤷‍♂️

@baldurh
Copy link
Author

baldurh commented Jun 7, 2018

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

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.

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

No branches or pull requests

2 participants