diff --git a/.eslintrc b/.eslintrc index 05527f778..7b11d5141 100644 --- a/.eslintrc +++ b/.eslintrc @@ -16,7 +16,8 @@ "env": { "browser": true, "mocha": true, - "node": true + "node": true, + "es6": true }, "rules": { "valid-jsdoc": 2, diff --git a/package-lock.json b/package-lock.json index 347944139..1be86e993 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2441,6 +2441,20 @@ "prop-types": "^15.6.0", "react-reconciler": "^0.7.0", "react-test-renderer": "^16.0.0-0" + }, + "dependencies": { + "react-test-renderer": { + "version": "16.4.2", + "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.4.2.tgz", + "integrity": "sha512-vdTPnRMDbxfv4wL4lzN4EkVGXyYs7LE2uImOsqh1FKiP6L5o1oJl8nore5sFi9vxrP9PK3l4rgb/fZ4PVUaWSA==", + "dev": true, + "requires": { + "fbjs": "^0.8.16", + "object-assign": "^4.1.1", + "prop-types": "^15.6.0", + "react-is": "^16.4.2" + } + } } }, "enzyme-adapter-utils": { @@ -3161,8 +3175,7 @@ "ansi-regex": { "version": "2.1.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "aproba": { "version": "1.1.1", @@ -3228,7 +3241,6 @@ "version": "0.0.9", "bundled": true, "dev": true, - "optional": true, "requires": { "inherits": "~2.0.0" } @@ -3237,7 +3249,6 @@ "version": "2.10.1", "bundled": true, "dev": true, - "optional": true, "requires": { "hoek": "2.x.x" } @@ -3254,8 +3265,7 @@ "buffer-shims": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "caseless": { "version": "0.12.0", @@ -3272,14 +3282,12 @@ "code-point-at": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "combined-stream": { "version": "1.0.5", "bundled": true, "dev": true, - "optional": true, "requires": { "delayed-stream": "~1.0.0" } @@ -3292,20 +3300,17 @@ "console-control-strings": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "core-util-is": { "version": "1.0.2", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "cryptiles": { "version": "2.0.5", "bundled": true, "dev": true, - "optional": true, "requires": { "boom": "2.x.x" } @@ -3345,8 +3350,7 @@ "delayed-stream": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "delegates": { "version": "1.0.0", @@ -3378,8 +3382,7 @@ "extsprintf": { "version": "1.0.2", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "forever-agent": { "version": "0.6.1", @@ -3407,7 +3410,6 @@ "version": "1.0.11", "bundled": true, "dev": true, - "optional": true, "requires": { "graceful-fs": "^4.1.2", "inherits": "~2.0.0", @@ -3475,8 +3477,7 @@ "graceful-fs": { "version": "4.1.11", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "har-schema": { "version": "1.0.5", @@ -3504,7 +3505,6 @@ "version": "3.1.3", "bundled": true, "dev": true, - "optional": true, "requires": { "boom": "2.x.x", "cryptiles": "2.x.x", @@ -3552,7 +3552,6 @@ "version": "1.0.0", "bundled": true, "dev": true, - "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -3566,8 +3565,7 @@ "isarray": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "isstream": { "version": "0.1.2", @@ -3640,14 +3638,12 @@ "mime-db": { "version": "1.27.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "mime-types": { "version": "2.1.15", "bundled": true, "dev": true, - "optional": true, "requires": { "mime-db": "~1.27.0" } @@ -3663,14 +3659,12 @@ "minimist": { "version": "0.0.8", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "mkdirp": { "version": "0.5.1", "bundled": true, "dev": true, - "optional": true, "requires": { "minimist": "0.0.8" } @@ -3725,8 +3719,7 @@ "number-is-nan": { "version": "1.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "oauth-sign": { "version": "0.8.2", @@ -3784,8 +3777,7 @@ "process-nextick-args": { "version": "1.0.7", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "punycode": { "version": "1.4.1", @@ -3823,7 +3815,6 @@ "version": "2.2.9", "bundled": true, "dev": true, - "optional": true, "requires": { "buffer-shims": "~1.0.0", "core-util-is": "~1.0.0", @@ -3875,8 +3866,7 @@ "safe-buffer": { "version": "5.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "semver": { "version": "5.3.0", @@ -3900,7 +3890,6 @@ "version": "1.0.9", "bundled": true, "dev": true, - "optional": true, "requires": { "hoek": "2.x.x" } @@ -3934,7 +3923,6 @@ "version": "1.0.2", "bundled": true, "dev": true, - "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -3945,7 +3933,6 @@ "version": "1.0.1", "bundled": true, "dev": true, - "optional": true, "requires": { "safe-buffer": "^5.0.1" } @@ -3960,7 +3947,6 @@ "version": "3.0.1", "bundled": true, "dev": true, - "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -3975,7 +3961,6 @@ "version": "2.2.1", "bundled": true, "dev": true, - "optional": true, "requires": { "block-stream": "*", "fstream": "^1.0.2", @@ -4031,8 +4016,7 @@ "util-deprecate": { "version": "1.0.2", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "uuid": { "version": "3.0.1", @@ -7021,9 +7005,9 @@ } }, "react": { - "version": "16.3.2", - "resolved": "https://registry.npmjs.org/react/-/react-16.3.2.tgz", - "integrity": "sha512-o5GPdkhciQ3cEph6qgvYB7LTOHw/GB0qRI6ZFNugj49qJCFfgHwVNjZ5u+b7nif4vOeMIOuYj3CeYe2IBD74lg==", + "version": "16.4.2", + "resolved": "https://registry.npmjs.org/react/-/react-16.4.2.tgz", + "integrity": "sha512-dMv7YrbxO4y2aqnvA7f/ik9ibeLSHQJTI6TrYAenPSaQ6OXfb+Oti+oJiy8WBxgRzlKatYqtCjphTgDSCEiWFg==", "dev": true, "requires": { "fbjs": "^0.8.16", @@ -7033,9 +7017,9 @@ } }, "react-dom": { - "version": "16.3.2", - "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.3.2.tgz", - "integrity": "sha512-MMPko3zYncNrz/7gG17wJWUREZDvskZHXOwbttzl0F0L3wDmToyuETuo/r8Y5yvDejwYcRyWI1lvVBjLJWFwKA==", + "version": "16.4.2", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.4.2.tgz", + "integrity": "sha512-Usl73nQqzvmJN+89r97zmeUpQDKDlh58eX6Hbs/ERdDHzeBzWy+ENk7fsGQ+5KxArV1iOFPT46/VneklK9zoWw==", "dev": true, "requires": { "fbjs": "^0.8.16", @@ -7045,9 +7029,9 @@ } }, "react-is": { - "version": "16.4.1", - "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.4.1.tgz", - "integrity": "sha512-xpb0PpALlFWNw/q13A+1aHeyJyLYCg0/cCHPUA43zYluZuIPHaHL3k8OBsTgQtxqW0FhyDEMvi8fZ/+7+r4OSQ==", + "version": "16.4.2", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.4.2.tgz", + "integrity": "sha512-rI3cGFj/obHbBz156PvErrS5xc6f1eWyTwyV4mo0vF2lGgXgS+mm7EKD5buLJq6jNgIagQescGSVG2YzgXt8Yg==", "dev": true }, "react-lifecycles-compat": { @@ -7068,15 +7052,15 @@ } }, "react-test-renderer": { - "version": "16.3.2", - "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.3.2.tgz", - "integrity": "sha512-lL8WHIpCTMdSe+CRkt0rfMxBkJFyhVrpdQ54BaJRIrXf9aVmbeHbRA8GFRpTvohPN5tPzMabmrzW2PUfWCfWwQ==", + "version": "16.4.2", + "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.4.2.tgz", + "integrity": "sha512-vdTPnRMDbxfv4wL4lzN4EkVGXyYs7LE2uImOsqh1FKiP6L5o1oJl8nore5sFi9vxrP9PK3l4rgb/fZ4PVUaWSA==", "dev": true, "requires": { "fbjs": "^0.8.16", "object-assign": "^4.1.1", "prop-types": "^15.6.0", - "react-is": "^16.3.2" + "react-is": "^16.4.2" } }, "read-pkg": { diff --git a/package.json b/package.json index 2237fee36..bccafc968 100644 --- a/package.json +++ b/package.json @@ -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", @@ -90,9 +90,9 @@ "glob": "^7.1.1", "jest": "^23.4.1", "npm-run": "^5.0.1", - "react": "^16.3.2", - "react-dom": "^16.3.2", - "react-test-renderer": "^16.3.2", + "react": "^16.4.2", + "react-dom": "^16.4.2", + "react-test-renderer": "^16.4.2", "redux": "^4.0.0", "rimraf": "^2.6.2", "rollup": "^0.61.1", diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 1a5faa625..f07175dd7 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -1,35 +1,22 @@ import hoistStatics from 'hoist-non-react-statics' import invariant from 'invariant' -import { Component, createElement } from 'react' +import React, { 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, - } - } - } + +function isOldReact() { + const version = React.version.split('.') + if (+version[0] < 16) return true + if (+version[0] > 16) return false + return +version[1] < 4 } +const oldReact = isOldReact() export default function connectAdvanced( /* @@ -88,10 +75,6 @@ export default function connectAdvanced( [subscriptionKey]: subscriptionShape, } - function getDerivedStateFromProps(nextProps, prevState) { - return prevState.updater(nextProps, prevState) - } - return function wrapWithConnect(WrappedComponent) { invariant( typeof WrappedComponent == 'function', @@ -124,20 +107,64 @@ 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 , ` + `or explicitly pass "${storeKey}" as a prop to "${displayName}".` ) - + const storeState = store.getState() + const subscription = (this.propsMode ? null : context[subscriptionKey]) + const childPropsSelector = this.createChildSelector(store) this.state = { - updater: this.createUpdater() + props, + childPropsSelector, + childProps: {}, + store, + storeState, + error: null, + subscription: new Subscription(store, subscription, this.updateChildPropsFromReduxStore.bind(this)), + lastNotify: noop, + notifyNestedSubs: noop + } + this.state = { + ...this.state, + ...Connect.getChildPropsState(props, this.state) + } + } + + static getChildPropsState(props, state) { + try { + const nextProps = state.childPropsSelector(state.store.getState(), props) + if (nextProps === state.childProps) return null + return { childProps: nextProps } + } catch (error) { + return { error } + } + } + + static getDerivedStateFromProps(props, state) { + let ret = null + // this next check only should trigger if gDSFP reacts to state changes + // in React 16.3, it doesn't. react-lifecycles-compat matches React 16.3 behavior + // if a future version of react-lifecycles-compat DOES trigger on state changes + // the oldReact check must be removed + if (!oldReact && state.lastNotify !== state.notifyNestedSubs) { + ret = { lastNotify: state.notifyNestedSubs } + if (shallowEqual(props, state.props) || state.error) { + return ret + } + } + if ((connectOptions.pure && shallowEqual(props, state.props)) || state.error) return ret + const nextChildProps = Connect.getChildPropsState(props, state) + return { + ...ret, + ...nextChildProps, + props } - this.initSubscription() } getChildContext() { @@ -145,33 +172,52 @@ export default function connectAdvanced( // to any descendants receiving store+subscription from context; it passes along // subscription passed to it. Otherwise, it shadows the parent subscription, which allows // Connect to control ordering of notifications to flow top-down. - const subscription = this.propsMode ? null : this.subscription + const subscription = this.propsMode ? null : this.state.subscription return { [subscriptionKey]: subscription || this.context[subscriptionKey] } } componentDidMount() { - if (!shouldHandleStateChanges) return - - // componentWillMount fires during server side rendering, but componentDidMount and - // componentWillUnmount do not. Because of this, trySubscribe happens during ...didMount. - // Otherwise, unsubscription would never take place during SSR, causing a memory leak. - // To handle the case where a child component may have triggered a state change by - // dispatching an action in its componentWillMount, we have to re-run the select and maybe - // re-render. - this.subscription.trySubscribe() - this.runUpdater() + return this.updateSubscription() } shouldComponentUpdate(_, nextState) { - return nextState.shouldComponentUpdate + if (!connectOptions.pure) { + return true + } + return nextState.childProps !== this.state.childProps || nextState.error } componentWillUnmount() { - if (this.subscription) this.subscription.tryUnsubscribe() - this.subscription = null - this.notifyNestedSubs = noop - this.store = null + if (this.state.subscription) this.state.subscription.tryUnsubscribe() this.isUnmounted = true + this.setState({ + store: null, + notifyNestedSubs: noop + }) + } + + updateSubscription(hotReloadCallback) { + if (!shouldHandleStateChanges) return + + this.setState(state => { + if (state.subscription.isReady() && !hotReloadCallback) return null + this.state.subscription.hydrate() + return { + // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in + // the middle of the notification loop, where `this.state.subscription` will then be null. An + // extra null check every change can be avoided by copying the method onto `this` and then + // replacing it with a no-op on unmount. This can probably be avoided if Subscription's + // listeners logic is changed to not call listeners that have been unsubscribed in the + // middle of the notification loop. + notifyNestedSubs: state.subscription.notifyNestedSubs.bind(this.state.subscription), + lastNotify: state.notifyNestedSubs + } + }, () => { + if (hotReloadCallback) { + hotReloadCallback() + } + this.updateChildPropsFromReduxStore(false) + }) } getWrappedInstance() { @@ -186,46 +232,43 @@ 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) - } - - initSubscription() { - if (!shouldHandleStateChanges) return - - // 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)) - - // `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 - // extra null check every change can be avoided by copying the method onto `this` and then - // replacing it with a no-op on unmount. This can probably be avoided if Subscription's - // listeners logic is changed to not call listeners that have been unsubscribed in the - // middle of the notification loop. - this.notifyNestedSubs = this.subscription.notifyNestedSubs.bind(this.subscription) - } - - onStateChange() { - this.runUpdater(this.notifyNestedSubs) + 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.state.notifyNestedSubs() + } + return ret + }, () => { + if (notify) this.state.notifyNestedSubs() + }) } isSubscribed() { - return Boolean(this.subscription) && this.subscription.isSubscribed() + return Boolean(this.state.subscription) && this.state.subscription.isSubscribed() } addExtraProps(props) { - if (!withRef && !renderCountProp && !(this.propsMode && this.subscription)) return props + if (!withRef && !renderCountProp && !(this.propsMode && this.state.subscription)) return props // make a shallow copy so that fields added don't leak to the original selector. // this is especially important for 'ref' since that's a reference back to the component // instance. a singleton memoized selector would then be holding a reference to the @@ -233,7 +276,7 @@ export default function connectAdvanced( const withExtras = { ...props } if (withRef) withExtras.ref = this.setWrappedInstance if (renderCountProp) withExtras[renderCountProp] = this.renderCount++ - if (this.propsMode && this.subscription) withExtras[subscriptionKey] = this.subscription + if (this.propsMode && this.state.subscription) withExtras[subscriptionKey] = this.state.subscription return withExtras } @@ -241,7 +284,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)) } } } @@ -251,7 +294,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() { @@ -262,29 +305,31 @@ export default function connectAdvanced( // 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 // listeners, this does mean that the old versions of connected descendants will still be - // notified of state changes; however, their onStateChange function is a no-op so this + // notified of state changes; however, their updateChildPropsFromReduxStore function is a no-op so this // isn't a huge deal. let oldListeners = []; - if (this.subscription) { - oldListeners = this.subscription.listeners.get() - this.subscription.tryUnsubscribe() + if (this.state.subscription) { + if (this.state.subscription.listeners.get) { + // only retrieve listeners if subscription has completed. If hot reload occurs immediately + // the subscription has not yet happened, so we don't need to retrieve old listeners + oldListeners = this.state.subscription.listeners.get() + } + this.state.subscription.tryUnsubscribe() } - this.initSubscription() if (shouldHandleStateChanges) { - this.subscription.trySubscribe() - oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener)) + this.updateSubscription(() => { + oldListeners.forEach(listener => this.state.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) } } diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index db8146799..24635f60f 100644 --- a/src/utils/Subscription.js +++ b/src/utils/Subscription.js @@ -44,6 +44,11 @@ function createListenerCollection() { } } +// subscriptions are set up in componentDidMount, so we need to delay the actual subscribing until then. +// To do this, the subscription object is created but does not subscribe until the parent is ready +// in cDM, a parent calls "hydrate()" which triggers the resolution of the promise. If +// a child component tries to subscribe before this point, it will wait for the parent subscription +// to be ready and then try to subscribe. export default class Subscription { constructor(store, parentSub, onStateChange) { this.store = store @@ -51,6 +56,22 @@ export default class Subscription { this.onStateChange = onStateChange this.unsubscribe = null this.listeners = nullListeners + this.loaded = false + } + + markReady() { + this.loaded = true + } + + hydrate() { + if (!this.loaded) { + this.markReady() + } + this.trySubscribe() + } + + isReady() { + return this.loaded } addNestedSub(listener) { @@ -66,13 +87,21 @@ export default class Subscription { return Boolean(this.unsubscribe) } + subscribeToParent() { + this.unsubscribe = this.parentSub.addNestedSub(this.onStateChange) + this.listeners = createListenerCollection() + } + trySubscribe() { if (!this.unsubscribe) { - this.unsubscribe = this.parentSub - ? this.parentSub.addNestedSub(this.onStateChange) - : this.store.subscribe(this.onStateChange) - - this.listeners = createListenerCollection() + if (this.parentSub) { + if (this.parentSub.isReady()) { + return this.subscribeToParent() + } + } else { + this.unsubscribe = this.store.subscribe(this.onStateChange) + this.listeners = createListenerCollection() + } } } diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 9d2c82822..f023a4b9c 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -2378,9 +2378,8 @@ describe('React', () => { ) - const container = testRenderer.find(Container) - expect(container.instance().store).toBe(store) + expect(container.instance().state.store).toBe(store) }) }) })