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

bug in beta.21 #843

Closed
fwh1990 opened this issue Feb 5, 2018 · 27 comments · May be fixed by Omrisnyk/npm-lockfiles#153
Closed

bug in beta.21 #843

fwh1990 opened this issue Feb 5, 2018 · 27 comments · May be fixed by Omrisnyk/npm-lockfiles#153
Assignees
Labels

Comments

@fwh1990
Copy link

fwh1990 commented Feb 5, 2018

Can only update a mounted or mounting component. This usually means you called setState, replaceState, or forceUpdate on an unmounted component. This is a no-op.

webpack 3.10.0
babel 7.0-beta39

Rollback to beta-19 worked.
I don't know why it happened, no more code provide to you, sorry.

@gregberge
Copy link
Collaborator

This is our number one bug. @theKashey it seems to be introduced by your render PR.

@theKashey
Copy link
Collaborator

@fwh1990 - so, as long you got a bug - you are the best person to track it down! 🏹

  1. Enable debug mode. setConfig({debug: true}). Maybe it will shed some light (probably not).
  2. Double check that this warning is just a warning. Ie react-hot-loader working correctly.
  3. And what is you actually changing. Why something got unmount?

The problem is caused by #806. On each render, and pre-prender call each Component you use will compare some magical number. It will change when you execute a new portion of code (HRM, async-chunk, whatever).
Then, it thinks: hm, maybe it was HRM, lets double check.. and perform hotReplacementRender. And next it schedules a force update to all components updated. React will throw a warning if I do it during the render.

The question - why something got rendered, but next got unmounted.

So - what I am asking about - just double check that your application and RHL is working.
If yes - I will just track the mounted state, and don't update freshly unmounted components.
If no - so that will a be a problem, as long it should be ok.

@skipjack
Copy link

skipjack commented Feb 6, 2018

@theKashey I'm experiencing this issue too, it's being thrown on a ton of components. I don't fully understand all your points though...

Enable debug mode. setConfig({debug: true}). Maybe it will shed some light (probably not).

I understand this point based on this.

Double check that this warning is just a warning. Ie react-hot-loader working correctly.

As far as I could tell the loader still worked fine from a functionality standpoint, however maybe I'm missing something about this question.

And what is you actually changing. Why something got unmount?

I'm not sure what you mean by this, but...

  • One of the components with children in the tree unmounts.
  • A varied amount of errors are thrown for components that don't contain any setState, replaceState, or forceUpdate calls.

Hope this helps. Not sure how much else I can dig into this but if needed I might be able to supply a reproducible example. Reverting to 4.0.0-beta.19, as mentioned by @fwh1990, does fix the issue.

@theKashey
Copy link
Collaborator

  • One of the components with children in the tree unmounts.

Is it expected behaviour? Something is unmounting cos it should do it, or it is a bug?

  • A varied amount of errors are thrown for components that don't contain any setState, replaceState, or forceUpdate calls.

RHL converts all the components into the Stateful components, and also call .forceUpdate on them. So - you can have stateful error on stateless component 😸

@skipjack
Copy link

skipjack commented Feb 6, 2018

Is it expected behaviour? Something is unmounting cos it should do it, or it is a bug?

Yes, the unmounting is expected. Occurs when changing routes via react-router (at least in my case).

RHL converts all the components into the Stateful components, and also call .forceUpdate on them. So - you can have stateful error on stateless component.

Yeah that would explain it. That sounds more like a "this package" issue rather than something a consumer of the package is doing wrong.

@theKashey
Copy link
Collaborator

Ok. So everything is OK, and we should just track the component state to suppress the warning.

@fwh1990
Copy link
Author

fwh1990 commented Feb 8, 2018

You may try to test react-loadable package.

@fnpen
Copy link

fnpen commented Feb 8, 2018

You may try to test react-loadable package.

Have the same problem.

@fnpen
Copy link

fnpen commented Feb 8, 2018

I'm using react-loadable@5.3.1

react-hot-loader@4.0.0-beta.19 is working nice.

If react-hot-loader@4.0.0-beta.21:

Warning: Can only update a mounted or mounting component. This usually means you called setState, replaceState, or forceUpdate on an unmounted component. This is a no-op.

Please check the code for the loading component.

If react-hot-loader@4.0.0-beta.20:

invariant.js:42 Uncaught Error: HotExportedApp(...): Nothing was returned from render. This usually means a return statement is missing. Or, to render nothing, return null.

What can I do?

@theKashey
Copy link
Collaborator

@fnpen the second error is a different error. And look it is IS an error. Please provide more information.

@fnpen
Copy link

fnpen commented Feb 8, 2018

@theKashey I can make repo with code to help reproduce it. Do you need it?

@theKashey
Copy link
Collaborator

@fnpen - it will be just perfect.

@fnpen
Copy link

fnpen commented Feb 8, 2018

@theKashey https://github.com/fnpen/react-hot-loader-mount-bug

Not working with clean create-react-app + react-hot-loader@4.0.0-beta.21. (without redux, router and etc.)

Run yarn start and press button.

@theKashey
Copy link
Collaborator

