Skip to content

Commit 582180a

Browse files
ctrlplusbctrlplusb
committed
Experimental componentWillUnmount calls now get executed after children resolved, and additionally no longer have their errors swallowed.
1 parent a10a054 commit 582180a

2 files changed

Lines changed: 78 additions & 79 deletions

File tree

‎src/__tests__/index.test.js‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
/* eslint-disable class-methods-use-this */
66

77
import React, { Component } from 'react'
8+
import ReactDOM from 'react-dom'
89
import PropTypes from 'prop-types'
910
import reactTreeWalker from '../index'
1011

@@ -137,13 +138,12 @@ describe('reactTreeWalker', () => {
137138
})
138139
})
139140

140-
it('calls componentWillUnmount and does not fail if it errors', () => {
141+
it('calls componentWillUnmount', () => {
141142
let called = true
142143

143144
class Baz extends Component {
144145
componentWillUnmount() {
145146
called = true
146-
throw new Error('An error during unmount')
147147
}
148148

149149
render() {
@@ -219,6 +219,33 @@ describe('reactTreeWalker', () => {
219219
})
220220
})
221221

222+
/*
223+
fit('supports portals', () => {
224+
// eslint-disable-next-line react/prefer-stateless-function
225+
function Baz() {
226+
return ReactDOM.createPortal(
227+
<div>
228+
<Foo data={1} />
229+
<Foo data={2} />
230+
</div>,
231+
document.createElement('div'),
232+
)
233+
}
234+
const actual = []
235+
// eslint-disable-next-line no-unused-vars
236+
const visitor = (element, instance, context) => {
237+
if (instance && typeof instance.getData === 'function') {
238+
const data = instance.getData()
239+
actual.push(data)
240+
}
241+
}
242+
return reactTreeWalker(<Baz />, visitor).then(() => {
243+
const expected = [1, 2]
244+
expect(actual).toEqual(expected)
245+
})
246+
})
247+
*/
248+
222249
describe('error handling', () => {
223250
it('throws async visitor errors', () => {
224251
const tree = createTree(true)

‎src/index.js‎

Lines changed: 49 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,6 @@ const ensureChild = child =>
5454
? ensureChild(child.render())
5555
: child
5656

57-
const isPromise = x => x != null && typeof x.then === 'function'
58-
5957
// Recurse an React Element tree, running visitor on each element.
6058
// If visitor returns `false`, don't call the element's render function
6159
// or recurse into its child elements
@@ -79,53 +77,43 @@ export default function reactTreeWalker(
7977

8078
const recursive = (currentElement, currentContext) =>
8179
new Promise(innerResolve => {
82-
const doVisit = (getChildren, visitResult, childContext) => {
83-
const visitChildren = () => {
84-
const child = ensureChild(getChildren())
85-
const theChildContext =
86-
typeof childContext === 'function' ? childContext() : childContext
87-
88-
if (child == null) {
89-
// If no children then we can't traverse. We've reached the leaf.
90-
innerResolve()
91-
} else if (Children.count(child)) {
92-
// If its a react Children collection we need to breadth-first
93-
// traverse each of them.
94-
const mapper = aChild =>
95-
aChild ? recursive(aChild, theChildContext) : undefined
96-
const children = Children.map(child, cur => cur)
97-
// pMapSeries allows us to do depth-first traversal. Thanks @sindresorhus!
98-
pMapSeries(children, mapper)
99-
.then(innerResolve, reject)
100-
.catch(reject)
101-
} else {
102-
// Otherwise we pass the individual child to the next recursion.
103-
recursive(child, theChildContext)
104-
.then(innerResolve, reject)
105-
.catch(reject)
106-
}
107-
}
108-
109-
if (visitResult === false) {
110-
// Visitor returned false, indicating a desire to not traverse.
111-
innerResolve()
112-
} else if (isPromise(visitResult)) {
113-
// We need to execute the result and pass it's result through to our
114-
// continuer.
115-
visitResult
116-
.then(promiseResult => {
117-
if (promiseResult === false) {
80+
const visitCurrentElement = (childResolver, context, compInstance) =>
81+
Promise.resolve(safeVisitor(currentElement, compInstance, context))
82+
.then(result => {
83+
if (result === false) {
84+
// Visitor returned false, indicating a desire to not visit
85+
// the children of the current element, so we will just resolve.
86+
innerResolve()
87+
} else {
88+
// A false wasn't returned so we will attempt to visit the children
89+
// for the current element.
90+
const child = ensureChild(childResolver())
91+
const theChildContext =
92+
typeof context === 'function' ? context() : context
93+
94+
if (child == null) {
95+
// No children. We've reached the end of this branch. resolve.
11896
innerResolve()
97+
} else if (Children.count(child)) {
98+
// If its a react Children collection we need to breadth-first
99+
// traverse each of them, and pMapSeries allows us to do a
100+
// depth-first traversal that respects Promises. Thanks @sindresorhus!
101+
pMapSeries(
102+
Children.map(child, cur => cur),
103+
aChild =>
104+
aChild ? recursive(aChild, theChildContext) : undefined,
105+
)
106+
.then(innerResolve, reject)
107+
.catch(reject)
119108
} else {
120-
visitChildren()
109+
// Otherwise we pass the individual child to the next recursion.
110+
recursive(child, theChildContext)
111+
.then(innerResolve, reject)
112+
.catch(reject)
121113
}
122-
})
123-
.catch(reject)
124-
} else {
125-
// Visitor returned true, indicating a desire to continue traversing.
126-
visitChildren()
127-
}
128-
}
114+
}
115+
})
116+
.catch(reject)
129117

130118
// Is this element a Component?
131119
if (typeof currentElement.type === 'function') {
@@ -164,62 +152,46 @@ export default function reactTreeWalker(
164152
instance.state = Object.assign({}, instance.state, newState)
165153
}
166154

167-
doVisit(
155+
visitCurrentElement(
168156
() => {
169-
// Call componentWillMount if it exists.
170157
if (instance.componentWillMount) {
171158
instance.componentWillMount()
172159
}
173-
174-
const children = instance.render()
175-
176-
if (
177-
options.componentWillUnmount &&
178-
instance.componentWillUnmount
179-
) {
180-
try {
181-
instance.componentWillUnmount()
182-
} catch (err) {
183-
// This is an experimental feature, we don't want to break
184-
// the bootstrapping process, but lets warn the user it
185-
// occurred.
186-
console.warn(
187-
'Error calling componentWillUnmount whilst walking your react tree',
188-
)
189-
console.warn(err)
190-
}
191-
}
192-
193-
return children
160+
return instance.render()
194161
},
195-
safeVisitor(currentElement, instance, currentContext),
196162
() =>
197-
// Ensure the child context is initialised if it is available. We will
198-
// need to pass it down the tree.
163+
// Ensure the child context is initialised if it is available.
164+
// We will need to pass it down the tree.
199165
instance.getChildContext
200166
? Object.assign(
201167
{},
202168
currentContext,
203169
instance.getChildContext(),
204170
)
205171
: currentContext,
206-
)
172+
instance,
173+
).then(() => {
174+
if (
175+
options.componentWillUnmount &&
176+
instance.componentWillUnmount
177+
) {
178+
instance.componentWillUnmount()
179+
}
180+
})
207181
} else {
208182
// Stateless Functional Component
209-
doVisit(
183+
visitCurrentElement(
210184
() => Component(props, currentContext),
211-
safeVisitor(currentElement, null, currentContext),
212185
currentContext,
213186
)
214187
}
215188
} else {
216189
// This must be a basic element, such as a string or dom node.
217-
doVisit(
190+
visitCurrentElement(
218191
() =>
219192
currentElement.props && currentElement.props.children
220193
? currentElement.props.children
221194
: undefined,
222-
safeVisitor(currentElement, null, currentContext),
223195
currentContext,
224196
)
225197
}

0 commit comments

Comments
 (0)