From 7093cd64b536c32ebe802ad7bf2883586ead447c Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Tue, 12 Mar 2019 20:40:13 +0100 Subject: [PATCH] fix(Transition): Stale nodes in transition callbacks --- src/CSSTransition.js | 35 ++++++++++++--------------------- src/Transition.js | 37 +++++++++++++++-------------------- test/CSSTransition-test.js | 24 +++++++++++++++-------- test/SwitchTransition-test.js | 6 +++--- test/Transition-test.js | 10 +++++----- test/TransitionGroup-test.js | 6 +++--- 6 files changed, 55 insertions(+), 63 deletions(-) diff --git a/src/CSSTransition.js b/src/CSSTransition.js index d0240b62..c219c390 100644 --- a/src/CSSTransition.js +++ b/src/CSSTransition.js @@ -90,72 +90,61 @@ class CSSTransition extends React.Component { exit: {}, } - onEnter = (maybeNode, maybeAppearing) => { - const [node, appearing] = this.resolveArguments(maybeNode, maybeAppearing) + onEnter = (node, appearing) => { this.removeClasses(node, 'exit'); this.addClass(node, appearing ? 'appear' : 'enter', 'base'); if (this.props.onEnter) { - this.props.onEnter(maybeNode, maybeAppearing) + this.props.onEnter(node, appearing) } } - onEntering = (maybeNode, maybeAppearing) => { - const [node, appearing] = this.resolveArguments(maybeNode, maybeAppearing) + onEntering = (node, appearing) => { const type = appearing ? 'appear' : 'enter'; this.addClass(node, type, 'active') if (this.props.onEntering) { - this.props.onEntering(maybeNode, maybeAppearing) + this.props.onEntering(node, appearing) } } - onEntered = (maybeNode, maybeAppearing) => { - const [node, appearing] = this.resolveArguments(maybeNode, maybeAppearing) + onEntered = (node, appearing) => { const type = appearing ? 'appear' : 'enter' this.removeClasses(node, type); this.addClass(node, type, 'done'); if (this.props.onEntered) { - this.props.onEntered(maybeNode, maybeAppearing) + this.props.onEntered(node, appearing) } } - onExit = (maybeNode) => { - const [node] = this.resolveArguments(maybeNode) + onExit = (node) => { this.removeClasses(node, 'appear'); this.removeClasses(node, 'enter'); this.addClass(node, 'exit', 'base') if (this.props.onExit) { - this.props.onExit(maybeNode) + this.props.onExit(node) } } - onExiting = (maybeNode) => { - const [node] = this.resolveArguments(maybeNode) + onExiting = (node) => { this.addClass(node, 'exit', 'active') if (this.props.onExiting) { - this.props.onExiting(maybeNode) + this.props.onExiting(node) } } - onExited = (maybeNode) => { - const [node] = this.resolveArguments(maybeNode) + onExited = (node) => { this.removeClasses(node, 'exit'); this.addClass(node, 'exit', 'done'); if (this.props.onExited) { - this.props.onExited(maybeNode) + this.props.onExited(node) } } - // when prop `nodeRef` is provided `node` is excluded - resolveArguments = (maybeNode, maybeAppearing) => this.props.nodeRef - ? [this.props.nodeRef.current, maybeNode] // here `maybeNode` is actually `appearing` - : [maybeNode, maybeAppearing] // `findDOMNode` was used - getClassNames = (type) => { const { classNames } = this.props; const isStringClassNames = typeof classNames === 'string'; diff --git a/src/Transition.js b/src/Transition.js index 981acc26..5d0639b1 100644 --- a/src/Transition.js +++ b/src/Transition.js @@ -224,9 +224,6 @@ class Transition extends React.Component { performEnter(mounting) { const { enter } = this.props const appearing = this.context ? this.context.isMounting : mounting - const [maybeNode, maybeAppearing] = this.props.nodeRef - ? [appearing] - : [ReactDOM.findDOMNode(this), appearing] const timeouts = this.getTimeouts() const enterTimeout = appearing ? timeouts.appear : timeouts.enter @@ -234,19 +231,19 @@ class Transition extends React.Component { // if we are mounting and running this it means appear _must_ be set if ((!mounting && !enter) || config.disabled) { this.safeSetState({ status: ENTERED }, () => { - this.props.onEntered(maybeNode) + this.props.onEntered(this.getNode()) }) return } - this.props.onEnter(maybeNode, maybeAppearing) + this.props.onEnter(this.getNode(), appearing) this.safeSetState({ status: ENTERING }, () => { - this.props.onEntering(maybeNode, maybeAppearing) + this.props.onEntering(this.getNode(), appearing) this.onTransitionEnd(enterTimeout, () => { this.safeSetState({ status: ENTERED }, () => { - this.props.onEntered(maybeNode, maybeAppearing) + this.props.onEntered(this.getNode(), appearing) }) }) }) @@ -255,26 +252,23 @@ class Transition extends React.Component { performExit() { const { exit } = this.props const timeouts = this.getTimeouts() - const maybeNode = this.props.nodeRef - ? undefined - : ReactDOM.findDOMNode(this) // no exit animation skip right to EXITED if (!exit || config.disabled) { this.safeSetState({ status: EXITED }, () => { - this.props.onExited(maybeNode) + this.props.onExited(this.getNode()) }) return } - this.props.onExit(maybeNode) + this.props.onExit(this.getNode()) this.safeSetState({ status: EXITING }, () => { - this.props.onExiting(maybeNode) + this.props.onExiting(this.getNode()) this.onTransitionEnd(timeouts.exit, () => { this.safeSetState({ status: EXITED }, () => { - this.props.onExited(maybeNode) + this.props.onExited(this.getNode()) }) }) }) @@ -316,9 +310,7 @@ class Transition extends React.Component { onTransitionEnd(timeout, handler) { this.setNextCallback(handler) - const node = this.props.nodeRef - ? this.props.nodeRef.current - : ReactDOM.findDOMNode(this) + const node = this.getNode() const doesNotHaveTimeoutOrListener = timeout == null && !this.props.addEndListener @@ -328,10 +320,7 @@ class Transition extends React.Component { } if (this.props.addEndListener) { - const [maybeNode, maybeNextCallback] = this.props.nodeRef - ? [this.nextCallback] - : [node, this.nextCallback] - this.props.addEndListener(maybeNode, maybeNextCallback) + this.props.addEndListener(node, this.nextCallback) } if (timeout != null) { @@ -339,6 +328,12 @@ class Transition extends React.Component { } } + getNode() { + return this.props.nodeRef + ? this.props.nodeRef.current + : ReactDOM.findDOMNode(this); + } + render() { const status = this.state.status diff --git a/test/CSSTransition-test.js b/test/CSSTransition-test.js index 59fa557c..475414ec 100644 --- a/test/CSSTransition-test.js +++ b/test/CSSTransition-test.js @@ -122,18 +122,21 @@ describe('CSSTransition', () => { classNames='appear-test' in={true} appear={true} - onEnter={(isAppearing) => { + onEnter={(node, isAppearing) => { count++; + expect(node).toEqual(nodeRef.current); expect(isAppearing).toEqual(true); expect(nodeRef.current.className).toEqual('appear-test-appear'); }} - onEntering={(isAppearing) => { + onEntering={(node, isAppearing) => { count++; + expect(node).toEqual(nodeRef.current); expect(isAppearing).toEqual(true); expect(nodeRef.current.className).toEqual('appear-test-appear appear-test-appear-active'); }} - onEntered={(isAppearing) => { + onEntered={(node, isAppearing) => { + expect(node).toEqual(nodeRef.current); expect(isAppearing).toEqual(true); expect(nodeRef.current.className).toEqual('appear-test-appear-done appear-test-enter-done'); expect(count).toEqual(2); @@ -185,11 +188,13 @@ describe('CSSTransition', () => { classNames={{ appear: 'appear-test' }} in={true} appear={true} - onEnter={(isAppearing) => { + onEnter={(node, isAppearing) => { + expect(node).toEqual(nodeRef.current); expect(isAppearing).toEqual(true); expect(nodeRef.current.className).toEqual('appear-test'); }} - onEntered={(isAppearing) => { + onEntered={(node, isAppearing) => { + expect(node).toEqual(nodeRef.current); expect(isAppearing).toEqual(true); expect(nodeRef.current.className).toEqual(''); done(); @@ -215,19 +220,22 @@ describe('CSSTransition', () => { ).setProps({ in: true, - onEnter(isAppearing){ + onEnter(node, isAppearing){ count++; + expect(node).toEqual(nodeRef.current); expect(isAppearing).toEqual(false); expect(nodeRef.current.className).toEqual('not-appear-test-enter'); }, - onEntering(isAppearing){ + onEntering(node, isAppearing){ count++; + expect(node).toEqual(nodeRef.current); expect(isAppearing).toEqual(false); expect(nodeRef.current.className).toEqual('not-appear-test-enter not-appear-test-enter-active'); }, - onEntered(isAppearing){ + onEntered(node, isAppearing){ + expect(node).toEqual(nodeRef.current); expect(isAppearing).toEqual(false); expect(nodeRef.current.className).toEqual('not-appear-test-enter-done'); expect(count).toEqual(2); diff --git a/test/SwitchTransition-test.js b/test/SwitchTransition-test.js index 76d2a527..4e57d2b6 100644 --- a/test/SwitchTransition-test.js +++ b/test/SwitchTransition-test.js @@ -10,9 +10,9 @@ describe('SwitchTransition', () => { beforeEach(() => { log = []; let events = { - onEnter: (m) => log.push(m ? 'appear' : 'enter'), - onEntering: (m) => log.push(m ? 'appearing' : 'entering'), - onEntered: (m) => log.push(m ? 'appeared' : 'entered'), + onEnter: (node, appearing) => log.push(appearing ? 'appear' : 'enter'), + onEntering: (node, appearing) => log.push(appearing ? 'appearing' : 'entering'), + onEntered: (node, appearing) => log.push(appearing ? 'appeared' : 'entered'), onExit: () => log.push('exit'), onExiting: () => log.push('exiting'), onExited: () => log.push('exited') diff --git a/test/Transition-test.js b/test/Transition-test.js index 21e56e2c..a7822686 100644 --- a/test/Transition-test.js +++ b/test/Transition-test.js @@ -100,7 +100,7 @@ describe('Transition', () => { }) it('should allow addEndListener instead of timeouts', done => { - let listener = jest.fn(end => setTimeout(end, 0)) + let listener = jest.fn((node, end) => setTimeout(end, 0)) const nodeRef = React.createRef() let wrapper = mount( @@ -121,7 +121,7 @@ describe('Transition', () => { it('should fallback to timeouts with addEndListener', done => { let calledEnd = false - let listener = (end) => + let listener = (node, end) => setTimeout(() => { calledEnd = true end() @@ -512,13 +512,13 @@ describe('Transition', () => { }) describe('node in callbacks', () => { - it('use stale nodes', done => { + it('does not use stale nodes', done => { const enteringNode = React.createRef(); const enteredNode = React.createRef(); function makeAssertions() { - expect(enteringNode.current.nodeName).toBe('H1'); - expect(enteredNode.current.nodeName).toBe('H1'); + expect(enteringNode.current.nodeName).toBe('H2'); + expect(enteredNode.current.nodeName).toBe('H3'); done(); } diff --git a/test/TransitionGroup-test.js b/test/TransitionGroup-test.js index 8920e8c2..88628b71 100644 --- a/test/TransitionGroup-test.js +++ b/test/TransitionGroup-test.js @@ -26,9 +26,9 @@ describe('TransitionGroup', () => { log = [] let events = { - onEnter: (m) => log.push(m ? 'appear' : 'enter'), - onEntering: (m) => log.push(m ? 'appearing' : 'entering'), - onEntered: (m) => log.push(m ? 'appeared' : 'entered'), + onEnter: (node, appearing) => log.push(appearing ? 'appear' : 'enter'), + onEntering: (node, appearing) => log.push(appearing ? 'appearing' : 'entering'), + onEntered: (node, appearing) => log.push(appearing ? 'appeared' : 'entered'), onExit: () => log.push('exit'), onExiting: () => log.push('exiting'), onExited: () => log.push('exited'),