🚀

@fnpen
Copy link

fnpen commented Feb 8, 2018

@theKashey @thibautRe

https://github.com/gaearon/react-hot-loader/blob/dd24b52a6d1c52210ca919a548d05764df7cdff1/src/AppContainer.dev.js

shouldComponentUpdate(prevProps, prevState) {
    // Don't update the component if the state had an error and still has one.
    // This allows to break an infinite loop of error -> render -> error -> render
    // https://github.com/gaearon/react-hot-loader/issues/696
    if (prevState.error && this.state.error) {
      return false
    }

    return true
  }

Always true? Or need check change?

@theKashey
Copy link
Collaborator

Always true. AppContainer shall not decide how the application would work.

@theKashey theKashey mentioned this issue Feb 8, 2018
gregberge pushed a commit that referenced this issue Feb 10, 2018
* Incriment generation only on replacement event(related to #843)

* hotRender: drill into the new children branch(fix #843)

* tests for the props change case + ci

* react-hot-renderer - optional child case
@gregberge
Copy link
Collaborator

Fixed in beta 22.

@fnpen
Copy link

fnpen commented Feb 10, 2018

@neoziro @theKashey thanks!

@fnpen
Copy link

fnpen commented Feb 11, 2018

@theKashey solved for example. But error exists in my app with redux / react-router. I will try extract the reason to my example.

@theKashey
Copy link
Collaborator

@fnpen - just share some sources.

@FallOutChonny
Copy link

Hello, first of all I would like to thank you guys for maintaining this project.
I update RHL to 4.0.0-beta.22 and have similar problem, project used:

react - 16.2.0 with SSR
styled-components - 3.1.6
react-loadable - 5.3.1
babel - 7.0.0-beta.38
webpack - 3.10.0

When I edit file and save changes, terminal shows
Imgur

and developer tools shows
Imgur

Or I don't edit file, just change the route, developer tools shows a lot of warning messages
Imgur

Hot-reload is ok, but the warning message looks a bit annoying.
Could you please help me? Thank you.

@theKashey
Copy link
Collaborator

HotExportedAbout should synchronously flush the changes, but it doesn't do it. Just a few lines to debug.

@theKashey
Copy link
Collaborator

I've failed to find the reason about "Can only update" - should be ok in last beta, in case you dont get error with "cannot read children".
And that error is actually an error - #830 (comment), fixing it right now.

theKashey added a commit that referenced this issue Feb 13, 2018
@cyrilwanner
Copy link

cyrilwanner commented Feb 13, 2018

I also have the same issue in beta.22 ("Can only update" but without the children error FallOutChonny has). React-hot-loader still works as expected, there are just these additional warnings.

I've setup an example project with loadable-components as I also have this in my real project, if it helps you debugging the cause: https://github.com/cyrilwanner/react-hot-loader-bug (follow the steps in the readme to reproduce the bug).

Please tell me if I can provide additional information/help.

@theKashey
Copy link
Collaborator

@cyrilwanner - thank you. Looking....

@theKashey
Copy link
Collaborator

theKashey commented Feb 13, 2018

Ok, kudos to @cyrilwanner, not it's clear - HRM/RHL works very well. A bit more very than it should.

  1. You are updating something. Everything is ok, and update got scoped by hot HOC.
  2. Meanwhile a global variable, we call generation got increased.
  3. When you hit a button, and change a route - whole React tree start re-rendering.
  4. "Sub-render" trap detect that something gonna to be rendered, but that something was rendered within lower generation, and may contain async-updated HRM chunks.
  5. It starts hot-render process, to reconcile the possible change.
  6. Meanwhile, hot-render just rendered the old react tree, cos react-hot-renderer ignores context changes, as long context updates are outside just rendering it could(and shall) perform.
  7. Now it is a time for a normal React rendered. It renders tree as it should be, and unmounts components scheduled for update.
  8. Profit 👍

The ways to fix:

  1. The logic in .4 is just incorrect. It should react only to react updates occurred just after HRM event, the renders normal flow might just miss(portals, async-loaded components and so on).
    Have no idea how to distinguish these two execution branches -_ just after_ HRM, and 10 seconds after HRM. Or how to "raise" stored generations of all the components, not related to a specific update.
  2. Try to hook into willRecieveProps. That will allow just sync-flush of force-updates.
  3. Track the mounted state of components, just to hide these errors.

WIP.

@theKashey
Copy link
Collaborator

theKashey commented Feb 14, 2018

2 and 3 got implemented.

gregberge pushed a commit that referenced this issue Feb 16, 2018
* safe define displayName, fix #845

* stand children did not exists, fix #843

* improve TS documentation

* add SSR+HRM example

* subrender - fix deferred updates

* dont fix package version

* should have react as a peer dep

* use didMount instead of will, related to #860
gregberge pushed a commit that referenced this issue Feb 18, 2018
* safe define displayName, fix #845

* stand children did not exists, fix #843

* improve TS documentation

* add SSR+HRM example

* subrender - fix deferred updates

* transfer original class methods into the Proxy prototype, fixes #858

* fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants