Skip to content

Commit

Permalink
fix(Transition): Stale nodes in transition callbacks
Browse files Browse the repository at this point in the history
  • Loading branch information
@eps1lon
eps1lon committed May 8, 2020
1 parent a4e8160 commit 7093cd6
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 63 deletions.
35 changes: 12 additions & 23 deletions src/CSSTransition.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
37 changes: 16 additions & 21 deletions src/Transition.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,29 +224,26 @@ 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
// no enter animation skip right to ENTERED
// 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)
})
})
})
Expand All @@ -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())
})
})
})
Expand Down Expand Up @@ -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
Expand All @@ -328,17 +320,20 @@ 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) {
setTimeout(this.nextCallback, timeout)
}
}

getNode() {
return this.props.nodeRef
? this.props.nodeRef.current
: ReactDOM.findDOMNode(this);
}

render() {
const status = this.state.status

Expand Down
24 changes: 16 additions & 8 deletions test/CSSTransition-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions test/SwitchTransition-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
10 changes: 5 additions & 5 deletions test/Transition-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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()
Expand Down Expand Up @@ -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();
}
Expand Down
6 changes: 3 additions & 3 deletions test/TransitionGroup-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down

0 comments on commit 7093cd6

Please sign in to comment.