Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove usages of async-unsafe lifecycle methods #919

Merged
merged 9 commits into from
Jun 21, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ It does not modify the component class passed to it; instead, it *returns* a new

* [`renderCountProp`] *(String)*: if defined, a property named this value will be added to the props passed to the wrapped component. Its value will be the number of times the component has been rendered, which can be useful for tracking down unnecessary re-renders. Default value: `undefined`

* [`shouldHandleStateChanges`] *(Boolean)*: controls whether the connector component subscribes to redux store state changes. If set to false, it will only re-render on `componentWillReceiveProps`. Default value: `true`
* [`shouldHandleStateChanges`] *(Boolean)*: controls whether the connector component subscribes to redux store state changes. If set to false, it will only re-render when parent component re-renders. Default value: `true`

* [`storeKey`] *(String)*: the key of props/context to get the store. You probably only need this if you are in the inadvisable position of having multiple stores. Default value: `'store'`

Expand Down
5 changes: 5 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@
"lodash": "^4.17.5",
"lodash-es": "^4.17.5",
"loose-envify": "^1.1.0",
"prop-types": "^15.6.1"
"prop-types": "^15.6.1",
"react-lifecycles-compat": "^3.0.0"
},
"devDependencies": {
"babel-cli": "^6.26.0",
Expand Down
4 changes: 2 additions & 2 deletions src/components/Provider.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ export function createProvider(storeKey = 'store', subKey) {
}

if (process.env.NODE_ENV !== 'production') {
Provider.prototype.componentWillReceiveProps = function (nextProps) {
if (this[storeKey] !== nextProps.store) {
Provider.prototype.componentDidUpdate = function () {
if (this[storeKey] !== this.props.store) {
warnAboutReceivingStore()
}
}
Expand Down
108 changes: 51 additions & 57 deletions src/components/connectAdvanced.js
Original file line number Diff line number Diff line change
@@ -1,32 +1,34 @@
import hoistStatics from 'hoist-non-react-statics'
import invariant from 'invariant'
import { Component, createElement } from 'react'
import { polyfill } from 'react-lifecycles-compat'

import Subscription from '../utils/Subscription'
import { storeShape, subscriptionShape } from '../utils/PropTypes'

let hotReloadingVersion = 0
const dummyState = {}
function noop() {}
function makeSelectorStateful(sourceSelector, store) {
// wrap the selector in an object that tracks its results between runs.
const selector = {
run: function runComponentSelector(props) {
try {
const nextProps = sourceSelector(store.getState(), props)
if (nextProps !== selector.props || selector.error) {
selector.shouldComponentUpdate = true
selector.props = nextProps
selector.error = null
function makeUpdater(sourceSelector, store) {
return function updater(props, prevState) {
try {
const nextProps = sourceSelector(store.getState(), props)
if (nextProps !== prevState.props || prevState.error) {
return {
shouldComponentUpdate: true,
props: nextProps,
error: null,
}
} catch (error) {
selector.shouldComponentUpdate = true
selector.error = error
}
return {
shouldComponentUpdate: false,
}
} catch (error) {
return {
shouldComponentUpdate: true,
error,
}
}
}

return selector
}

export default function connectAdvanced(
Expand Down Expand Up @@ -86,6 +88,10 @@ export default function connectAdvanced(
[subscriptionKey]: subscriptionShape,
}

function getDerivedStateFromProps(nextProps, prevState) {
return prevState.updater(nextProps, prevState)
}

return function wrapWithConnect(WrappedComponent) {
invariant(
typeof WrappedComponent == 'function',
Expand Down Expand Up @@ -117,7 +123,6 @@ export default function connectAdvanced(
super(props, context)

this.version = version
this.state = {}
this.renderCount = 0
this.store = props[storeKey] || context[storeKey]
this.propsMode = Boolean(props[storeKey])
Expand All @@ -129,7 +134,9 @@ export default function connectAdvanced(
`or explicitly pass "${storeKey}" as a prop to "${displayName}".`
)

this.initSelector()
this.state = {
updater: this.createUpdater()
}
this.initSubscription()
}

Expand All @@ -152,25 +159,19 @@ export default function connectAdvanced(
// dispatching an action in its componentWillMount, we have to re-run the select and maybe
// re-render.
this.subscription.trySubscribe()
this.selector.run(this.props)
if (this.selector.shouldComponentUpdate) this.forceUpdate()
}

componentWillReceiveProps(nextProps) {
this.selector.run(nextProps)
this.runUpdater()
}

shouldComponentUpdate() {
return this.selector.shouldComponentUpdate
shouldComponentUpdate(_, nextState) {
return nextState.shouldComponentUpdate
}

componentWillUnmount() {
if (this.subscription) this.subscription.tryUnsubscribe()
this.subscription = null
this.notifyNestedSubs = noop
this.store = null
this.selector.run = noop
this.selector.shouldComponentUpdate = false
this.isUnmounted = true
}

getWrappedInstance() {
Expand All @@ -185,10 +186,17 @@ export default function connectAdvanced(
this.wrappedInstance = ref
}

initSelector() {
createUpdater() {
const sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions)
this.selector = makeSelectorStateful(sourceSelector, this.store)
this.selector.run(this.props)
return makeUpdater(sourceSelector, this.store)
}

runUpdater(callback = noop) {
if (this.isUnmounted) {
return
}

this.setState(prevState => prevState.updater(this.props, prevState), callback)
}

initSubscription() {
Expand All @@ -209,24 +217,7 @@ export default function connectAdvanced(
}

onStateChange() {
this.selector.run(this.props)

if (!this.selector.shouldComponentUpdate) {
this.notifyNestedSubs()
} else {
this.componentDidUpdate = this.notifyNestedSubsOnComponentDidUpdate
this.setState(dummyState)
}
}

notifyNestedSubsOnComponentDidUpdate() {
// `componentDidUpdate` is conditionally implemented when `onStateChange` determines it
// needs to notify nested subs. Once called, it unimplements itself until further state
// changes occur. Doing it this way vs having a permanent `componentDidUpdate` that does
// a boolean check every time avoids an extra method call most of the time, resulting
// in some perf boost.
this.componentDidUpdate = undefined
this.notifyNestedSubs()
this.runUpdater(this.notifyNestedSubs)
}

isSubscribed() {
Expand All @@ -247,13 +238,10 @@ export default function connectAdvanced(
}

render() {
const selector = this.selector
selector.shouldComponentUpdate = false

if (selector.error) {
throw selector.error
if (this.state.error) {
throw this.state.error
} else {
return createElement(WrappedComponent, this.addExtraProps(selector.props))
return createElement(WrappedComponent, this.addExtraProps(this.state.props))
}
}
}
Expand All @@ -263,13 +251,13 @@ export default function connectAdvanced(
Connect.childContextTypes = childContextTypes
Connect.contextTypes = contextTypes
Connect.propTypes = contextTypes
Connect.getDerivedStateFromProps = getDerivedStateFromProps

if (process.env.NODE_ENV !== 'production') {
Connect.prototype.componentWillUpdate = function componentWillUpdate() {
Connect.prototype.componentDidUpdate = function componentDidUpdate() {
// We are hot reloading!
if (this.version !== version) {
this.version = version
this.initSelector()

// If any connected descendants don't hot reload (and resubscribe in the process), their
// listeners will be lost when we unsubscribe. Unfortunately, by copying over all
Expand All @@ -287,10 +275,16 @@ export default function connectAdvanced(
this.subscription.trySubscribe()
oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener))
}

const updater = this.createUpdater()
this.setState({updater})
this.runUpdater()
}
}
}

polyfill(Connect)

return hoistStatics(Connect, WrappedComponent)
}
}
15 changes: 15 additions & 0 deletions test/components/Provider.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,4 +220,19 @@ describe('React', () => {
store.dispatch({ type: 'APPEND', body: 'd' })
expect(childMapStateInvokes).toBe(4)
})

it('works in <StrictMode> without warnings', () => {
const spy = jest.spyOn(console, 'error').mockImplementation(() => {})
const store = createStore(() => ({}))

TestRenderer.create(
<React.StrictMode>
<Provider store={store}>
<div />
</Provider>
</React.StrictMode>
)

expect(spy).not.toHaveBeenCalled()
})
})