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

isMounted behavior different with create-react-class #9627

Closed
mridgway opened this issue May 9, 2017 · 7 comments
Closed

isMounted behavior different with create-react-class #9627

mridgway opened this issue May 9, 2017 · 7 comments
Milestone

Comments

@mridgway
Copy link
Contributor

mridgway commented May 9, 2017

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

Calling this.isMounted() in componentWillUnmount in prior versions would return true. Now it returns false.

I believe this was untested behavior before, but the new tests that were added may check the wrong value: https://github.com/facebook/react/blob/master/src/isomorphic/classic/class/__tests__/create-react-class-integration-test.js#L417 Changing this line to test for true will exhibit the behavior.

The fix would be to defer setting the __isMounted flag to false until after all mixins and the componentWillUnmount method were called on the component.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

15.5.x and anything using create-react-class is broken. Working correctly in 15.4.x with React.createClass.

@mridgway
Copy link
Contributor Author

mridgway commented May 9, 2017

I fixed this locally by splitting the IsMountedMixin into two:

  var IsMountedPreMixin = {
    componentDidMount: function () {
      this.__isMounted = true;
    }
  };

  var IsMountedPostMixin = {
    componentWillUnmount: function () {
      this.__isMounted = false;
    }
  };

Then installing the Post after the spec has been mixed in:

    mixSpecIntoComponent(Constructor, IsMountedPreMixin);
    mixSpecIntoComponent(Constructor, spec);
    mixSpecIntoComponent(Constructor, IsMountedPostMixin);

For the life of me I can't find the right repo to PR this against though.

mridgway added a commit to mridgway/react that referenced this issue May 9, 2017
mridgway added a commit to mridgway/react that referenced this issue May 9, 2017
Split the IsMountedMixin in two so that the __isMounted flag is set to false after componentWillUnmount is executed in mixins and the component.
gaearon pushed a commit that referenced this issue Jun 12, 2017
* [#9627] Fix create-react-class isMounted ordering issue

Split the IsMountedMixin in two so that the __isMounted flag is set to false after componentWillUnmount is executed in mixins and the component.

* Revert changes to integration test
@gaearon gaearon added this to the 15.6 milestone Jun 12, 2017
@gaearon gaearon closed this as completed Jun 12, 2017
nhunzaker pushed a commit to nhunzaker/react that referenced this issue Jun 13, 2017
…book#9638)

* [facebook#9627] Fix create-react-class isMounted ordering issue

Split the IsMountedMixin in two so that the __isMounted flag is set to false after componentWillUnmount is executed in mixins and the component.

* Revert changes to integration test
@gaearon
Copy link
Collaborator

gaearon commented Jun 13, 2017

This should be fixed with [email protected]. Can you please verify?

@mridgway
Copy link
Contributor Author

I don't see that version published yet.

@gaearon
Copy link
Collaborator

gaearon commented Jun 13, 2017

It's not set to latest (still an RC) but it exists:

https://unpkg.com/[email protected]/

Will turn into 15.6.0 tomorrow.

@mridgway
Copy link
Contributor Author

Ah indeed. It's probably our internal registry being slow. I'll try it out soon.

@mridgway
Copy link
Contributor Author

Confirmed working correctly in [email protected].

@gaearon
Copy link
Collaborator

gaearon commented Jun 13, 2017

Great. Thanks for sending the fix!

gaearon pushed a commit that referenced this issue Jun 20, 2017
* Fix componentWillUnmount test case in isMounted tests and add mixin tests

* Upgrade create-react-class to 15.6.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants