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.

Sign up for GitHub

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

Fix getDerivedStateFromProps usage #980

Closed
wants to merge 20 commits into from
Closed

Fix getDerivedStateFromProps usage #980

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
b631441
up min react versions
cellog Jul 21, 2018
25e3fc6
working, except the issue with multiple mapState calls
cellog Jul 21, 2018
de4e4f3
typo
cellog Jul 21, 2018
f751ac9
success! gDSFP-based react-redux
cellog Jul 22, 2018
9284894
up the peer dependency to React 16.4
cellog Jul 22, 2018
948c20f
remove debugging code
cellog Jul 22, 2018
490b0e3
use polyfill, make peer dependency relaxed
cellog Jul 24, 2018
86e986b
fix style details
cellog Jul 25, 2018
1d96dac
move instance props into state
cellog Aug 1, 2018
26e4fb8
make subscription async-safe
cellog Aug 2, 2018
6b83e13
fully fix subscription
cellog Aug 2, 2018
8a8e151
update subscription change check to be accurate
cellog Aug 2, 2018
52d4f45
rename resolves to be clearer
cellog Aug 3, 2018
3333363
resolve conflicts
cellog Aug 3, 2018
d50fb7d
Merge react-redux master into another-fix-5.1
cellog Aug 3, 2018
1d8ab34
remove console statements
cellog Aug 3, 2018
fe60a4b
fix for React 16-16.3
cellog Aug 3, 2018
72fca1c
fix breaking tests
cellog Aug 3, 2018
1285e4e
remove unnecessary Promise (React lifecycle does this for us)
cellog Aug 3, 2018
35e9d12
fix nitpicks and use a constant instead of function call every connect()
cellog Aug 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 16 additions & 18 deletions package-lock.json
View file
Open in desktop

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

8 changes: 4 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"invariant": "^2.2.4",
"loose-envify": "^1.1.0",
"prop-types": "^15.6.1",
"react-lifecycles-compat": "^3.0.0"
"react-lifecycles-compat": "^3.0.4"
},
"devDependencies": {
"babel-cli": "^6.26.0",
Expand Down Expand Up @@ -86,9 +86,9 @@
"eslint-plugin-react": "^7.9.1",
"glob": "^7.1.1",
"jest": "^23.1.0",
"react": "^16.3.2",
"react-dom": "^16.3.2",
"react-test-renderer": "^16.3.2",
"react": "^16.4.1",
"react-dom": "^16.4.1",
"react-test-renderer": "^16.4.1",
"redux": "^4.0.0",
"rimraf": "^2.6.2",
"rollup": "^0.61.1",
Expand Down
114 changes: 69 additions & 45 deletions src/components/connectAdvanced.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,34 +2,13 @@ import hoistStatics from 'hoist-non-react-statics'
import invariant from 'invariant'
import { Component, createElement } from 'react'
import { polyfill } from 'react-lifecycles-compat'
import shallowEqual from '../utils/shallowEqual'

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

let hotReloadingVersion = 0
function noop() {}
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,
}
}
return {
shouldComponentUpdate: false,
}
} catch (error) {
return {
shouldComponentUpdate: true,
error,
}
}
}
}

