Skip to content

Commit

Permalink
Errors thrown on the fast path should be rethrown in render()
Browse files Browse the repository at this point in the history
This fixes #351 because child render() will never run for dispatches after componentWillUnmount()
  • Loading branch information
gaearon committed Apr 14, 2016
1 parent 85982f6 commit ea53d6f
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 1 deletion.
23 changes: 22 additions & 1 deletion src/components/connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ function getDisplayName(WrappedComponent) {
return WrappedComponent.displayName || WrappedComponent.name || 'Component'
}

let errorObject = { value: null }
function tryCatch(fn, ctx) {
try {
return fn.apply(ctx)
} catch (e) {
errorObject.value = e
return errorObject
}
}

// Helps track hot reloading.
let nextVersion = 0

Expand Down Expand Up @@ -220,6 +230,7 @@ export default function connect(mapStateToProps, mapDispatchToProps, mergeProps,
this.haveOwnPropsChanged = true
this.hasStoreStateChanged = true
this.haveStatePropsBeenPrecalculated = false
this.statePropsPrecalculationError = null
this.renderedElement = null
this.finalMapDispatchToProps = null
this.finalMapStateToProps = null
Expand All @@ -237,9 +248,13 @@ export default function connect(mapStateToProps, mapDispatchToProps, mergeProps,
}

if (pure && !this.doStatePropsDependOnOwnProps) {
if (!this.updateStatePropsIfNeeded()) {
const haveStatePropsChanged = tryCatch(this.updateStatePropsIfNeeded, this)
if (!haveStatePropsChanged) {
return
}
if (haveStatePropsChanged === errorObject) {
this.statePropsPrecalculationError = errorObject.value
}
this.haveStatePropsBeenPrecalculated = true
}

Expand All @@ -261,12 +276,18 @@ export default function connect(mapStateToProps, mapDispatchToProps, mergeProps,
haveOwnPropsChanged,
hasStoreStateChanged,
haveStatePropsBeenPrecalculated,
statePropsPrecalculationError,
renderedElement
} = this

this.haveOwnPropsChanged = false
this.hasStoreStateChanged = false
this.haveStatePropsBeenPrecalculated = false
this.statePropsPrecalculationError = null

if (statePropsPrecalculationError) {
throw statePropsPrecalculationError
}

let shouldUpdateStateProps = true
let shouldUpdateDispatchProps = true
Expand Down
80 changes: 80 additions & 0 deletions test/components/connect.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1626,6 +1626,39 @@ describe('React', () => {
spy.destroy()
})

it('should not swallow errors when bailing out early', () => {
const store = createStore(stringBuilder)
let renderCalls = 0
let mapStateCalls = 0

@connect(state => {
mapStateCalls++
if (state === 'a') {
throw new Error('Oops')
} else {
return {}
}
})
class Container extends Component {
render() {
renderCalls++
return <Passthrough {...this.props} />
}
}

TestUtils.renderIntoDocument(
<ProviderMock store={store}>
<Container />
</ProviderMock>
)

expect(renderCalls).toBe(1)
expect(mapStateCalls).toBe(1)
expect(
() => store.dispatch({ type: 'APPEND', body: 'a' })
).toThrow('Oops')
})

it('should allow providing a factory function to mapStateToProps', () => {
let updatedCount = 0
let memoizedReturnCount = 0
Expand Down Expand Up @@ -1788,5 +1821,52 @@ describe('React', () => {
expect(renderCount).toBe(2)
})

it('should allow to clean up child state in parent componentWillUnmount', () => {
function reducer(state = { data: null }, action) {
switch (action.type) {
case 'fetch':
return { data: { profile: { name: 'April' } } }
case 'clean':
return { data: null }
default:
return state
}
}

@connect(null)
class Parent extends React.Component {
componentWillMount() {
this.props.dispatch({ type: 'fetch' })
}

componentWillUnmount() {
this.props.dispatch({ type: 'clean' })
}

render() {
return <Child />
}
}

@connect(state => ({
profile: state.data.profile
}))
class Child extends React.Component {
render() {
return null
}
}

const store = createStore(reducer)
const div = document.createElement('div')
ReactDOM.render(
<ProviderMock store={store}>
<Parent />
</ProviderMock>,
div
)

ReactDOM.unmountComponentAtNode(div)
})
})
})

0 comments on commit ea53d6f

Please sign in to comment.