Skip to content

Commit 7093cd6

Browse files
eps1loneps1lon
committed
fix(Transition): Stale nodes in transition callbacks
1 parent a4e8160 commit 7093cd6

File tree

6 files changed

+55
-63
lines changed

6 files changed

+55
-63
lines changed

‎src/CSSTransition.js‎

Lines changed: 12 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -90,72 +90,61 @@ class CSSTransition extends React.Component {
9090
exit: {},
9191
}
9292

93-
onEnter = (maybeNode, maybeAppearing) => {
94-
const [node, appearing] = this.resolveArguments(maybeNode, maybeAppearing)
93+
onEnter = (node, appearing) => {
9594
this.removeClasses(node, 'exit');
9695
this.addClass(node, appearing ? 'appear' : 'enter', 'base');
9796

9897
if (this.props.onEnter) {
99-
this.props.onEnter(maybeNode, maybeAppearing)
98+
this.props.onEnter(node, appearing)
10099
}
101100
}
102101

103-
onEntering = (maybeNode, maybeAppearing) => {
104-
const [node, appearing] = this.resolveArguments(maybeNode, maybeAppearing)
102+
onEntering = (node, appearing) => {
105103
const type = appearing ? 'appear' : 'enter';
106104
this.addClass(node, type, 'active')
107105

108106
if (this.props.onEntering) {
109-
this.props.onEntering(maybeNode, maybeAppearing)
107+
this.props.onEntering(node, appearing)
110108
}
111109
}
112110

113-
onEntered = (maybeNode, maybeAppearing) => {
114-
const [node, appearing] = this.resolveArguments(maybeNode, maybeAppearing)
111+
onEntered = (node, appearing) => {
115112
const type = appearing ? 'appear' : 'enter'
116113
this.removeClasses(node, type);
117114
this.addClass(node, type, 'done');
118115

119116
if (this.props.onEntered) {
120-
this.props.onEntered(maybeNode, maybeAppearing)
117+
this.props.onEntered(node, appearing)
121118
}
122119
}
123120

124-
onExit = (maybeNode) => {
125-
const [node] = this.resolveArguments(maybeNode)
121+
onExit = (node) => {
126122
this.removeClasses(node, 'appear');
127123
this.removeClasses(node, 'enter');
128124
this.addClass(node, 'exit', 'base')
129125

130126
if (this.props.onExit) {
131-
this.props.onExit(maybeNode)
127+
this.props.onExit(node)
132128
}
133129
}
134130

135-
onExiting = (maybeNode) => {
136-
const [node] = this.resolveArguments(maybeNode)
131+
onExiting = (node) => {
137132
this.addClass(node, 'exit', 'active')
138133

139134
if (this.props.onExiting) {
140-
this.props.onExiting(maybeNode)
135+
this.props.onExiting(node)
141136
}
142137
}
143138

144-
onExited = (maybeNode) => {
145-
const [node] = this.resolveArguments(maybeNode)
139+
onExited = (node) => {
146140
this.removeClasses(node, 'exit');
147141
this.addClass(node, 'exit', 'done');
148142

149143
if (this.props.onExited) {
150-
this.props.onExited(maybeNode)
144+
this.props.onExited(node)
151145
}
152146
}
153147

154-
// when prop `nodeRef` is provided `node` is excluded
155-
resolveArguments = (maybeNode, maybeAppearing) => this.props.nodeRef
156-
? [this.props.nodeRef.current, maybeNode] // here `maybeNode` is actually `appearing`
157-
: [maybeNode, maybeAppearing] // `findDOMNode` was used
158-
159148
getClassNames = (type) => {
160149
const { classNames } = this.props;
161150
const isStringClassNames = typeof classNames === 'string';

‎src/Transition.js‎

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -224,29 +224,26 @@ class Transition extends React.Component {
224224
performEnter(mounting) {
225225
const { enter } = this.props
226226
const appearing = this.context ? this.context.isMounting : mounting
227-
const [maybeNode, maybeAppearing] = this.props.nodeRef
228-
? [appearing]
229-
: [ReactDOM.findDOMNode(this), appearing]
230227

231228
const timeouts = this.getTimeouts()
232229
const enterTimeout = appearing ? timeouts.appear : timeouts.enter
233230
// no enter animation skip right to ENTERED
234231
// if we are mounting and running this it means appear _must_ be set
235232
if ((!mounting && !enter) || config.disabled) {
236233
this.safeSetState({ status: ENTERED }, () => {
237-
this.props.onEntered(maybeNode)
234+
this.props.onEntered(this.getNode())
238235
})
239236
return
240237
}
241238

242-
this.props.onEnter(maybeNode, maybeAppearing)
239+
this.props.onEnter(this.getNode(), appearing)
243240

244241
this.safeSetState({ status: ENTERING }, () => {
245-
this.props.onEntering(maybeNode, maybeAppearing)
242+
this.props.onEntering(this.getNode(), appearing)
246243

247244
this.onTransitionEnd(enterTimeout, () => {
248245
this.safeSetState({ status: ENTERED }, () => {
249-
this.props.onEntered(maybeNode, maybeAppearing)
246+
this.props.onEntered(this.getNode(), appearing)
250247
})
251248
})
252249
})
@@ -255,26 +252,23 @@ class Transition extends React.Component {
255252
performExit() {
256253
const { exit } = this.props
257254
const timeouts = this.getTimeouts()
258-
const maybeNode = this.props.nodeRef
259-
? undefined
260-
: ReactDOM.findDOMNode(this)
261255

262256
// no exit animation skip right to EXITED
263257
if (!exit || config.disabled) {
264258
this.safeSetState({ status: EXITED }, () => {
265-
this.props.onExited(maybeNode)
259+
this.props.onExited(this.getNode())
266260
})
267261
return
268262
}
269263

270-
this.props.onExit(maybeNode)
264+
this.props.onExit(this.getNode())
271265

272266
this.safeSetState({ status: EXITING }, () => {
273-
this.props.onExiting(maybeNode)
267+
this.props.onExiting(this.getNode())
274268

275269
this.onTransitionEnd(timeouts.exit, () => {
276270
this.safeSetState({ status: EXITED }, () => {
277-
this.props.onExited(maybeNode)
271+
this.props.onExited(this.getNode())
278272
})
279273
})
280274
})
@@ -316,9 +310,7 @@ class Transition extends React.Component {
316310

317311
onTransitionEnd(timeout, handler) {
318312
this.setNextCallback(handler)
319-
const node = this.props.nodeRef
320-
? this.props.nodeRef.current
321-
: ReactDOM.findDOMNode(this)
313+
const node = this.getNode()
322314

323315
const doesNotHaveTimeoutOrListener =
324316
timeout == null && !this.props.addEndListener
@@ -328,17 +320,20 @@ class Transition extends React.Component {
328320
}
329321

330322
if (this.props.addEndListener) {
331-
const [maybeNode, maybeNextCallback] = this.props.nodeRef
332-
? [this.nextCallback]
333-
: [node, this.nextCallback]
334-
this.props.addEndListener(maybeNode, maybeNextCallback)
323+
this.props.addEndListener(node, this.nextCallback)
335324
}
336325

337326
if (timeout != null) {
338327
setTimeout(this.nextCallback, timeout)
339328
}
340329
}
341330

331+
getNode() {
332+
return this.props.nodeRef
333+
? this.props.nodeRef.current
334+
: ReactDOM.findDOMNode(this);
335+
}
336+
342337
render() {
343338
const status = this.state.status
344339

‎test/CSSTransition-test.js‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -122,18 +122,21 @@ describe('CSSTransition', () => {
122122
classNames='appear-test'
123123
in={true}
124124
appear={true}
125-
onEnter={(isAppearing) => {
125+
onEnter={(node, isAppearing) => {
126126
count++;
127+
expect(node).toEqual(nodeRef.current);
127128
expect(isAppearing).toEqual(true);
128129
expect(nodeRef.current.className).toEqual('appear-test-appear');
129130
}}
130-
onEntering={(isAppearing) => {
131+
onEntering={(node, isAppearing) => {
131132
count++;
133+
expect(node).toEqual(nodeRef.current);
132134
expect(isAppearing).toEqual(true);
133135
expect(nodeRef.current.className).toEqual('appear-test-appear appear-test-appear-active');
134136
}}
135137

136-
onEntered={(isAppearing) => {
138+
onEntered={(node, isAppearing) => {
139+
expect(node).toEqual(nodeRef.current);
137140
expect(isAppearing).toEqual(true);
138141
expect(nodeRef.current.className).toEqual('appear-test-appear-done appear-test-enter-done');
139142
expect(count).toEqual(2);
@@ -185,11 +188,13 @@ describe('CSSTransition', () => {
185188
classNames={{ appear: 'appear-test' }}
186189
in={true}
187190
appear={true}
188-
onEnter={(isAppearing) => {
191+
onEnter={(node, isAppearing) => {
192+
expect(node).toEqual(nodeRef.current);
189193
expect(isAppearing).toEqual(true);
190194
expect(nodeRef.current.className).toEqual('appear-test');
191195
}}
192-
onEntered={(isAppearing) => {
196+
onEntered={(node, isAppearing) => {
197+
expect(node).toEqual(nodeRef.current);
193198
expect(isAppearing).toEqual(true);
194199
expect(nodeRef.current.className).toEqual('');
195200
done();
@@ -215,19 +220,22 @@ describe('CSSTransition', () => {
215220
).setProps({
216221
in: true,
217222

218-
onEnter(isAppearing){
223+
onEnter(node, isAppearing){
219224
count++;
225+
expect(node).toEqual(nodeRef.current);
220226
expect(isAppearing).toEqual(false);
221227
expect(nodeRef.current.className).toEqual('not-appear-test-enter');
222228
},
223229

224-
onEntering(isAppearing){
230+
onEntering(node, isAppearing){
225231
count++;
232+
expect(node).toEqual(nodeRef.current);
226233
expect(isAppearing).toEqual(false);
227234
expect(nodeRef.current.className).toEqual('not-appear-test-enter not-appear-test-enter-active');
228235
},
229236

230-
onEntered(isAppearing){
237+
onEntered(node, isAppearing){
238+
expect(node).toEqual(nodeRef.current);
231239
expect(isAppearing).toEqual(false);
232240
expect(nodeRef.current.className).toEqual('not-appear-test-enter-done');
233241
expect(count).toEqual(2);

‎test/SwitchTransition-test.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ describe('SwitchTransition', () => {
1010
beforeEach(() => {
1111
log = [];
1212
let events = {
13-
onEnter: (m) => log.push(m ? 'appear' : 'enter'),
14-
onEntering: (m) => log.push(m ? 'appearing' : 'entering'),
15-
onEntered: (m) => log.push(m ? 'appeared' : 'entered'),
13+
onEnter: (node, appearing) => log.push(appearing ? 'appear' : 'enter'),
14+
onEntering: (node, appearing) => log.push(appearing ? 'appearing' : 'entering'),
15+
onEntered: (node, appearing) => log.push(appearing ? 'appeared' : 'entered'),
1616
onExit: () => log.push('exit'),
1717
onExiting: () => log.push('exiting'),
1818
onExited: () => log.push('exited')

‎test/Transition-test.js‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ describe('Transition', () => {
100100
})
101101

102102
it('should allow addEndListener instead of timeouts', done => {
103-
let listener = jest.fn(end => setTimeout(end, 0))
103+
let listener = jest.fn((node, end) => setTimeout(end, 0))
104104

105105
const nodeRef = React.createRef()
106106
let wrapper = mount(
@@ -121,7 +121,7 @@ describe('Transition', () => {
121121

122122
it('should fallback to timeouts with addEndListener', done => {
123123
let calledEnd = false
124-
let listener = (end) =>
124+
let listener = (node, end) =>
125125
setTimeout(() => {
126126
calledEnd = true
127127
end()
@@ -512,13 +512,13 @@ describe('Transition', () => {
512512
})
513513

514514
describe('node in callbacks', () => {
515-
it('use stale nodes', done => {
515+
it('does not use stale nodes', done => {
516516
const enteringNode = React.createRef();
517517
const enteredNode = React.createRef();
518518

519519
function makeAssertions() {
520-
expect(enteringNode.current.nodeName).toBe('H1');
521-
expect(enteredNode.current.nodeName).toBe('H1');
520+
expect(enteringNode.current.nodeName).toBe('H2');
521+
expect(enteredNode.current.nodeName).toBe('H3');
522522

523523
done();
524524
}

‎test/TransitionGroup-test.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,9 @@ describe('TransitionGroup', () => {
2626

2727
log = []
2828
let events = {
29-
onEnter: (m) => log.push(m ? 'appear' : 'enter'),
30-
onEntering: (m) => log.push(m ? 'appearing' : 'entering'),
31-
onEntered: (m) => log.push(m ? 'appeared' : 'entered'),
29+
onEnter: (node, appearing) => log.push(appearing ? 'appear' : 'enter'),
30+
onEntering: (node, appearing) => log.push(appearing ? 'appearing' : 'entering'),
31+
onEntered: (node, appearing) => log.push(appearing ? 'appeared' : 'entered'),
3232
onExit: () => log.push('exit'),
3333
onExiting: () => log.push('exiting'),
3434
onExited: () => log.push('exited'),

0 commit comments

Comments
 (0)