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

Jump to bottom

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 6 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 15 additions & 22 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.

11 changes: 5 additions & 6 deletions package.json
View file
Open in desktop
Expand Up @@ -39,15 +39,14 @@
"coverage": "codecov"
},
"peerDependencies": {
"react": "^0.14.0 || ^15.0.0-0 || ^16.0.0-0",
"react": "^16.4.0-0",
Copy link
Member

Choose a reason for hiding this comment

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

We're not going to break React compat in this version yet. We need to still support <=16.2, otherwise this is a major revision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#965 (comment) Could you and @markerikson decide what you want and let me know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(point of clarification) Because gDSFP changed in React 16.4, the only way to support both is to have parallel implementations, and a runtime check when connectAdvanced is loaded, something like this pseudo-code:

import React from 'react'

const parts = React.version.split('.')
if (parts[0] + 0 < 16) {
  module.exports = require('old/connectAdvanced')
} else if (parts[0] === '16' && parts[1] + 0 <= 3) {
  module.exports = require('new5.1/connectAdvanced')
} else if (parts[0] === '16' && parts[1] + 0 >= 4 || parts[0] + 0 > 16) {
  module.exports = require('new5.2/connectAdvanced')
}

Or you could use the 5.1 (current test.1 version) with the polyfill for the older React versions.

Copy link
Member

Choose a reason for hiding this comment

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

gDSFP changed in 16.4 to fire more often (specifically on any update, not just ones from props changes), so it should be compatible with 16.3 and react-lifecycle-compat. If it's not, then this still suffers from a buggy or improper implementation.

One thing that might of use is in #856, which makes use of a setState callback that returns null to cancel the update. I might not take that PR entirely verbatim, but the concept may be helpful to the implementation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because gDSFP has no way of knowing whether an update comes from a state change or from a props change, there is no way to support receiving both props and state (redux changes) that works in both 16.3 and 16.4.

I'm baffled as to why you would think it necessary to explain how the gDSFP API changed as if it wasn't the core of the original bug report this PR and my prior version addresses. Perhaps you're not reading the PR carefully enough?

Copy link
Member

Choose a reason for hiding this comment

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

You certainly can know and you've implemented such a check. this.state.props is essentially prevProps, which you can check against nextProps to determine the source.

Or, if you wanted to do it differently, you can add something to the setState (which comes through with prevState) to signal that this is a state-based update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, but I need to make a pointed remark. Please don't take it personally, I am merely pointing out an inefficiency in our communication which I would love to see remedied so we can actually solve this problem.

I don't believe you've actually taken any substantive time to look at this pull request before dismissing it, because it literally does everything you have just described. For instance, here is the source code to gDSFP:

      static getDerivedStateFromProps(props, state) {
        if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null
        const nextChildProps = Connect.getChildPropsState(props, state)
        return nextChildProps
      }

Note the comparison of props to state.props.

I misspoke and did not check to see if the patch worked in 16.0-16.3, which would have saved some time as well.

So, I just tried the pull request in React 16.3, and React 16.0 and the latest React 15. All tests except the strict mode test (which only exists in 16.3+) pass in all React 16 versions. In React 15, all the tests fail because the test renderer isn't working to return the root, and that is with the polyfill. I haven't done the research to see why that is happening, but it is an issue with the tests. I also haven't taken the time yet to see if the thing works in a test project even though all the tests fail.

So the main issue is that the peer dependency in package.json is too restrictive, and can be relaxed to include React 16+, and possibly 15, if the issue with the tests can be determined to be only in the tests.

"redux": "^2.0.0 || ^3.0.0 || ^4.0.0-0"
},
"dependencies": {
"hoist-non-react-statics": "^2.5.5",
"invariant": "^2.2.4",
"loose-envify": "^1.1.0",
"prop-types": "^15.6.1",
"react-lifecycles-compat": "^3.0.0"
"prop-types": "^15.6.1"
},
"devDependencies": {
"babel-cli": "^6.26.0",
Expand Down Expand Up @@ -86,9 +85,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
111 changes: 65 additions & 46 deletions src/components/connectAdvanced.js
View file
Open in desktop
@@ -1,35 +1,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 +66,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 +98,49 @@ 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 = {
updater: this.createUpdater()
props,
childPropsSelector,
childProps: {},
store,
storeState,
error: null,
}
this.state = {
...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

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
}

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 +160,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 +190,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 +227,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 +239,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 +263,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 +273,6 @@ export default function connectAdvanced(
Connect.childContextTypes = childContextTypes
Connect.contextTypes = contextTypes
Connect.propTypes = contextTypes
Connect.getDerivedStateFromProps = getDerivedStateFromProps

if (process.env.NODE_ENV !== 'production') {
Connect.prototype.componentDidUpdate = function componentDidUpdate() {
Expand All @@ -276,15 +297,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
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)
})
})
})