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

Failing test for false positive "setState on unmounted component" warning #15067

Closed
wants to merge 1 commit into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 8, 2019

Failing test for #15057.
Possibly relates to some cases of #14369.

The crux of the issue is that even if we use a guard variable, sometimes an effect can't setState without triggering a warning. In particular, this happens if:

  • We set state in a component (e.g. from an event emitter call)
  • We flush previous effect before we the state gets set
  • Flushing previous effect caused a state change in a parent which resulted in component unmounting
  • The scheduler thinks we called setState on an unmounted component, and warns

I'm not sure how to fix it. I imagine I'd expect that if flushPassiveEffects here led to unmounting of that component, we ideally shouldn't warn here.

I'm not sure if there are any bigger issues in cases where it's not unmounted though. Is this just a stray warning or is there a larger potential concern?

@acdlite
Copy link
Collaborator

acdlite commented Mar 9, 2019

We could just not warn if there are pending passive effects.

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 15, 2019

#15067

@acdlite
Copy link
Collaborator

acdlite commented Mar 16, 2019

I think this is the same as the serial/discrete events problem. Added a comment in your PR: #15122 (comment)

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 30, 2019

This will be fixed in 16.9

@gaearon gaearon closed this Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants