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

Fix "Uncaught TypeError: Cannot read property 'classList' of null" #270

Merged
merged 1 commit into from
Jan 5, 2018

Conversation

luontola
Copy link
Contributor

@luontola luontola commented Jan 5, 2018

This is a fix for issue #208

This fix has also been published at https://www.npmjs.com/package/@luontola/react-transition-group as 2.3.0-patch.3

@@ -187,6 +187,9 @@ class Transition extends React.Component {
// nextStatus will always be ENTERING or EXITING.
this.cancelNextCallback();
const node = ReactDOM.findDOMNode(this);
if (!node) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is enough to solve the problem. When is the node not available? this will leave the component in a bad state if it had to do anything else, does this situation mean the component is already unmounted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our use case is that we have complex forms where form fields are dynamically added and removed based on the values of other form fields. They are rendered like this:

<TransitionGroup>
  {fields.map(field =>
    <CSSTransition {...transitionProps}>
      {field}
    </CSSTransition>)}
</TransitionGroup>

The rendering of some of those fields also has many conditionals, including rendering the field as null:

const SomeField = ({someProp}) => {
  if (someProp === 'foo') {
    return <div>...</div>;
  }
  if (someProp === 'bar') {
    return <div>...</div>;
  }
  return null;
};

As a result, sometimes CSSTransition's child component will have no DOM node. This used to work fine with react-transition-group v1.2.0, but with v2.2.1 it causes the crash mentioned in issue #208.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so much concern here is that this effectively skips an animation, but doesn't clean up the state. So now you'd have the Transition Component props saying it's entered, but its internal state still saying it's exited.

I think this need to handled in CSSTranstion, which uses the DOM node as opposed to here which is mostly DOM agnostic. Said another way, i think transitioning a component where the DOM node is null is a valid use case, it should be handled up one level in CSSTransition where it's not a valid case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, handling it inside CSSTransition sounds best. My first attempt was to add null checks to addClass and removeClass in CSSTransition.js, but those two places were not enough, so I backed out before going too deep into that rabbit hole.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be eneough to return early from all of the onFoo hooks there

@luontola
Copy link
Contributor Author

luontola commented Jan 5, 2018

Here is an updated PR, which only modifies CSSTransition and also adds a test for the bug.

@jquense jquense merged commit 9cbb3e7 into reactjs:master Jan 5, 2018
@jquense
Copy link
Collaborator

jquense commented Jan 5, 2018

thanks!

@jpfiorilla
Copy link

solved the issue for me, thanks a bunch

@mreishus
Copy link

mreishus commented Feb 5, 2018

is there going to be a new release of react-transition-group that includes this fix?

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

Successfully merging this pull request may close these issues.

4 participants