export default function connectAdvanced(
/*
Expand Down Expand Up @@ -88,10 +67,6 @@ 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 @@ -124,22 +99,52 @@ export default function connectAdvanced(

this.version = version
this.renderCount = 0
this.store = props[storeKey] || context[storeKey]
const store = props[storeKey] || context[storeKey]
this.propsMode = Boolean(props[storeKey])
this.setWrappedInstance = this.setWrappedInstance.bind(this)

invariant(this.store,
invariant(store,
`Could not find "${storeKey}" in either the context or props of ` +
`"${displayName}". Either wrap the root component in a <Provider>, ` +
`or explicitly pass "${storeKey}" as a prop to "${displayName}".`
)
const storeState = store.getState()

const childPropsSelector = this.createChildSelector(store)
this.state = {
props,
childPropsSelector,
childProps: {},
store,
storeState,
error: null,
}
this.state = {
updater: this.createUpdater()
...this.state,
...Connect.getChildPropsState(props, this.state)
}
this.initSubscription()
}

static getChildPropsState(props, state) {
try {
const nextProps = state.childPropsSelector(state.store.getState(), props)
Copy link
Contributor

@markerikson markerikson Aug 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the store state here is still my biggest concern, async-wise, because it seems vulnerable to the "time-slicing" behavior. But, I guess we can go with it until we get more examples from the React team for what proper patterns look like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is that if we don't do this, then we will always call mapStateToProps twice, once for props change, once for redux state change, whenever an action is dispatched. This is because when an update occurs, it follows this path:

Parent <- redux
Parent updates state to store new redux state
Parent gDSFP called
Parent props change, 
Parent render called, renders children
Child <- props
child gDSFP called
...
Child <- redux

This is because the parent call to update child subscriptions happens in the getState callback, which is not called until the entire tree has been rendered (at least in the tests). Basically, we can't rely on the subscription happening at a deterministic moment, and so the only way to get a non-stale redux state is to retrieve it just-in-time when a props change comes in.

I don't think this will be a problem, because in the worst case (as I understand it), a calculation might get thrown out and re-started. In other words, I don't think it is possible for a major delay between gDSFP and render, because if rendering is suspended, and redux state changes, we will get an async event that calls setState, which will re-trigger gDSFP when rendering resumes.

In addition, if props change after a delay, gDSFP will have to be called again prior to rendering. The worst case is that the stored redux state is stale, which is OK, because if an action is dispatched, we will compare the stale state to the actual state, and determine that we should re-run mSP to see if derived props have changed. So we won't ever be incorrect, just possibly slightly inefficient. But since we are talking about a single extra call to mSP over the course of many seconds, it should not be significant for all but the most unusual cases where the redux state is updating constantly and a large number of components are rendering in response to changes.

So I put this in because in my reasoning, it saves us from constantly calling mSP unnecessarily twice as often, at the expense of occasionally calling it twice after a long delay due to suspense (as in seconds), where performance will not be a noticeable issue.

The next thing is to start playing with the suspense demo and see if we can get it to break the expectations I laid out above

if (nextProps === state.childProps) return null
return { childProps: nextProps }
} catch (error) {
return { error }
}
}

static getDerivedStateFromProps(props, state) {
if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add parentheses around the pure && shallowEqual() bit just to clarify the comparison?

const nextChildProps = Connect.getChildPropsState(props, state)
return {
...nextChildProps,
props
}
}

getChildContext() {
// If this component received store from props, its subscription should be transparent
// to any descendants receiving store+subscription from context; it passes along
Expand All @@ -159,11 +164,14 @@ 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.runUpdater()
this.updateChildPropsFromReduxStore(false)
}

shouldComponentUpdate(_, nextState) {
return nextState.shouldComponentUpdate
if (!connectOptions['pure']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason for the bracketed field access here? Pretty sure you should be able to dot-access it - that would only be an issue if connectOptions was undefined, which it won't be.

return true
}
return nextState.childProps !== this.state.childProps || nextState.error
}

componentWillUnmount() {
Expand All @@ -186,17 +194,35 @@ export default function connectAdvanced(
this.wrappedInstance = ref
}

createUpdater() {
const sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions)
return makeUpdater(sourceSelector, this.store)
createChildSelector(store = this.state.store) {
return selectorFactory(store.dispatch, selectorFactoryOptions)
}

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

this.setState(prevState => prevState.updater(this.props, prevState), callback)
this.setState(prevState => {
const nextState = this.state.store.getState()
if (nextState === prevState.storeState) {
return null
}
const childPropsState = Connect.getChildPropsState(this.props, {
...this.state,
storeState: nextState
})
const ret = {
storeState: nextState,
...childPropsState
}
if (notify && childPropsState === null) {
this.notifyNestedSubs()
}
return ret
}, () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could change this to notify ? this.notifyNestedSubs : noop .

if (notify) this.notifyNestedSubs()
})
}

initSubscription() {
Expand All @@ -205,7 +231,7 @@ export default function connectAdvanced(
// parentSub's source should match where store came from: props vs. context. A component
// connected to the store via props shouldn't use subscription from context, or vice versa.
const parentSub = (this.propsMode ? this.props : this.context)[subscriptionKey]
this.subscription = new Subscription(this.store, parentSub, this.onStateChange.bind(this))
this.subscription = new Subscription(this.state.store, parentSub, this.onStateChange.bind(this))

// `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in
// the middle of the notification loop, where `this.subscription` will then be null. An
Expand All @@ -217,7 +243,7 @@ export default function connectAdvanced(
}

onStateChange() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all this does is call updateChildPropsFromReduxStore(), we could just pass that to the Subscription instead.

this.runUpdater(this.notifyNestedSubs)
this.updateChildPropsFromReduxStore()
}

isSubscribed() {
Expand All @@ -241,7 +267,7 @@ export default function connectAdvanced(
if (this.state.error) {
throw this.state.error
} else {
return createElement(WrappedComponent, this.addExtraProps(this.state.props))
return createElement(WrappedComponent, this.addExtraProps(this.state.childProps))
}
}
}
Expand All @@ -251,7 +277,7 @@ export default function connectAdvanced(
Connect.childContextTypes = childContextTypes
Connect.contextTypes = contextTypes
Connect.propTypes = contextTypes
Connect.getDerivedStateFromProps = getDerivedStateFromProps
polyfill(Connect)

if (process.env.NODE_ENV !== 'production') {
Connect.prototype.componentDidUpdate = function componentDidUpdate() {
Expand All @@ -276,15 +302,13 @@ export default function connectAdvanced(
oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener))
}

const updater = this.createUpdater()
this.setState({updater})
this.runUpdater()
const childPropsSelector = this.createChildSelector()
const childProps = childPropsSelector(this.props, this.state.storeState)
this.setState({ childPropsSelector, childProps })
}
}
}

polyfill(Connect)

return hoistStatics(Connect, WrappedComponent)
}
}
2 changes: 1 addition & 1 deletion test/components/connect.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2362,7 +2362,7 @@ describe('React', () => {
)

const container = testRenderer.root.findByType(Container)
expect(container.instance.store).toBe(store)
expect(container.instance.state.store).toBe(store)
})
})
})