From 0b04e67ace4025641fadb50df5e7304b2d0531a2 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Sun, 17 Mar 2019 22:54:26 -0400 Subject: [PATCH 01/39] Update React to latest --- package-lock.json | 26 +++++++++++++------------- package.json | 4 ++-- test/react/16.8/package-lock.json | 28 ++++++++++++++-------------- test/react/16.8/package.json | 4 ++-- 4 files changed, 31 insertions(+), 31 deletions(-) diff --git a/package-lock.json b/package-lock.json index d2a5137eb..0917c93b9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9026,27 +9026,27 @@ "dev": true }, "react": { - "version": "16.8.2", - "resolved": "https://registry.npmjs.org/react/-/react-16.8.2.tgz", - "integrity": "sha512-aB2ctx9uQ9vo09HVknqv3DGRpI7OIGJhCx3Bt0QqoRluEjHSaObJl+nG12GDdYH6sTgE7YiPJ6ZUyMx9kICdXw==", + "version": "16.8.4", + "resolved": "https://registry.npmjs.org/react/-/react-16.8.4.tgz", + "integrity": "sha512-0GQ6gFXfUH7aZcjGVymlPOASTuSjlQL4ZtVC5YKH+3JL6bBLCVO21DknzmaPlI90LN253ojj02nsapy+j7wIjg==", "dev": true, "requires": { "loose-envify": "^1.1.0", "object-assign": "^4.1.1", "prop-types": "^15.6.2", - "scheduler": "^0.13.2" + "scheduler": "^0.13.4" } }, "react-dom": { - "version": "16.8.2", - "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.8.2.tgz", - "integrity": "sha512-cPGfgFfwi+VCZjk73buu14pYkYBR1b/SRMSYqkLDdhSEHnSwcuYTPu6/Bh6ZphJFIk80XLvbSe2azfcRzNF+Xg==", + "version": "16.8.4", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.8.4.tgz", + "integrity": "sha512-Ob2wK7XG2tUDt7ps7LtLzGYYB6DXMCLj0G5fO6WeEICtT4/HdpOi7W/xLzZnR6RCG1tYza60nMdqtxzA8FaPJQ==", "dev": true, "requires": { "loose-envify": "^1.1.0", "object-assign": "^4.1.1", "prop-types": "^15.6.2", - "scheduler": "^0.13.2" + "scheduler": "^0.13.4" } }, "react-is": { @@ -9551,7 +9551,7 @@ "dependencies": { "jsesc": { "version": "0.5.0", - "resolved": "http://registry.npmjs.org/jsesc/-/jsesc-0.5.0.tgz", + "resolved": "https://registry.npmjs.org/jsesc/-/jsesc-0.5.0.tgz", "integrity": "sha1-597mbjXW/Bb3EP6R1c9p9w8IkR0=", "dev": true } @@ -10188,7 +10188,7 @@ }, "minimist": { "version": "1.2.0", - "resolved": "http://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz", + "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.0.tgz", "integrity": "sha1-o1AIsg9BOD7sH7kU9M1d95omQoQ=", "dev": true } @@ -10201,9 +10201,9 @@ "dev": true }, "scheduler": { - "version": "0.13.2", - "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.13.2.tgz", - "integrity": "sha512-qK5P8tHS7vdEMCW5IPyt8v9MJOHqTrOUgPXib7tqm9vh834ibBX5BNhwkplX/0iOzHW5sXyluehYfS9yrkz9+w==", + "version": "0.13.4", + "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.13.4.tgz", + "integrity": "sha512-cvSOlRPxOHs5dAhP9yiS/6IDmVAVxmk33f0CtTJRkmUWcb1Us+t7b1wqdzoC0REw2muC9V5f1L/w5R5uKGaepA==", "dev": true, "requires": { "loose-envify": "^1.1.0", diff --git a/package.json b/package.json index 37843b7d7..62f150ab5 100644 --- a/package.json +++ b/package.json @@ -74,8 +74,8 @@ "jest-dom": "^3.1.2", "npm-run": "^5.0.1", "prettier": "^1.16.4", - "react": "^16.8.2", - "react-dom": "^16.8.2", + "react": "^16.8.4", + "react-dom": "^16.8.4", "react-testing-library": "^5.9.0", "redux": "^4.0.1", "rimraf": "^2.6.3", diff --git a/test/react/16.8/package-lock.json b/test/react/16.8/package-lock.json index df461400e..5e00a715c 100644 --- a/test/react/16.8/package-lock.json +++ b/test/react/16.8/package-lock.json @@ -293,33 +293,33 @@ } }, "react": { - "version": "16.8.2", - "resolved": "https://registry.npmjs.org/react/-/react-16.8.2.tgz", - "integrity": "sha512-aB2ctx9uQ9vo09HVknqv3DGRpI7OIGJhCx3Bt0QqoRluEjHSaObJl+nG12GDdYH6sTgE7YiPJ6ZUyMx9kICdXw==", + "version": "16.8.4", + "resolved": "https://registry.npmjs.org/react/-/react-16.8.4.tgz", + "integrity": "sha512-0GQ6gFXfUH7aZcjGVymlPOASTuSjlQL4ZtVC5YKH+3JL6bBLCVO21DknzmaPlI90LN253ojj02nsapy+j7wIjg==", "dev": true, "requires": { "loose-envify": "^1.1.0", "object-assign": "^4.1.1", "prop-types": "^15.6.2", - "scheduler": "^0.13.2" + "scheduler": "^0.13.4" } }, "react-dom": { - "version": "16.8.2", - "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.8.2.tgz", - "integrity": "sha512-cPGfgFfwi+VCZjk73buu14pYkYBR1b/SRMSYqkLDdhSEHnSwcuYTPu6/Bh6ZphJFIk80XLvbSe2azfcRzNF+Xg==", + "version": "16.8.4", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.8.4.tgz", + "integrity": "sha512-Ob2wK7XG2tUDt7ps7LtLzGYYB6DXMCLj0G5fO6WeEICtT4/HdpOi7W/xLzZnR6RCG1tYza60nMdqtxzA8FaPJQ==", "dev": true, "requires": { "loose-envify": "^1.1.0", "object-assign": "^4.1.1", "prop-types": "^15.6.2", - "scheduler": "^0.13.2" + "scheduler": "^0.13.4" } }, "react-is": { - "version": "16.8.2", - "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.8.2.tgz", - "integrity": "sha512-D+NxhSR2HUCjYky1q1DwpNUD44cDpUXzSmmFyC3ug1bClcU/iDNy0YNn1iwme28fn+NFhpA13IndOd42CrFb+Q==", + "version": "16.8.4", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.8.4.tgz", + "integrity": "sha512-PVadd+WaUDOAciICm/J1waJaSvgq+4rHE/K70j0PFqKhkTBsPv/82UGQJNXAngz1fOQLLxI6z1sEDmJDQhCTAA==", "dev": true }, "react-testing-library": { @@ -357,9 +357,9 @@ "dev": true }, "scheduler": { - "version": "0.13.2", - "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.13.2.tgz", - "integrity": "sha512-qK5P8tHS7vdEMCW5IPyt8v9MJOHqTrOUgPXib7tqm9vh834ibBX5BNhwkplX/0iOzHW5sXyluehYfS9yrkz9+w==", + "version": "0.13.4", + "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.13.4.tgz", + "integrity": "sha512-cvSOlRPxOHs5dAhP9yiS/6IDmVAVxmk33f0CtTJRkmUWcb1Us+t7b1wqdzoC0REw2muC9V5f1L/w5R5uKGaepA==", "dev": true, "requires": { "loose-envify": "^1.1.0", diff --git a/test/react/16.8/package.json b/test/react/16.8/package.json index 301f12c15..f1cc380f6 100644 --- a/test/react/16.8/package.json +++ b/test/react/16.8/package.json @@ -2,8 +2,8 @@ "private": true, "devDependencies": { "create-react-class": "^15.6.3", - "react": "16.8", - "react-dom": "16.8" + "react": "^16.8.4", + "react-dom": "^16.8.4" }, "dependencies": { "jest-dom": "^3.1.2", From 16a66c4dc790e6ca8052a4e3a36e99d61095b908 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Sun, 17 Mar 2019 23:21:34 -0400 Subject: [PATCH 02/39] Update React peer dependency to 16.8.x --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 62f150ab5..64840c25b 100644 --- a/package.json +++ b/package.json @@ -36,7 +36,7 @@ "coverage": "codecov" }, "peerDependencies": { - "react": "^16.4.0-0", + "react": "^16.8.4", "redux": "^2.0.0 || ^3.0.0 || ^4.0.0-0" }, "dependencies": { From c31b94a9495c25000e00fcb63397aa233236af3a Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Sun, 17 Mar 2019 23:26:29 -0400 Subject: [PATCH 03/39] Initial re-implementation of `connectAdvanced` using hooks Matches changes from v7.0.0-alpha.1 --- src/components/Provider.js | 54 +++--- src/components/connectAdvanced.js | 280 ++++++++++++++++++------------ src/utils/Subscription.js | 98 +++++++++++ 3 files changed, 286 insertions(+), 146 deletions(-) create mode 100644 src/utils/Subscription.js diff --git a/src/components/Provider.js b/src/components/Provider.js index 03c562645..9586b5f73 100644 --- a/src/components/Provider.js +++ b/src/components/Provider.js @@ -1,6 +1,7 @@ import React, { Component } from 'react' import PropTypes from 'prop-types' import { ReactReduxContext } from './Context' +import Subscription from '../utils/Subscription' class Provider extends Component { constructor(props) { @@ -8,56 +9,47 @@ class Provider extends Component { const { store } = props + this.notifySubscribers = this.notifySubscribers.bind(this) + const subscription = new Subscription(store) + subscription.onStateChange = this.notifySubscribers + this.state = { - storeState: store.getState(), - store + store, + subscription } + + this.previousState = store.getState() } componentDidMount() { this._isMounted = true - this.subscribe() + + this.state.subscription.trySubscribe() + + if (this.previousState !== this.props.store.getState()) { + this.state.subscription.notifyNestedSubs() + } } componentWillUnmount() { if (this.unsubscribe) this.unsubscribe() + this.state.subscription.tryUnsubscribe() + this._isMounted = false } componentDidUpdate(prevProps) { if (this.props.store !== prevProps.store) { - if (this.unsubscribe) this.unsubscribe() - - this.subscribe() + this.state.subscription.tryUnsubscribe() + const subscription = new Subscription(this.props.store) + subscription.onStateChange = this.notifySubscribers + this.setState({ store: this.props.store, subscription }) } } - subscribe() { - const { store } = this.props - - this.unsubscribe = store.subscribe(() => { - const newStoreState = store.getState() - - if (!this._isMounted) { - return - } - - this.setState(providerState => { - // If the value is the same, skip the unnecessary state update. - if (providerState.storeState === newStoreState) { - return null - } - - return { storeState: newStoreState } - }) - }) - - // Actions might have been dispatched between render and mount - handle those - const postMountStoreState = store.getState() - if (postMountStoreState !== this.state.storeState) { - this.setState({ storeState: postMountStoreState }) - } + notifySubscribers() { + this.state.subscription.notifyNestedSubs() } render() { diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 69d900bf2..f4e403365 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -1,7 +1,14 @@ import hoistStatics from 'hoist-non-react-statics' import invariant from 'invariant' -import React, { Component, PureComponent } from 'react' +import React, { + useContext, + useMemo, + useEffect, + useRef, + useReducer +} from 'react' import { isValidElementType, isContextConsumer } from 'react-is' +import Subscription from '../utils/Subscription' import { ReactReduxContext } from './Context' @@ -119,145 +126,188 @@ export default function connectAdvanced( const { pure } = connectOptions - let OuterBaseComponent = Component + function createChildSelector(store) { + return selectorFactory(store.dispatch, selectorFactoryOptions) + } + + const usePureOnlyMemo = pure ? useMemo : x => x() - if (pure) { - OuterBaseComponent = PureComponent + function storeStateUpdatesReducer(state, action) { + const [, updateCount = 0] = state + return [action.payload, updateCount + 1] } - function makeDerivedPropsSelector() { - let lastProps - let lastState - let lastDerivedProps - let lastStore - let lastSelectorFactoryOptions - let sourceSelector + function ConnectFunction(props) { + const [propsContext, forwardedRef, wrapperProps] = useMemo(() => { + const { context, forwardedRef, ...wrapperProps } = props + return [context, forwardedRef, wrapperProps] + }, [props]) - return function selectDerivedProps( - state, - props, - store, - selectorFactoryOptions - ) { - if (pure && lastProps === props && lastState === state) { - return lastDerivedProps - } + const ContextToUse = useMemo(() => { + return propsContext && + propsContext.Consumer && + isContextConsumer() + ? propsContext + : Context + }, [propsContext, Context]) - if ( - store !== lastStore || - lastSelectorFactoryOptions !== selectorFactoryOptions - ) { - lastStore = store - lastSelectorFactoryOptions = selectorFactoryOptions - sourceSelector = selectorFactory( - store.dispatch, - selectorFactoryOptions - ) - } + const contextValue = useContext(ContextToUse) - lastProps = props - lastState = state + invariant( + props.store || contextValue, + `Could not find "store" in the context of ` + + `"${displayName}". Either wrap the root component in a , ` + + `or pass a custom React context provider to and the corresponding ` + + `React context consumer to ${displayName} in connect options.` + ) - const nextProps = sourceSelector(state, props) + const store = props.store || contextValue.store - lastDerivedProps = nextProps - return lastDerivedProps - } - } + const childPropsSelector = useMemo(() => { + return createChildSelector(store) + }, [store]) - function makeChildElementSelector() { - let lastChildProps, lastForwardRef, lastChildElement, lastComponent + const [subscription, notifyNestedSubs] = useMemo(() => { + if (!shouldHandleStateChanges) return [] - return function selectChildElement( - WrappedComponent, - childProps, - forwardRef - ) { - if ( - childProps !== lastChildProps || - forwardRef !== lastForwardRef || - lastComponent !== WrappedComponent - ) { - lastChildProps = childProps - lastForwardRef = forwardRef - lastComponent = WrappedComponent - lastChildElement = ( - - ) + // 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] + const subscription = new Subscription(store, contextValue.subscription) + + // `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. + const notifyNestedSubs = subscription.notifyNestedSubs.bind( + subscription + ) + + return [subscription, notifyNestedSubs] + }, [store, contextValue.subscription]) + + const overriddenContextValue = useMemo(() => { + return { + ...contextValue, + subscription } + }, [contextValue, subscription]) - return lastChildElement - } - } + const [[previousStateUpdateResult], dispatch] = useReducer( + storeStateUpdatesReducer, + [] + ) - class Connect extends OuterBaseComponent { - constructor(props) { - super(props) - invariant( - forwardRef ? !props.wrapperProps[storeKey] : !props[storeKey], - 'Passing redux store in props has been removed and does not do anything. ' + - customStoreWarningMessage - ) - this.selectDerivedProps = makeDerivedPropsSelector() - this.selectChildElement = makeChildElementSelector() - this.indirectRenderWrappedComponent = this.indirectRenderWrappedComponent.bind( - this - ) + if (previousStateUpdateResult && previousStateUpdateResult.error) { + throw previousStateUpdateResult.error } - indirectRenderWrappedComponent(value) { - // calling renderWrappedComponent on prototype from indirectRenderWrappedComponent bound to `this` - return this.renderWrappedComponent(value) - } + const lastChildProps = useRef() + const lastWrapperProps = useRef(wrapperProps) + const childPropsFromStoreUpdate = useRef() - renderWrappedComponent(value) { - invariant( - value, - `Could not find "store" in the context of ` + - `"${displayName}". Either wrap the root component in a , ` + - `or pass a custom React context provider to and the corresponding ` + - `React context consumer to ${displayName} in connect options.` - ) - const { storeState, store } = value + const actualChildProps = usePureOnlyMemo(() => { + if (childPropsFromStoreUpdate.current) { + return childPropsFromStoreUpdate.current + } + + // TODO We're reading the store directly in render() here. Bad idea? + return childPropsSelector(store.getState(), wrapperProps) + }, [store, previousStateUpdateResult, wrapperProps]) - let wrapperProps = this.props - let forwardedRef + useEffect(() => { + lastWrapperProps.current = wrapperProps + lastChildProps.current = actualChildProps - if (forwardRef) { - wrapperProps = this.props.wrapperProps - forwardedRef = this.props.forwardedRef + if (childPropsFromStoreUpdate.current) { + childPropsFromStoreUpdate.current = null + notifyNestedSubs() } + }) - let derivedProps = this.selectDerivedProps( - storeState, - wrapperProps, - store, - selectorFactoryOptions - ) + useEffect(() => { + if (!shouldHandleStateChanges) return + + let didUnsubscribe = false + + const checkForUpdates = () => { + if (didUnsubscribe) { + // Don't run stale listeners. + // Redux doesn't guarantee unsubscriptions happen until next dispatch. + return + } + + const latestStoreState = store.getState() + + let newChildProps, error + try { + newChildProps = childPropsSelector( + latestStoreState, + lastWrapperProps.current + ) + } catch (e) { + error = e + } + + if (newChildProps === lastChildProps.current) { + notifyNestedSubs() + } else { + dispatch({ + type: 'STORE_UPDATED', + payload: { + latestStoreState, + error + } + }) + lastChildProps.current = newChildProps + childPropsFromStoreUpdate.current = newChildProps + } + } - return this.selectChildElement( - WrappedComponent, - derivedProps, - forwardedRef - ) - } + // Pull data from the store after first render in case the store has + // changed since we began. + + subscription.onStateChange = checkForUpdates + subscription.trySubscribe() - render() { - const ContextToUse = - this.props.context && - this.props.context.Consumer && - isContextConsumer() - ? this.props.context - : Context - - return ( - - {this.indirectRenderWrappedComponent} - + checkForUpdates() + + const unsubscribeWrapper = () => { + didUnsubscribe = true + subscription.tryUnsubscribe() + } + + return unsubscribeWrapper + }, [store, subscription, childPropsSelector]) + + const renderedChild = useMemo(() => { + const renderedWrappedComponent = ( + ) - } + + if (shouldHandleStateChanges) { + return ( + + {renderedWrappedComponent} + + ) + } + + return renderedWrappedComponent + }, [ + ContextToUse, + WrappedComponent, + actualChildProps, + forwardedRef, + overriddenContextValue + ]) + + return renderedChild } + const Connect = pure ? React.memo(ConnectFunction) : ConnectFunction Connect.WrappedComponent = WrappedComponent Connect.displayName = displayName diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js new file mode 100644 index 000000000..6cee6500a --- /dev/null +++ b/src/utils/Subscription.js @@ -0,0 +1,98 @@ +import { unstable_batchedUpdates } from 'react-dom' + +// encapsulates the subscription logic for connecting a component to the redux store, as +// well as nesting subscriptions of descendant components, so that we can ensure the +// ancestor components re-render before descendants + +const CLEARED = null +const nullListeners = { notify() {} } + +function createListenerCollection() { + // the current/next pattern is copied from redux's createStore code. + // TODO: refactor+expose that code to be reusable here? + let current = [] + let next = [] + + return { + clear() { + next = CLEARED + current = CLEARED + }, + + notify() { + const listeners = (current = next) + unstable_batchedUpdates(() => { + for (let i = 0; i < listeners.length; i++) { + listeners[i]() + } + }) + }, + + get() { + return next + }, + + subscribe(listener) { + let isSubscribed = true + if (next === current) next = current.slice() + next.push(listener) + + return function unsubscribe() { + if (!isSubscribed || current === CLEARED) return + isSubscribed = false + + if (next === current) next = current.slice() + next.splice(next.indexOf(listener), 1) + } + } + } +} + +export default class Subscription { + constructor(store, parentSub) { + this.store = store + this.parentSub = parentSub + this.unsubscribe = null + this.listeners = nullListeners + + this.handleChangeWrapper = this.handleChangeWrapper.bind(this) + } + + addNestedSub(listener) { + this.trySubscribe() + return this.listeners.subscribe(listener) + } + + notifyNestedSubs() { + this.listeners.notify() + } + + handleChangeWrapper() { + if (this.onStateChange) { + this.onStateChange() + } + } + + isSubscribed() { + return Boolean(this.unsubscribe) + } + + trySubscribe() { + if (!this.unsubscribe) { + this.unsubscribe = this.parentSub + ? this.parentSub.addNestedSub(this.handleChangeWrapper) + : this.store.subscribe(this.handleChangeWrapper) + + this.listeners = createListenerCollection() + } + } + + tryUnsubscribe() { + if (this.unsubscribe) { + this.unsubscribe() + this.unsubscribe = null + this.listeners.clear() + this.listeners = nullListeners + } + } +} From 9543efe67c20ad2a0d3072431956e2bc44e9e98f Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Sun, 17 Mar 2019 23:44:12 -0400 Subject: [PATCH 04/39] Update tests to match v7-alpha.1 behavior Added rtl.act() calls around dispatches and other component updates Added clarification on expected mapState calls in some places Disabled some no-longer-relevant tests per implementation Made tests run against React 16.8 by default --- test/components/Provider.spec.js | 99 +++++++-- test/components/connect.spec.js | 256 +++++++++++++++++----- test/components/connectAdvanced.spec.js | 27 ++- test/integration/server-rendering.spec.js | 8 +- test/run-tests.js | 11 +- 5 files changed, 314 insertions(+), 87 deletions(-) diff --git a/test/components/Provider.spec.js b/test/components/Provider.spec.js index ddb02637f..c0cdeac82 100644 --- a/test/components/Provider.spec.js +++ b/test/components/Provider.spec.js @@ -15,6 +15,7 @@ describe('React', () => { afterEach(() => rtl.cleanup()) const createChild = (storeKey = 'store') => { + /* class Child extends Component { render() { return ( @@ -28,6 +29,43 @@ describe('React', () => { ) } } + */ + class Child extends Component { + render() { + //const store = this.context[storeKey]; + return ( + + {({ store }) => { + let text = '' + + if (store) { + text = store.getState().toString() + } + + return ( +
+ {storeKey} - {text} +
+ ) + }} + + ) + + /* + let text = ''; + + if(store) { + text = store.getState().toString() + } + + return ( +
+ {storeKey} - {text} +
+ ) + */ + } + } return Child } @@ -107,24 +145,48 @@ describe('React', () => { const tester = rtl.render() expect(tester.getByTestId('store')).toHaveTextContent('store - 11') - store1.dispatch({ type: 'hi' }) + + /* + rtl.act(() => { + store1.dispatch({ type: 'hi' }) + }) expect(tester.getByTestId('store')).toHaveTextContent('store - 12') +*/ + rtl.act(() => { + externalSetState({ store: store2 }) + }) - externalSetState({ store: store2 }) expect(tester.getByTestId('store')).toHaveTextContent('store - 20') - store1.dispatch({ type: 'hi' }) + rtl.act(() => { + store1.dispatch({ type: 'hi' }) + }) + + expect(tester.getByTestId('store')).toHaveTextContent('store - 20') + rtl.act(() => { + store2.dispatch({ type: 'hi' }) + }) expect(tester.getByTestId('store')).toHaveTextContent('store - 20') - store2.dispatch({ type: 'hi' }) - expect(tester.getByTestId('store')).toHaveTextContent('store - 40') - externalSetState({ store: store3 }) + rtl.act(() => { + externalSetState({ store: store3 }) + }) + + expect(tester.getByTestId('store')).toHaveTextContent('store - 101') + rtl.act(() => { + store1.dispatch({ type: 'hi' }) + }) + expect(tester.getByTestId('store')).toHaveTextContent('store - 101') - store1.dispatch({ type: 'hi' }) + rtl.act(() => { + store2.dispatch({ type: 'hi' }) + }) + expect(tester.getByTestId('store')).toHaveTextContent('store - 101') - store2.dispatch({ type: 'hi' }) + rtl.act(() => { + store3.dispatch({ type: 'hi' }) + }) + expect(tester.getByTestId('store')).toHaveTextContent('store - 101') - store3.dispatch({ type: 'hi' }) - expect(tester.getByTestId('store')).toHaveTextContent('store - 10202') }) it('should handle subscriptions correctly when there is nested Providers', () => { @@ -170,7 +232,10 @@ describe('React', () => { const store = createStore(stringBuilder) - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + let childMapStateInvokes = 0 @connect(state => ({ state })) @@ -211,7 +276,10 @@ describe('React', () => { expect(childMapStateInvokes).toBe(1) // The store state stays consistent when setState calls are batched - store.dispatch({ type: 'APPEND', body: 'c' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'c' }) + }) + expect(childMapStateInvokes).toBe(2) expect(childCalls).toEqual([['a', 'a'], ['ac', 'ac']]) @@ -221,7 +289,10 @@ describe('React', () => { expect(childMapStateInvokes).toBe(3) // Provider uses unstable_batchedUpdates() under the hood - store.dispatch({ type: 'APPEND', body: 'd' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'd' }) + }) + expect(childCalls).toEqual([ ['a', 'a'], ['ac', 'ac'], // then store update is processed @@ -249,7 +320,7 @@ describe('React', () => { expect(spy).not.toHaveBeenCalled() }) - it('should unsubscribe before unmounting', () => { + it.skip('should unsubscribe before unmounting', () => { const store = createStore(createExampleTextReducer()) const subscribe = store.subscribe diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index d6ed4b6a4..1c87539d1 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -159,9 +159,15 @@ describe('React', () => { ) expect(tester.getByTestId('string')).toHaveTextContent('') - store.dispatch({ type: 'APPEND', body: 'a' }) + + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) expect(tester.getByTestId('string')).toHaveTextContent('a') - store.dispatch({ type: 'APPEND', body: 'b' }) + + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'b' }) + }) expect(tester.getByTestId('string')).toHaveTextContent('ab') }) @@ -185,9 +191,15 @@ describe('React', () => { spy.mockRestore() expect(tester.getByTestId('string')).toHaveTextContent('') - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + expect(tester.getByTestId('string')).toHaveTextContent('a') - store.dispatch({ type: 'APPEND', body: 'b' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'b' }) + }) + expect(tester.getByTestId('string')).toHaveTextContent('ab') }) @@ -210,7 +222,10 @@ describe('React', () => { spy.mockRestore() expect(tester.getByTestId('string')).toHaveTextContent('') - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + expect(tester.getByTestId('string')).toHaveTextContent('a') }) @@ -486,12 +501,24 @@ describe('React', () => { const tester = rtl.render() expect(tester.getByTestId('stateThing')).toHaveTextContent('') - merged('a') + rtl.act(() => { + merged('a') + }) + expect(tester.getByTestId('stateThing')).toHaveTextContent('HELLO az') - merged('b') + rtl.act(() => { + merged('b') + }) + expect(tester.getByTestId('stateThing')).toHaveTextContent('HELLO azbz') - externalSetState({ extra: 'Z' }) - merged('c') + rtl.act(() => { + externalSetState({ extra: 'Z' }) + }) + + rtl.act(() => { + merged('c') + }) + expect(tester.getByTestId('stateThing')).toHaveTextContent('HELLO azbzcZ') }) @@ -872,17 +899,25 @@ describe('React', () => { } const div = document.createElement('div') - store.subscribe(() => ReactDOM.unmountComponentAtNode(div)) - ReactDOM.render( - - - , - div - ) + store.subscribe(() => { + ReactDOM.unmountComponentAtNode(div) + }) + + rtl.act(() => { + ReactDOM.render( + + + , + div + ) + }) expect(mapStateToPropsCalls).toBe(1) const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + expect(spy).toHaveBeenCalledTimes(0) expect(mapStateToPropsCalls).toBe(1) spy.mockRestore() @@ -926,7 +961,9 @@ describe('React', () => { ) try { - store.dispatch({ type: 'APPEND', body: 'A' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'A' }) + }) } finally { ReactDOM.unmountComponentAtNode(div) } @@ -972,7 +1009,11 @@ describe('React', () => { App = connect(() => ({}))(App) let A = () =>

A

- A = connect(() => ({ calls: ++mapStateToPropsCalls }))(A) + function mapState(state) { + const calls = ++mapStateToPropsCalls + return { calls, state } + } + A = connect(mapState)(A) const B = () =>

B

@@ -1024,7 +1065,11 @@ describe('React', () => { linkB.click() document.body.removeChild(div) - expect(mapStateToPropsCalls).toBe(2) + // Called 3 times: + // - Initial mount + // - After first link click, stil mounted + // - After second link click, but the queued state update is discarded due to batching as it's unmounted + expect(mapStateToPropsCalls).toBe(3) expect(spy).toHaveBeenCalledTimes(0) spy.mockRestore() }) @@ -1089,11 +1134,20 @@ describe('React', () => { ) expect(spy).toHaveBeenCalledTimes(1) expect(tester.getByTestId('string')).toHaveTextContent('') - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + expect(spy).toHaveBeenCalledTimes(2) - store.dispatch({ type: 'APPEND', body: 'b' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'b' }) + }) + expect(spy).toHaveBeenCalledTimes(3) - store.dispatch({ type: 'APPEND', body: '' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: '' }) + }) + expect(spy).toHaveBeenCalledTimes(3) }) @@ -1142,39 +1196,60 @@ describe('React', () => { expect(tester.getByTestId('string')).toHaveTextContent('') expect(tester.getByTestId('pass')).toHaveTextContent('') - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + expect(spy).toHaveBeenCalledTimes(2) expect(tester.getByTestId('string')).toHaveTextContent('a') expect(tester.getByTestId('pass')).toHaveTextContent('') - tree.setState({ pass: '' }) + rtl.act(() => { + tree.setState({ pass: '' }) + }) + expect(spy).toHaveBeenCalledTimes(2) expect(tester.getByTestId('string')).toHaveTextContent('a') expect(tester.getByTestId('pass')).toHaveTextContent('') - tree.setState({ pass: 'through' }) + rtl.act(() => { + tree.setState({ pass: 'through' }) + }) + expect(spy).toHaveBeenCalledTimes(3) expect(tester.getByTestId('string')).toHaveTextContent('a') expect(tester.getByTestId('pass')).toHaveTextContent('through') - tree.setState({ pass: 'through' }) + rtl.act(() => { + tree.setState({ pass: 'through' }) + }) + expect(spy).toHaveBeenCalledTimes(3) expect(tester.getByTestId('string')).toHaveTextContent('a') expect(tester.getByTestId('pass')).toHaveTextContent('through') const obj = { prop: 'val' } - tree.setState({ pass: obj }) + rtl.act(() => { + tree.setState({ pass: obj }) + }) + expect(spy).toHaveBeenCalledTimes(4) expect(tester.getByTestId('string')).toHaveTextContent('a') expect(tester.getByTestId('pass')).toHaveTextContent('{"prop":"val"}') - tree.setState({ pass: obj }) + rtl.act(() => { + tree.setState({ pass: obj }) + }) + expect(spy).toHaveBeenCalledTimes(4) expect(tester.getByTestId('string')).toHaveTextContent('a') expect(tester.getByTestId('pass')).toHaveTextContent('{"prop":"val"}') const obj2 = Object.assign({}, obj, { val: 'otherval' }) - tree.setState({ pass: obj2 }) + rtl.act(() => { + tree.setState({ pass: obj2 }) + }) + expect(spy).toHaveBeenCalledTimes(5) expect(tester.getByTestId('string')).toHaveTextContent('a') expect(tester.getByTestId('pass')).toHaveTextContent( @@ -1182,7 +1257,10 @@ describe('React', () => { ) obj2.val = 'mutation' - tree.setState({ pass: obj2 }) + rtl.act(() => { + tree.setState({ pass: obj2 }) + }) + expect(spy).toHaveBeenCalledTimes(5) expect(tester.getByTestId('string')).toHaveTextContent('a') expect(tester.getByTestId('pass')).toHaveTextContent( @@ -1418,7 +1496,9 @@ describe('React', () => { imitateHotReloading(ParentBefore, ParentAfter, container) - store.dispatch({ type: ACTION_TYPE }) + rtl.act(() => { + store.dispatch({ type: ACTION_TYPE }) + }) expect(tester.getByTestId('actions')).toHaveTextContent('1') }) @@ -1796,9 +1876,11 @@ describe('React', () => { const mapStateSpy = jest.fn() const mapDispatchSpy = jest.fn().mockReturnValue({}) + const impureRenderSpy = jest.fn() class ImpureComponent extends Component { render() { + impureRenderSpy() return } } @@ -1836,23 +1918,37 @@ describe('React', () => { ) - expect(mapStateSpy).toHaveBeenCalledTimes(1) - expect(mapDispatchSpy).toHaveBeenCalledTimes(1) + // 1) Initial render + // 2) Post-mount check + // 3) After "wasted" re-render + expect(mapStateSpy).toHaveBeenCalledTimes(2) + expect(mapDispatchSpy).toHaveBeenCalledTimes(2) + + // 1) Initial render + // 2) Triggered by post-mount check with impure results + expect(impureRenderSpy).toHaveBeenCalledTimes(2) expect(tester.getByTestId('statefulValue')).toHaveTextContent('foo') // Impure update storeGetter.storeKey = 'bar' externalSetState({ storeGetter }) - expect(mapStateSpy).toHaveBeenCalledTimes(2) - expect(mapDispatchSpy).toHaveBeenCalledTimes(2) + // 4) After the the impure update + expect(mapStateSpy).toHaveBeenCalledTimes(3) + expect(mapDispatchSpy).toHaveBeenCalledTimes(3) + + // 3) Triggered by impure update + expect(impureRenderSpy).toHaveBeenCalledTimes(3) expect(tester.getByTestId('statefulValue')).toHaveTextContent('bar') }) it('should pass state consistently to mapState', () => { const store = createStore(stringBuilder) - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + let childMapStateInvokes = 0 @connect(state => ({ state })) @@ -1894,8 +1990,7 @@ describe('React', () => { expect(childMapStateInvokes).toBe(1) expect(childCalls).toEqual([['a', 'a']]) - // The store state stays consistent when setState calls are batched - ReactDOM.unstable_batchedUpdates(() => { + rtl.act(() => { store.dispatch({ type: 'APPEND', body: 'c' }) }) expect(childMapStateInvokes).toBe(2) @@ -1906,7 +2001,10 @@ describe('React', () => { rtl.fireEvent.click(button) expect(childMapStateInvokes).toBe(3) - store.dispatch({ type: 'APPEND', body: 'd' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'd' }) + }) + expect(childMapStateInvokes).toBe(4) expect(childCalls).toEqual([ ['a', 'a'], @@ -1941,7 +2039,9 @@ describe('React', () => { expect(renderCalls).toBe(1) expect(mapStateCalls).toBe(1) - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) // After store a change mapState has been called expect(mapStateCalls).toBe(2) @@ -1974,15 +2074,24 @@ describe('React', () => { expect(renderCalls).toBe(1) expect(mapStateCalls).toBe(1) - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + expect(mapStateCalls).toBe(2) expect(renderCalls).toBe(1) - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + expect(mapStateCalls).toBe(3) expect(renderCalls).toBe(1) - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + expect(mapStateCalls).toBe(4) expect(renderCalls).toBe(2) }) @@ -2060,7 +2169,10 @@ describe('React', () => { ) - store.dispatch({ type: 'test' }) + rtl.act(() => { + store.dispatch({ type: 'test' }) + }) + expect(updatedCount).toBe(0) expect(memoizedReturnCount).toBe(2) }) @@ -2095,7 +2207,10 @@ describe('React', () => { ) - store.dispatch({ type: 'test' }) + rtl.act(() => { + store.dispatch({ type: 'test' }) + }) + expect(initialOwnProps).toBe(undefined) expect(initialState).not.toBe(undefined) expect(secondaryOwnProps).not.toBe(undefined) @@ -2161,7 +2276,10 @@ describe('React', () => { ) - store.dispatch({ type: 'test' }) + rtl.act(() => { + store.dispatch({ type: 'test' }) + }) + expect(updatedCount).toBe(0) expect(memoizedReturnCount).toBe(2) }) @@ -2192,7 +2310,9 @@ describe('React', () => { expect(renderCalls).toBe(1) expect(mapStateCalls).toBe(1) - store.dispatch({ type: 'APPEND', body: 'a' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) expect(mapStateCalls).toBe(2) expect(renderCalls).toBe(1) @@ -2258,9 +2378,13 @@ describe('React', () => { } } - @connect(state => ({ - profile: state.data.profile - })) + function mapState(state) { + return { + profile: state.data.profile + } + } + + @connect(mapState) class Child extends React.Component { render() { return null @@ -2268,7 +2392,10 @@ describe('React', () => { } const store = createStore(reducer) - store.dispatch({ type: 'fetch' }) + rtl.act(() => { + store.dispatch({ type: 'fetch' }) + }) + const div = document.createElement('div') ReactDOM.render( @@ -2320,7 +2447,10 @@ describe('React', () => { ) const rendersBeforeStateChange = renderCount - store.dispatch({ type: 'ACTION' }) + rtl.act(() => { + store.dispatch({ type: 'ACTION' }) + }) + expect(renderCount).toBe(rendersBeforeStateChange + 1) }) @@ -2430,7 +2560,10 @@ describe('React', () => { ) expect(mapStateToProps).toHaveBeenCalledTimes(1) - store.dispatch({ type: 'INC' }) + rtl.act(() => { + store.dispatch({ type: 'INC' }) + }) + expect(mapStateToProps).toHaveBeenCalledTimes(2) }) @@ -2468,7 +2601,9 @@ describe('React', () => { ) - store.dispatch({ type: 'INC' }) + rtl.act(() => { + store.dispatch({ type: 'INC' }) + }) }) it('should subscribe properly when a new store is provided via props', () => { @@ -2537,12 +2672,17 @@ describe('React', () => { expect(mapStateToPropsC).toHaveBeenCalledTimes(1) expect(mapStateToPropsD).toHaveBeenCalledTimes(1) - store1.dispatch({ type: 'INC' }) + rtl.act(() => { + store1.dispatch({ type: 'INC' }) + }) + expect(mapStateToPropsB).toHaveBeenCalledTimes(1) expect(mapStateToPropsC).toHaveBeenCalledTimes(1) expect(mapStateToPropsD).toHaveBeenCalledTimes(2) - store2.dispatch({ type: 'INC' }) + rtl.act(() => { + store2.dispatch({ type: 'INC' }) + }) expect(mapStateToPropsB).toHaveBeenCalledTimes(2) expect(mapStateToPropsC).toHaveBeenCalledTimes(2) expect(mapStateToPropsD).toHaveBeenCalledTimes(2) @@ -2608,7 +2748,7 @@ describe('React', () => { }).toThrow(/storeKey has been removed/) }) - it('should error on custom store', () => { + it.skip('should error on custom store', () => { function Comp() { return
hi
} diff --git a/test/components/connectAdvanced.spec.js b/test/components/connectAdvanced.spec.js index 6a47c8579..fcce46b55 100644 --- a/test/components/connectAdvanced.spec.js +++ b/test/components/connectAdvanced.spec.js @@ -6,7 +6,7 @@ import 'jest-dom/extend-expect' describe('React', () => { describe('connectAdvanced', () => { - it('should map state and render once on mount', () => { + it('should map state and render on mount', () => { const initialState = { foo: 'bar' } @@ -36,7 +36,10 @@ describe('React', () => { expect(tester.getByTestId('foo')).toHaveTextContent('bar') - expect(mapCount).toEqual(1) + // Implementation detail: + // 1) Initial render + // 2) Post-mount subscription and update check + expect(mapCount).toEqual(2) expect(renderCount).toEqual(1) }) @@ -67,10 +70,12 @@ describe('React', () => { ) - store.dispatch({ type: 'NEW_REFERENCE' }) + rtl.act(() => { + store.dispatch({ type: 'NEW_REFERENCE' }) + }) // Should have mapped the state on mount and on the dispatch - expect(mapCount).toEqual(2) + expect(mapCount).toEqual(3) // Should have rendered on mount and after the dispatch bacause the map // state returned new reference @@ -113,8 +118,11 @@ describe('React', () => { expect(tester.getByTestId('foo')).toHaveTextContent('bar') - // The state should have been mapped twice: on mount and on the dispatch - expect(mapCount).toEqual(2) + // The state should have been mapped 3 times: + // 1) Initial render + // 2) Post-mount update check + // 3) Dispatch + expect(mapCount).toEqual(3) // But the render should have been called only on mount since the map state // did not return a new reference @@ -172,8 +180,11 @@ describe('React', () => { outerComponent.setFoo('BAR') - // map on mount and on prop change - expect(mapCount).toEqual(2) + // The state should have been mapped 3 times: + // 1) Initial render + // 2) Post-mount update check + // 3) Prop change + expect(mapCount).toEqual(3) // render only on mount but skip on prop change because no new // reference was returned diff --git a/test/integration/server-rendering.spec.js b/test/integration/server-rendering.spec.js index 6bd703678..b660a51d9 100644 --- a/test/integration/server-rendering.spec.js +++ b/test/integration/server-rendering.spec.js @@ -47,13 +47,19 @@ describe('React', () => { expect(store.getState().greeting).toContain('Hi') }) - it('should render children with original state even if actions are dispatched in ancestor', () => { + it.skip('should render children with original state even if actions are dispatched in ancestor', () => { /* Dispatching during construct, render or willMount is almost always a bug with SSR (or otherwise) This behaviour is undocumented and is likely to change between implementations, this test only verifies current behaviour + + Note: this test passes in v6, because we use context to propagate the store state, and the entire + tree will see the same state during the render pass. + + In all other versions, including v7, the store state may change as actions are dispatched + during lifecycle methods, and components will see that new state immediately as they read it. */ const store = createStore(greetingReducer) diff --git a/test/run-tests.js b/test/run-tests.js index 9830503b5..f91da18a9 100644 --- a/test/run-tests.js +++ b/test/run-tests.js @@ -1,7 +1,7 @@ const npmRun = require('npm-run') const fs = require('fs') const path = require('path') -const LATEST_VERSION = '16.6' +const LATEST_VERSION = '16.8' const version = process.env.REACT || LATEST_VERSION let jestConfig = { @@ -33,9 +33,8 @@ const configFilePath = path.join(__dirname, 'jest-config.json') fs.writeFileSync(configFilePath, JSON.stringify(jestConfig)) -const commandLine = `jest -c "${configFilePath}" ${process.argv.slice(2).join(' ')}` +const commandLine = `jest -c "${configFilePath}" ${process.argv + .slice(2) + .join(' ')}` -npmRun.execSync( - commandLine, - { stdio: 'inherit' } -) +npmRun.execSync(commandLine, { stdio: 'inherit' }) From 1f2afe9ffbabab8aaaf06230e2701425ae0fa1e5 Mon Sep 17 00:00:00 2001 From: Rodrigo Saboya Date: Tue, 26 Feb 2019 00:51:49 -0300 Subject: [PATCH 05/39] adding a react hooks test that fails with v7 alpha --- test/components/hooks.spec.js | 107 ++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 test/components/hooks.spec.js diff --git a/test/components/hooks.spec.js b/test/components/hooks.spec.js new file mode 100644 index 000000000..d949bf1f0 --- /dev/null +++ b/test/components/hooks.spec.js @@ -0,0 +1,107 @@ +/*eslint-disable react/prop-types*/ + +import React from 'react' +import { createStore } from 'redux' +import { Provider as ProviderMock, connect } from '../../src/index.js' +import * as rtl from 'react-testing-library' +import 'jest-dom/extend-expect' + +describe('React', () => { + describe('connect', () => { + afterEach(() => rtl.cleanup()) + + it('should render on useEffect hook state update', () => { + const store = createStore((state, action) => { + let newState = + state !== undefined + ? state + : { + byId: {}, + list: [] + } + switch (action.type) { + case 'FOO': + newState = { + ...newState, + list: [1], + byId: { 1: 'foo' } + } + break + } + return newState + }) + + const mapStateSpy1 = jest.fn() + const renderSpy1 = jest.fn() + + const component1Decorator = connect(state => { + mapStateSpy1() + + return { + list: state.list + } + }) + + const component1 = props => { + renderSpy1() + + return + } + + const Component1 = component1Decorator(component1) + + const renderSpy2 = jest.fn() + + const Component2 = props => { + const [state, setState] = React.useState({ list: props.list }) + + React.useEffect(() => { + setState({ list: props.list }) + }, [props.list]) + + renderSpy2() + + return + } + + const mapStateSpy3 = jest.fn() + const renderSpy3 = jest.fn() + + const component3Decorator = connect((state, ownProps) => { + mapStateSpy3() + + return { + mappedProp: ownProps.list.map(id => state.byId[id]) + } + }) + + const component3 = () => { + renderSpy3() + + return
Hello
+ } + + const Component3 = component3Decorator(component3) + + rtl.render( + + + + ) + + expect(mapStateSpy1).toHaveBeenCalledTimes(1) + expect(renderSpy1).toHaveBeenCalledTimes(1) + expect(renderSpy2).toHaveBeenCalledTimes(2) + expect(mapStateSpy3).toHaveBeenCalledTimes(1) + expect(renderSpy3).toHaveBeenCalledTimes(1) + + store.dispatch({ type: 'FOO' }) + + expect(mapStateSpy1).toHaveBeenCalledTimes(2) + expect(renderSpy1).toHaveBeenCalledTimes(2) + expect(renderSpy2).toHaveBeenCalledTimes(3) + expect(mapStateSpy3).toHaveBeenCalledTimes(2) + expect(renderSpy3).toHaveBeenCalledTimes(2) + }) + }) +}) From 2f4372ecf4219d575709aeeb46d2656e6ae6a527 Mon Sep 17 00:00:00 2001 From: Rodrigo Saboya Date: Tue, 26 Feb 2019 01:19:45 -0300 Subject: [PATCH 06/39] wrapping store.dispatch with rlt.act, fixes component renders --- test/components/hooks.spec.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/components/hooks.spec.js b/test/components/hooks.spec.js index d949bf1f0..18e521f9e 100644 --- a/test/components/hooks.spec.js +++ b/test/components/hooks.spec.js @@ -95,13 +95,15 @@ describe('React', () => { expect(mapStateSpy3).toHaveBeenCalledTimes(1) expect(renderSpy3).toHaveBeenCalledTimes(1) - store.dispatch({ type: 'FOO' }) + rtl.act(() => { + store.dispatch({ type: 'FOO' }) + }) expect(mapStateSpy1).toHaveBeenCalledTimes(2) expect(renderSpy1).toHaveBeenCalledTimes(2) - expect(renderSpy2).toHaveBeenCalledTimes(3) - expect(mapStateSpy3).toHaveBeenCalledTimes(2) - expect(renderSpy3).toHaveBeenCalledTimes(2) + expect(renderSpy2).toHaveBeenCalledTimes(4) + expect(mapStateSpy3).toHaveBeenCalledTimes(3) + expect(renderSpy3).toHaveBeenCalledTimes(3) }) }) }) From d210de1fffd389a53d8bb64d1b6156a9d1a6c246 Mon Sep 17 00:00:00 2001 From: Rodrigo Saboya Date: Tue, 26 Feb 2019 12:07:23 -0300 Subject: [PATCH 07/39] reducing hooks test to 2 components --- test/components/hooks.spec.js | 46 ++++++++++++++--------------------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/test/components/hooks.spec.js b/test/components/hooks.spec.js index 18e521f9e..342a928af 100644 --- a/test/components/hooks.spec.js +++ b/test/components/hooks.spec.js @@ -42,33 +42,25 @@ describe('React', () => { } }) - const component1 = props => { - renderSpy1() - - return - } - - const Component1 = component1Decorator(component1) - - const renderSpy2 = jest.fn() - - const Component2 = props => { + const component2 = props => { const [state, setState] = React.useState({ list: props.list }) React.useEffect(() => { - setState({ list: props.list }) + setState(prevState => ({ ...prevState, list: props.list })) }, [props.list]) - renderSpy2() + renderSpy1() - return + return } - const mapStateSpy3 = jest.fn() - const renderSpy3 = jest.fn() + const Component1 = component1Decorator(component2) + + const mapStateSpy2 = jest.fn() + const renderSpy2 = jest.fn() - const component3Decorator = connect((state, ownProps) => { - mapStateSpy3() + const component2Decorator = connect((state, ownProps) => { + mapStateSpy2() return { mappedProp: ownProps.list.map(id => state.byId[id]) @@ -76,12 +68,12 @@ describe('React', () => { }) const component3 = () => { - renderSpy3() + renderSpy2() return
Hello
} - const Component3 = component3Decorator(component3) + const Component2 = component2Decorator(component3) rtl.render( @@ -90,20 +82,18 @@ describe('React', () => { ) expect(mapStateSpy1).toHaveBeenCalledTimes(1) - expect(renderSpy1).toHaveBeenCalledTimes(1) - expect(renderSpy2).toHaveBeenCalledTimes(2) - expect(mapStateSpy3).toHaveBeenCalledTimes(1) - expect(renderSpy3).toHaveBeenCalledTimes(1) + expect(renderSpy1).toHaveBeenCalledTimes(2) + expect(mapStateSpy2).toHaveBeenCalledTimes(1) + expect(renderSpy2).toHaveBeenCalledTimes(1) rtl.act(() => { store.dispatch({ type: 'FOO' }) }) expect(mapStateSpy1).toHaveBeenCalledTimes(2) - expect(renderSpy1).toHaveBeenCalledTimes(2) - expect(renderSpy2).toHaveBeenCalledTimes(4) - expect(mapStateSpy3).toHaveBeenCalledTimes(3) - expect(renderSpy3).toHaveBeenCalledTimes(3) + expect(renderSpy1).toHaveBeenCalledTimes(4) + expect(mapStateSpy2).toHaveBeenCalledTimes(3) + expect(renderSpy2).toHaveBeenCalledTimes(3) }) }) }) From 30178b7d4a753bad9a8207b007c77ce4b8572b17 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Sat, 2 Mar 2019 15:44:47 -0500 Subject: [PATCH 08/39] Fix case where wrapper props changed before store update render --- src/components/connectAdvanced.js | 5 ++++- test/components/hooks.spec.js | 33 ++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index f4e403365..7c20b9916 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -209,7 +209,10 @@ export default function connectAdvanced( const childPropsFromStoreUpdate = useRef() const actualChildProps = usePureOnlyMemo(() => { - if (childPropsFromStoreUpdate.current) { + if ( + childPropsFromStoreUpdate.current && + wrapperProps === lastWrapperProps.current + ) { return childPropsFromStoreUpdate.current } diff --git a/test/components/hooks.spec.js b/test/components/hooks.spec.js index 342a928af..3da3521ce 100644 --- a/test/components/hooks.spec.js +++ b/test/components/hooks.spec.js @@ -34,6 +34,8 @@ describe('React', () => { const mapStateSpy1 = jest.fn() const renderSpy1 = jest.fn() + let component1StateList + const component1Decorator = connect(state => { mapStateSpy1() @@ -42,9 +44,11 @@ describe('React', () => { } }) - const component2 = props => { + const component1 = props => { const [state, setState] = React.useState({ list: props.list }) + component1StateList = state.list + React.useEffect(() => { setState(prevState => ({ ...prevState, list: props.list })) }, [props.list]) @@ -54,7 +58,7 @@ describe('React', () => { return } - const Component1 = component1Decorator(component2) + const Component1 = component1Decorator(component1) const mapStateSpy2 = jest.fn() const renderSpy2 = jest.fn() @@ -67,13 +71,15 @@ describe('React', () => { } }) - const component3 = () => { + const component2 = props => { renderSpy2() + expect(props.list).toBe(component1StateList) + return
Hello
} - const Component2 = component2Decorator(component3) + const Component2 = component2Decorator(component2) rtl.render( @@ -81,19 +87,36 @@ describe('React', () => { ) + // 1. Initial render expect(mapStateSpy1).toHaveBeenCalledTimes(1) + + // 1.Initial render + // 2. C1 useEffect expect(renderSpy1).toHaveBeenCalledTimes(2) + + // 1. Initial render expect(mapStateSpy2).toHaveBeenCalledTimes(1) + + // 1. Initial render expect(renderSpy2).toHaveBeenCalledTimes(1) rtl.act(() => { store.dispatch({ type: 'FOO' }) }) + // 2. Store dispatch expect(mapStateSpy1).toHaveBeenCalledTimes(2) + + // 3. Store dispatch + // 4. C1 useEffect expect(renderSpy1).toHaveBeenCalledTimes(4) + + // 2. Connect(C2) subscriber + // 3. Ignored prev child props in re-render and re-runs mapState expect(mapStateSpy2).toHaveBeenCalledTimes(3) - expect(renderSpy2).toHaveBeenCalledTimes(3) + + // 2. Batched update from nested subscriber / C1 re-render + expect(renderSpy2).toHaveBeenCalledTimes(2) }) }) }) From 8b72631ec3c4c901d462d0dc1948c37bb7e240cf Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Sun, 17 Mar 2019 23:52:10 -0400 Subject: [PATCH 09/39] Mark ReactDOM as global in UMD builds Matches state as of v7.0.0-alpha.2 --- rollup.config.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/rollup.config.js b/rollup.config.js index 0d246bb52..28976bdc7 100644 --- a/rollup.config.js +++ b/rollup.config.js @@ -9,13 +9,14 @@ const env = process.env.NODE_ENV const config = { input: 'src/index.js', - external: Object.keys(pkg.peerDependencies || {}), + external: Object.keys(pkg.peerDependencies || {}).concat('react-dom'), output: { format: 'umd', name: 'ReactRedux', globals: { react: 'React', - redux: 'Redux' + redux: 'Redux', + 'react-dom': 'ReactDOM' } }, plugins: [ @@ -32,7 +33,8 @@ const config = { 'node_modules/react-is/index.js': [ 'isValidElementType', 'isContextConsumer' - ] + ], + 'node_modules/react-dom/index.js': ['unstable_batchedUpdates'] } }) ] From af0c3a7228307120674b882dcfb27c720b7cb480 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Tue, 26 Feb 2019 23:32:28 +0100 Subject: [PATCH 10/39] Fix perf problems with out-of-bounds array access Matches state as of v7.0.0-alpha.3 --- src/components/connectAdvanced.js | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 7c20b9916..dcd5cc98e 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -12,6 +12,10 @@ import Subscription from '../utils/Subscription' import { ReactReduxContext } from './Context' +const EMPTY_ARRAY = [] + +const NO_SUBSCRIPTION_ARRAY = [null, null] + const stringifyComponent = Comp => { try { return JSON.stringify(Comp) @@ -20,6 +24,13 @@ const stringifyComponent = Comp => { } } +function storeStateUpdatesReducer(state, action) { + const [, updateCount] = state + return [action.payload, updateCount + 1] +} + +const initStateUpdates = () => [null, 0] + export default function connectAdvanced( /* selectorFactory is a func that is responsible for returning the selector function used to @@ -132,11 +143,6 @@ export default function connectAdvanced( const usePureOnlyMemo = pure ? useMemo : x => x() - function storeStateUpdatesReducer(state, action) { - const [, updateCount = 0] = state - return [action.payload, updateCount + 1] - } - function ConnectFunction(props) { const [propsContext, forwardedRef, wrapperProps] = useMemo(() => { const { context, forwardedRef, ...wrapperProps } = props @@ -168,7 +174,7 @@ export default function connectAdvanced( }, [store]) const [subscription, notifyNestedSubs] = useMemo(() => { - if (!shouldHandleStateChanges) return [] + if (!shouldHandleStateChanges) return NO_SUBSCRIPTION_ARRAY // 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. @@ -197,7 +203,8 @@ export default function connectAdvanced( const [[previousStateUpdateResult], dispatch] = useReducer( storeStateUpdatesReducer, - [] + EMPTY_ARRAY, + initStateUpdates ) if (previousStateUpdateResult && previousStateUpdateResult.error) { @@ -311,6 +318,7 @@ export default function connectAdvanced( } const Connect = pure ? React.memo(ConnectFunction) : ConnectFunction + Connect.WrappedComponent = WrappedComponent Connect.displayName = displayName From 266ca974182dddc9ba684f1cb0be51aa0a32fd12 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Tue, 12 Mar 2019 22:18:57 -0400 Subject: [PATCH 11/39] Add modules to handle importing batchedUpdates --- src/utils/batch.js | 12 ++++++++++++ src/utils/reactBatchedUpdates.js | 2 ++ src/utils/reactBatchedUpdates.native.js | 4 ++++ 3 files changed, 18 insertions(+) create mode 100644 src/utils/batch.js create mode 100644 src/utils/reactBatchedUpdates.js create mode 100644 src/utils/reactBatchedUpdates.native.js diff --git a/src/utils/batch.js b/src/utils/batch.js new file mode 100644 index 000000000..d8a55dd91 --- /dev/null +++ b/src/utils/batch.js @@ -0,0 +1,12 @@ +// Default to a dummy "batch" implementation that just runs the callback +function defaultNoopBatch(callback) { + callback() +} + +let batch = defaultNoopBatch + +// Allow injecting another batching function later +export const setBatch = newBatch => (batch = newBatch) + +// Supply a getter just to skip dealing with ESM bindings +export const getBatch = () => batch diff --git a/src/utils/reactBatchedUpdates.js b/src/utils/reactBatchedUpdates.js new file mode 100644 index 000000000..2a66a4428 --- /dev/null +++ b/src/utils/reactBatchedUpdates.js @@ -0,0 +1,2 @@ +/* eslint-disable import/no-unresolved */ +export { unstable_batchedUpdates } from 'react-dom' diff --git a/src/utils/reactBatchedUpdates.native.js b/src/utils/reactBatchedUpdates.native.js new file mode 100644 index 000000000..c249de91d --- /dev/null +++ b/src/utils/reactBatchedUpdates.native.js @@ -0,0 +1,4 @@ +/* eslint-disable import/no-unresolved */ +import { unstable_batchedUpdates } from 'react-native' + +export { unstable_batchedUpdates } From 0887834c51293a9b0b9deaf526b06bc900ad8285 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Tue, 12 Mar 2019 22:19:33 -0400 Subject: [PATCH 12/39] Use appropriate batched update API for subscriptions --- src/utils/Subscription.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index 6cee6500a..4c73b072d 100644 --- a/src/utils/Subscription.js +++ b/src/utils/Subscription.js @@ -1,4 +1,4 @@ -import { unstable_batchedUpdates } from 'react-dom' +import { getBatch } from './batch' // encapsulates the subscription logic for connecting a component to the redux store, as // well as nesting subscriptions of descendant components, so that we can ensure the @@ -7,7 +7,7 @@ import { unstable_batchedUpdates } from 'react-dom' const CLEARED = null const nullListeners = { notify() {} } -function createListenerCollection() { +function createListenerCollection(batch) { // the current/next pattern is copied from redux's createStore code. // TODO: refactor+expose that code to be reusable here? let current = [] @@ -21,7 +21,7 @@ function createListenerCollection() { notify() { const listeners = (current = next) - unstable_batchedUpdates(() => { + batch(() => { for (let i = 0; i < listeners.length; i++) { listeners[i]() } @@ -83,7 +83,7 @@ export default class Subscription { ? this.parentSub.addNestedSub(this.handleChangeWrapper) : this.store.subscribe(this.handleChangeWrapper) - this.listeners = createListenerCollection() + this.listeners = createListenerCollection(getBatch()) } } From 593592e6264da112944ee7c4f437e33686010033 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Tue, 12 Mar 2019 22:19:57 -0400 Subject: [PATCH 13/39] Inject unstable_batchedUpdates in default entry point --- src/index.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/index.js b/src/index.js index 22b1bd9e5..8d6460fde 100644 --- a/src/index.js +++ b/src/index.js @@ -3,4 +3,9 @@ import connectAdvanced from './components/connectAdvanced' import { ReactReduxContext } from './components/Context' import connect from './connect/connect' -export { Provider, connectAdvanced, ReactReduxContext, connect } +import { setBatch } from './utils/batch' +import { unstable_batchedUpdates as batch } from './utils/reactBatchedUpdates' + +setBatch(batch) + +export { Provider, connectAdvanced, ReactReduxContext, connect, batch } From 4f0012c69027bdb6ad95edec8af3c93c5e37a78c Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Tue, 12 Mar 2019 22:20:10 -0400 Subject: [PATCH 14/39] Provide an alternate entry point for alternate renderers Matches state as of v7.0.0-alpha.4 --- src/alternate-renderers.js | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 src/alternate-renderers.js diff --git a/src/alternate-renderers.js b/src/alternate-renderers.js new file mode 100644 index 000000000..adc443004 --- /dev/null +++ b/src/alternate-renderers.js @@ -0,0 +1,11 @@ +import Provider from './components/Provider' +import connectAdvanced from './components/connectAdvanced' +import { ReactReduxContext } from './components/Context' +import connect from './connect/connect' + +import { getBatch } from './utils/batch' + +// For other renderers besides ReactDOM and React Native, use the default noop batch function +const batch = getBatch() + +export { Provider, connectAdvanced, ReactReduxContext, connect, batch } From 688a2706576792ac5bb224c08aab8f354481bc44 Mon Sep 17 00:00:00 2001 From: Miguel Oller Date: Sun, 17 Mar 2019 00:08:44 -0400 Subject: [PATCH 15/39] Remove batch arg from createListenerCollection (#1205) This prevents a bug with Terser (webpack's default minifier) where the returned batch function isn't defined due to function inlining. Matches state as of v7.0.0-alpha.5 --- src/utils/Subscription.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index 4c73b072d..e03f4838a 100644 --- a/src/utils/Subscription.js +++ b/src/utils/Subscription.js @@ -7,7 +7,8 @@ import { getBatch } from './batch' const CLEARED = null const nullListeners = { notify() {} } -function createListenerCollection(batch) { +function createListenerCollection() { + const batch = getBatch() // the current/next pattern is copied from redux's createStore code. // TODO: refactor+expose that code to be reusable here? let current = [] @@ -83,7 +84,7 @@ export default class Subscription { ? this.parentSub.addNestedSub(this.handleChangeWrapper) : this.store.subscribe(this.handleChangeWrapper) - this.listeners = createListenerCollection(getBatch()) + this.listeners = createListenerCollection() } } From 8755d5a71716ffaa917ca53bf720a5d6b0f86566 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 18 Mar 2019 20:22:46 -0400 Subject: [PATCH 16/39] Remove older React versions from Travis --- .travis.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index a7c96bbfe..84a82d892 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,9 +3,6 @@ node_js: - "10" env: matrix: - - REACT=16.4 - - REACT=16.5 - - REACT=16.6 - REACT=16.8 sudo: false script: From 01ce9bd0f70de59ed330d7eb5c44bc16514eba4b Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 18 Mar 2019 21:28:53 -0400 Subject: [PATCH 17/39] Add comments to connectAdvanced. Many much comments! --- src/components/connectAdvanced.js | 74 ++++++++++++++++++++++++++----- 1 file changed, 63 insertions(+), 11 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index dcd5cc98e..da33b4c7c 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -12,8 +12,8 @@ import Subscription from '../utils/Subscription' import { ReactReduxContext } from './Context' +// Define some constant arrays just to avoid re-creating these const EMPTY_ARRAY = [] - const NO_SUBSCRIPTION_ARRAY = [null, null] const stringifyComponent = Comp => { @@ -85,7 +85,7 @@ export default function connectAdvanced( ) { invariant( renderCountProp === undefined, - `renderCountProp is removed. render counting is built into the latest React dev tools profiling extension` + `renderCountProp is removed. render counting is built into the latest React Dev Tools profiling extension` ) invariant( @@ -141,15 +141,23 @@ export default function connectAdvanced( return selectorFactory(store.dispatch, selectorFactoryOptions) } - const usePureOnlyMemo = pure ? useMemo : x => x() + // If we aren't running in "pure" mode, we don't want to memoize values. + // To avoid conditionally calling hooks, we fall back to a tiny wrapper + // that just executes the given callback immediately. + const usePureOnlyMemo = pure ? useMemo : callback => callback() function ConnectFunction(props) { const [propsContext, forwardedRef, wrapperProps] = useMemo(() => { + // Distinguish between actual "data" props that were passed to the wrapper component, + // and values needed to control behavior (forwarded refs, alternate context instances). + // To maintain the wrapperProps object reference, memoize this destructuring. const { context, forwardedRef, ...wrapperProps } = props return [context, forwardedRef, wrapperProps] }, [props]) const ContextToUse = useMemo(() => { + // Users may optionally pass in a custom context instance to use instead of our ReactReduxContext. + // Memoize the check that determines which context instance we should use. return propsContext && propsContext.Consumer && isContextConsumer() @@ -157,8 +165,11 @@ export default function connectAdvanced( : Context }, [propsContext, Context]) + // Retrieve the store and ancestor subscription via context, if available const contextValue = useContext(ContextToUse) + + // The store _must_ exist as either a prop or in context invariant( props.store || contextValue, `Could not find "store" in the context of ` + @@ -168,8 +179,11 @@ export default function connectAdvanced( ) const store = props.store || contextValue.store + const propsMode = Boolean(props.store) const childPropsSelector = useMemo(() => { + // The child props selector needs the store reference as an input. + // Re-create this selector whenever the store changes. return createChildSelector(store) }, [store]) @@ -178,15 +192,12 @@ 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] const subscription = new Subscription(store, contextValue.subscription) // `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. + // the middle of the notification loop, where `subscription` will then be null. 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. const notifyNestedSubs = subscription.notifyNestedSubs.bind( subscription ) @@ -194,28 +205,42 @@ export default function connectAdvanced( return [subscription, notifyNestedSubs] }, [store, contextValue.subscription]) + // Determine what {store, subscription} value should be put into nested context, if necessary const overriddenContextValue = useMemo(() => { + + // Otherwise, put this component's subscription instance into context, so that + // connected descendants won't update until after this component is done return { ...contextValue, subscription } }, [contextValue, subscription]) - const [[previousStateUpdateResult], dispatch] = useReducer( + // We need to force this wrapper component to re-render whenever a Redux store update + // causes a change to the calculated child component props (or we caught an error in mapState) + const [[previousStateUpdateResult], forceComponentUpdateDispatch] = useReducer( storeStateUpdatesReducer, EMPTY_ARRAY, initStateUpdates ) + // Propagate any mapState/mapDispatch errors upwards if (previousStateUpdateResult && previousStateUpdateResult.error) { throw previousStateUpdateResult.error } + // Set up refs to coordinate values between the subscription effect and the render logic const lastChildProps = useRef() const lastWrapperProps = useRef(wrapperProps) const childPropsFromStoreUpdate = useRef() const actualChildProps = usePureOnlyMemo(() => { + // Tricky logic here: + // - This render may have been triggered by a Redux store update that produced new child props + // - However, we may have gotten new wrapper props after that + // If we have new child props, and the same wrapper props, we know we should use the new child props as-is. + // But, if we have new wrapper props, those might change the child props, so we have to recalculate things. + // So, we'll use the child props from store update only if the wrapper props are the same as last time. if ( childPropsFromStoreUpdate.current && wrapperProps === lastWrapperProps.current @@ -224,24 +249,33 @@ export default function connectAdvanced( } // TODO We're reading the store directly in render() here. Bad idea? + // This will likely cause Bad Things (TM) to happen in Concurrent Mode. + // Note that we do this because on renders _not_ caused by store updates, we need the latest store state + // to determine what the child props should be. return childPropsSelector(store.getState(), wrapperProps) }, [store, previousStateUpdateResult, wrapperProps]) + // Every time we do re-render: useEffect(() => { + // We want to capture the wrapper props and child props we used for later comparisons lastWrapperProps.current = wrapperProps lastChildProps.current = actualChildProps + // If the render was from a store update, clear out that reference and cascade the subscriber update if (childPropsFromStoreUpdate.current) { childPropsFromStoreUpdate.current = null notifyNestedSubs() } }) + // Our re-subscribe logic only runs when the store/subscription setup changes useEffect(() => { + // If we're not subscribed to the store, nothing to do here if (!shouldHandleStateChanges) return let didUnsubscribe = false + // We'll run this callback every time a store subscription update propagates to this component const checkForUpdates = () => { if (didUnsubscribe) { // Don't run stale listeners. @@ -253,6 +287,8 @@ export default function connectAdvanced( let newChildProps, error try { + // Actually run the selector with the most recent store state and wrapper props + // to determine what the child props should be newChildProps = childPropsSelector( latestStoreState, lastWrapperProps.current @@ -261,16 +297,23 @@ export default function connectAdvanced( error = e } + // If the child props haven't changed, nothing to do here - cascade the subscription update if (newChildProps === lastChildProps.current) { notifyNestedSubs() } else { - dispatch({ + // If the child props _did_ change (or we caught an error), this wrapper component needs to re-render + forceComponentUpdateDispatch({ type: 'STORE_UPDATED', payload: { latestStoreState, error } }) + + // Save references to the new child props. Note that we track the "child props from store update" + // as a ref instead of a useState/useReducer because we need a way to determine if that value has + // been processed. If this went into useState/useReducer, we couldn't clear out the value without + // forcing another re-render, which we don't want. lastChildProps.current = newChildProps childPropsFromStoreUpdate.current = newChildProps } @@ -292,12 +335,20 @@ export default function connectAdvanced( return unsubscribeWrapper }, [store, subscription, childPropsSelector]) + // Now that all that's done, we can finally try to actually render the child component. + // We memoize the elements for the rendered child component as an optimization. + // If React sees the exact same element reference as last time, it bails out of re-rendering + // that child, same as if it was wrapped in React.memo() or returned false from shouldComponentUpdate. const renderedChild = useMemo(() => { + // Render the actual child component const renderedWrappedComponent = ( ) if (shouldHandleStateChanges) { + // If this component is subscribed to store updates, we need to pass its own + // subscription instance down to our descendants. That means rendering the same + // Context instance, and putting a different value into the context. return ( {renderedWrappedComponent} @@ -317,6 +368,7 @@ export default function connectAdvanced( return renderedChild } + // If we're in "pure" mode, ensure our wrapper component only re-renders when incoming props have changed. const Connect = pure ? React.memo(ConnectFunction) : ConnectFunction Connect.WrappedComponent = WrappedComponent From b855e16ce66f762996d6994452c0a95108b21d6e Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 18 Mar 2019 21:30:57 -0400 Subject: [PATCH 18/39] Re-add test for a custom store as a prop --- test/components/connect.spec.js | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 1c87539d1..7d9bffdca 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -103,6 +103,7 @@ describe('React', () => { const store = createStore(() => ({ hi: 'there' })) const Container = React.memo(props => ) // eslint-disable-line + Container.displayName = 'Container' const WrappedContainer = connect(state => state)(Container) const tester = rtl.render( @@ -137,6 +138,7 @@ describe('React', () => { ) + expect(tester.getByTestId('pass')).toHaveTextContent('through') expect(tester.getByTestId('foo')).toHaveTextContent('bar') expect(tester.getByTestId('baz')).toHaveTextContent('42') @@ -1610,6 +1612,32 @@ describe('React', () => { expect(actualState).toEqual(expectedState) }) + it('should use the store from the props instead of from the context if present', () => { + class Container extends Component { + render() { + return + } + } + + let actualState + + const expectedState = { foos: {} } + const decorator = connect(state => { + actualState = state + return {} + }) + const Decorated = decorator(Container) + const mockStore = { + dispatch: () => {}, + subscribe: () => {}, + getState: () => expectedState + } + + rtl.render() + + expect(actualState).toEqual(expectedState) + }) + it('should use a custom context provider and consumer if passed as a prop to the component', () => { class Container extends Component { render() { From 939d97644e166bcf8f95e1c5fe20b17a6c6a9dd8 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 18 Mar 2019 22:38:12 -0400 Subject: [PATCH 19/39] Fix null pointer exception when store is given as a prop We were trying to read contextValue.subscription, even if that value was null. Reworked logic to handle cases where the store came in as a prop. --- src/components/connectAdvanced.js | 37 ++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index da33b4c7c..376b8b2d7 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -168,10 +168,13 @@ export default function connectAdvanced( // Retrieve the store and ancestor subscription via context, if available const contextValue = useContext(ContextToUse) - // The store _must_ exist as either a prop or in context + const didStoreComeFromProps = Boolean(props.store) + const didStoreComeFromContext = + Boolean(contextValue) && Boolean(contextValue.store) + invariant( - props.store || contextValue, + didStoreComeFromProps || didStoreComeFromContext, `Could not find "store" in the context of ` + `"${displayName}". Either wrap the root component in a , ` + `or pass a custom React context provider to and the corresponding ` + @@ -179,7 +182,6 @@ export default function connectAdvanced( ) const store = props.store || contextValue.store - const propsMode = Boolean(props.store) const childPropsSelector = useMemo(() => { // The child props selector needs the store reference as an input. @@ -190,9 +192,12 @@ export default function connectAdvanced( const [subscription, notifyNestedSubs] = useMemo(() => { if (!shouldHandleStateChanges) return NO_SUBSCRIPTION_ARRAY - // parentSub's source should match where store came from: props vs. context. A component + // This Subscription'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 subscription = new Subscription(store, contextValue.subscription) + const subscription = new Subscription( + store, + didStoreComeFromProps ? null : contextValue.subscription + ) // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in // the middle of the notification loop, where `subscription` will then be null. This can @@ -203,10 +208,17 @@ export default function connectAdvanced( ) return [subscription, notifyNestedSubs] - }, [store, contextValue.subscription]) + }, [store, didStoreComeFromProps, contextValue]) - // Determine what {store, subscription} value should be put into nested context, if necessary + // Determine what {store, subscription} value should be put into nested context, if necessary, + // and memoize that value to avoid unnecessary context updates. const overriddenContextValue = useMemo(() => { + if (didStoreComeFromProps) { + // This component is directly subscribed to a store from props. + // We don't want descendants reading from this store - pass down whatever + // the existing context value is from the nearest connected ancestor. + return contextValue + } // Otherwise, put this component's subscription instance into context, so that // connected descendants won't update until after this component is done @@ -214,15 +226,14 @@ export default function connectAdvanced( ...contextValue, subscription } - }, [contextValue, subscription]) + }, [didStoreComeFromProps, contextValue, subscription]) // We need to force this wrapper component to re-render whenever a Redux store update // causes a change to the calculated child component props (or we caught an error in mapState) - const [[previousStateUpdateResult], forceComponentUpdateDispatch] = useReducer( - storeStateUpdatesReducer, - EMPTY_ARRAY, - initStateUpdates - ) + const [ + [previousStateUpdateResult], + forceComponentUpdateDispatch + ] = useReducer(storeStateUpdatesReducer, EMPTY_ARRAY, initStateUpdates) // Propagate any mapState/mapDispatch errors upwards if (previousStateUpdateResult && previousStateUpdateResult.error) { From eb03a372bde5a4ee129f40c17c80d9fe1d4399ac Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 18 Mar 2019 23:35:54 -0400 Subject: [PATCH 20/39] Ensure wrapper props are passed correctly when forwarding refs --- src/components/connectAdvanced.js | 2 +- test/components/connect.spec.js | 42 +++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 376b8b2d7..aa8ea6460 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -390,7 +390,7 @@ export default function connectAdvanced( props, ref ) { - return + return }) forwarded.displayName = displayName diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 7d9bffdca..23f04c76a 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1796,6 +1796,48 @@ describe('React', () => { done() }) + + + it('should correctly separate and pass through props to the wrapped component with a forwarded ref', async done => { + const store = createStore(() => ({})) + + class Container extends Component { + render() { + return + } + } + + const decorator = connect( + state => state, + null, + null, + { forwardRef: true } + ) + const Decorated = decorator(Container) + + const ref = React.createRef() + + class Wrapper extends Component { + render() { + // The 'a' prop should eventually be passed to the wrapped component individually, + // not sent through as `wrapperProps={ {a : 42} }` + return + } + } + + const tester = rtl.render( + + + + ) + + tester.debug() + await rtl.waitForElement(() => tester.getByTestId('a')) + expect(tester.getByTestId('a')).toHaveTextContent('42') + + done() + }) + it('should return the instance of the wrapped component for use in calling child methods, impure component', async done => { const store = createStore(() => ({})) From 53c3b79ec64c8ab3b1594ab6e616bd1216a2a82e Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 18 Mar 2019 23:59:25 -0400 Subject: [PATCH 21/39] Add a test to verify subscription passthrough with store-as-prop --- test/components/Provider.spec.js | 8 ++- test/components/connect.spec.js | 100 ++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/test/components/Provider.spec.js b/test/components/Provider.spec.js index c0cdeac82..d6dbf135d 100644 --- a/test/components/Provider.spec.js +++ b/test/components/Provider.spec.js @@ -3,8 +3,7 @@ import React, { Component } from 'react' import ReactDOM from 'react-dom' import { createStore } from 'redux' -import { Provider, connect } from '../../src/index.js' -import { ReactReduxContext } from '../../src/components/Context' +import { Provider, connect, ReactReduxContext } from '../../src/index.js' import * as rtl from 'react-testing-library' import 'jest-dom/extend-expect' @@ -221,7 +220,10 @@ describe('React', () => { ) expect(innerMapStateToProps).toHaveBeenCalledTimes(1) - innerStore.dispatch({ type: 'INC' }) + rtl.act(() => { + innerStore.dispatch({ type: 'INC' }) + }) + expect(innerMapStateToProps).toHaveBeenCalledTimes(2) }) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 23f04c76a..7b27fb2b0 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1638,6 +1638,104 @@ describe('React', () => { expect(actualState).toEqual(expectedState) }) + it('should pass through ancestor subscription when store is given as a prop', () => { + const c3Spy = jest.fn() + const c2Spy = jest.fn() + const c1Spy = jest.fn() + + const Comp3 = ({ first }) => { + c3Spy() + return + } + const ConnectedComp3 = connect(state => state)(Comp3) + + const Comp2 = ({ second }) => { + c2Spy() + return ( +
+ + +
+ ) + } + const ConnectedComp2 = connect(state => state)(Comp2) + + const Comp1 = ({ children, first }) => { + c1Spy() + return ( +
+ + {children} +
+ ) + } + const ConnectedComp1 = connect(state => state)(Comp1) + + const reducer1 = (state = { first: '1' }, action) => { + switch (action.type) { + case 'CHANGE': + return { first: '2' } + default: + return state + } + } + + const reducer2 = (state = { second: '3' }, action) => { + switch (action.type) { + case 'CHANGE': + return { second: '4' } + default: + return state + } + } + + const store1 = createStore(reducer1) + const store2 = createStore(reducer2) + + const tester = rtl.render( + + + + + + ) + + // Initial render: C1/C3 read from store 1, C2 reads from store 2, one render each + expect(tester.getByTestId('a')).toHaveTextContent('1') + expect(tester.getByTestId('b')).toHaveTextContent('3') + expect(tester.getByTestId('c')).toHaveTextContent('1') + + expect(c3Spy).toHaveBeenCalledTimes(1) + expect(c2Spy).toHaveBeenCalledTimes(1) + expect(c1Spy).toHaveBeenCalledTimes(1) + + rtl.act(() => { + store1.dispatch({ type: 'CHANGE' }) + }) + + // Store 1 update: C1 and C3 should re-render, no updates for C2 + expect(tester.getByTestId('a')).toHaveTextContent('2') + expect(tester.getByTestId('b')).toHaveTextContent('3') + expect(tester.getByTestId('c')).toHaveTextContent('2') + + expect(c3Spy).toHaveBeenCalledTimes(2) + expect(c2Spy).toHaveBeenCalledTimes(1) + expect(c1Spy).toHaveBeenCalledTimes(2) + + rtl.act(() => { + store2.dispatch({ type: 'CHANGE' }) + }) + + // Store 2 update: C2 should re-render, no updates for C1 or C3 + expect(tester.getByTestId('a')).toHaveTextContent('2') + expect(tester.getByTestId('b')).toHaveTextContent('4') + expect(tester.getByTestId('c')).toHaveTextContent('2') + + expect(c3Spy).toHaveBeenCalledTimes(2) + expect(c2Spy).toHaveBeenCalledTimes(2) + expect(c1Spy).toHaveBeenCalledTimes(2) + }) + it('should use a custom context provider and consumer if passed as a prop to the component', () => { class Container extends Component { render() { @@ -1796,8 +1894,6 @@ describe('React', () => { done() }) - - it('should correctly separate and pass through props to the wrapped component with a forwarded ref', async done => { const store = createStore(() => ({})) From 4401bf84791d283de505231d5e71666fce2b68ad Mon Sep 17 00:00:00 2001 From: Max Kostow Date: Thu, 21 Mar 2019 16:53:10 -0700 Subject: [PATCH 22/39] add non-batched tests (#1208) --- test/components/connect.spec.js | 103 +++++++++++++++++++++++++++++++- 1 file changed, 102 insertions(+), 1 deletion(-) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 7b27fb2b0..65a8581ed 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -4,7 +4,7 @@ import React, { Component } from 'react' import createClass from 'create-react-class' import PropTypes from 'prop-types' import ReactDOM from 'react-dom' -import { createStore } from 'redux' +import { createStore, applyMiddleware } from 'redux' import { Provider as ProviderMock, connect } from '../../src/index.js' import * as rtl from 'react-testing-library' import 'jest-dom/extend-expect' @@ -2954,5 +2954,106 @@ describe('React', () => { connect()(createComp('div')) }).not.toThrow() }) + + it('should invoke mapState always with latest props', () => { + const store = createStore((state = 0) => state + 1) + + let propsPassedIn + + @connect(reduxCount => { + return { reduxCount } + }) + class InnerComponent extends Component { + render() { + propsPassedIn = this.props + return + } + } + + class OuterComponent extends Component { + constructor() { + super() + this.state = { count: 0 } + } + + render() { + return + } + } + + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) + outerComponent.setState(({ count }) => ({ count: count + 1 })) + store.dispatch({ type: '' }) + + expect(propsPassedIn.count).toEqual(1) + expect(propsPassedIn.reduxCount).toEqual(2) + }) + + it('should use the latest props when updated between actions', () => { + const store = applyMiddleware(store => { + let callback + return next => action => { + if (action.type === 'SET_COMPONENT_CALLBACK') { + callback = action.payload + } + if (callback && action.type === 'INC1') { + next(action) + callback() + store.dispatch({ type: 'INC2' }) + return + } + next(action) + } + })(createStore)((state = 0, action) => { + if (action.type === 'INC1') { + return state + 1 + } else if (action.type === 'INC2') { + return state + 2 + } + return state + }) + const Child = connect(count => ({ count }))(function(props) { + return ( +
+ ) + }) + class Parent extends Component { + constructor() { + super() + this.state = { + prop: 'a' + } + this.inc1 = () => store.dispatch({ type: 'INC1' }) + store.dispatch({ + type: 'SET_COMPONENT_CALLBACK', + payload: () => this.setState({ prop: 'b' }) + }) + } + + render() { + return ( + + + + ) + } + } + let parent + const rendered = rtl.render( (parent = ref)} />) + expect(rendered.getByTestId('child').dataset.count).toEqual('0') + expect(rendered.getByTestId('child').dataset.prop).toEqual('a') + parent.inc1() + expect(rendered.getByTestId('child').dataset.count).toEqual('3') + expect(rendered.getByTestId('child').dataset.prop).toEqual('b') + }) }) }) From b578e963e5a334019acef2ba87b0d3af93c32d85 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Thu, 21 Mar 2019 19:53:54 -0400 Subject: [PATCH 23/39] Force SSR tests to mimic a Node environment --- test/integration/server-rendering.spec.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/integration/server-rendering.spec.js b/test/integration/server-rendering.spec.js index b660a51d9..5bf44e20e 100644 --- a/test/integration/server-rendering.spec.js +++ b/test/integration/server-rendering.spec.js @@ -1,3 +1,10 @@ +/** + * @jest-environment node + * Set this so that `window` is undefined to correctly mimic a Node SSR scenario. + * That allows connectAdvanced to fall back to `useEffect` instead of `useLayoutEffect` + * to avoid ugly console warnings when used with SSR. + */ + /*eslint-disable react/prop-types*/ import React from 'react' From e77e005d350d7c788b6615ee7bb7e16e847457db Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Thu, 21 Mar 2019 20:33:54 -0400 Subject: [PATCH 24/39] Restructure connect tests to group by category for easier reading Yeah, this kills the blame history. Sorry. But it's a lot easier to figure out what the tests are used for now. --- test/components/connect.spec.js | 4876 ++++++++++++++++--------------- 1 file changed, 2466 insertions(+), 2410 deletions(-) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 65a8581ed..150dc0732 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -79,2981 +79,3037 @@ describe('React', () => { afterEach(() => rtl.cleanup()) - it('should receive the store state in the context', () => { - const store = createStore(() => ({ hi: 'there' })) - - @connect(state => state) - class Container extends Component { - render() { - return - } - } - - const tester = rtl.render( - - - - ) - - expect(tester.getByTestId('hi')).toHaveTextContent('there') - }) - - it('Should work with a memo component, if it exists', () => { - if (React.memo) { + describe('Core subscription and prop passing behavior', () => { + it('should receive the store state in the context', () => { const store = createStore(() => ({ hi: 'there' })) - const Container = React.memo(props => ) // eslint-disable-line - Container.displayName = 'Container' - const WrappedContainer = connect(state => state)(Container) + @connect(state => state) + class Container extends Component { + render() { + return + } + } const tester = rtl.render( - + ) expect(tester.getByTestId('hi')).toHaveTextContent('there') - } - }) + }) - it('should pass state and props to the given component', () => { - const store = createStore(() => ({ - foo: 'bar', - baz: 42, - hello: 'world' - })) - - @connect( - ({ foo, baz }) => ({ foo, baz }), - {} - ) - class Container extends Component { - render() { - return + it('should pass state and props to the given component', () => { + const store = createStore(() => ({ + foo: 'bar', + baz: 42, + hello: 'world' + })) + + @connect( + ({ foo, baz }) => ({ foo, baz }), + {} + ) + class Container extends Component { + render() { + return + } } - } - const tester = rtl.render( - - - - ) + const tester = rtl.render( + + + + ) - expect(tester.getByTestId('pass')).toHaveTextContent('through') - expect(tester.getByTestId('foo')).toHaveTextContent('bar') - expect(tester.getByTestId('baz')).toHaveTextContent('42') - expect(tester.queryByTestId('hello')).toBe(null) - }) + expect(tester.getByTestId('pass')).toHaveTextContent('through') + expect(tester.getByTestId('foo')).toHaveTextContent('bar') + expect(tester.getByTestId('baz')).toHaveTextContent('42') + expect(tester.queryByTestId('hello')).toBe(null) + }) - it('should subscribe class components to the store changes', () => { - const store = createStore(stringBuilder) + it('should subscribe class components to the store changes', () => { + const store = createStore(stringBuilder) - @connect(state => ({ string: state })) - class Container extends Component { - render() { - return + @connect(state => ({ string: state })) + class Container extends Component { + render() { + return + } } - } - const tester = rtl.render( - - - - ) - expect(tester.getByTestId('string')).toHaveTextContent('') + const tester = rtl.render( + + + + ) + expect(tester.getByTestId('string')).toHaveTextContent('') - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) - }) - expect(tester.getByTestId('string')).toHaveTextContent('a') + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + expect(tester.getByTestId('string')).toHaveTextContent('a') - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'b' }) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'b' }) + }) + expect(tester.getByTestId('string')).toHaveTextContent('ab') }) - expect(tester.getByTestId('string')).toHaveTextContent('ab') - }) - it('should subscribe pure function components to the store changes', () => { - const store = createStore(stringBuilder) + it('should subscribe pure function components to the store changes', () => { + const store = createStore(stringBuilder) - const Container = connect(state => ({ string: state }))( - function Container(props) { - return - } - ) + const Container = connect(state => ({ string: state }))( + function Container(props) { + return + } + ) - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + + const tester = rtl.render( + + + + ) + expect(spy).toHaveBeenCalledTimes(0) + spy.mockRestore() - const tester = rtl.render( - - - - ) - expect(spy).toHaveBeenCalledTimes(0) - spy.mockRestore() + expect(tester.getByTestId('string')).toHaveTextContent('') + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) - expect(tester.getByTestId('string')).toHaveTextContent('') - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) - }) + expect(tester.getByTestId('string')).toHaveTextContent('a') + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'b' }) + }) - expect(tester.getByTestId('string')).toHaveTextContent('a') - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'b' }) + expect(tester.getByTestId('string')).toHaveTextContent('ab') }) - expect(tester.getByTestId('string')).toHaveTextContent('ab') - }) + it("should retain the store's context", () => { + const store = new ContextBoundStore(stringBuilder) - it("should retain the store's context", () => { - const store = new ContextBoundStore(stringBuilder) + let Container = connect(state => ({ string: state }))( + function Container(props) { + return + } + ) - let Container = connect(state => ({ string: state }))(function Container( - props - ) { - return - }) + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + const tester = rtl.render( + + + + ) + expect(spy).toHaveBeenCalledTimes(0) + spy.mockRestore() - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - const tester = rtl.render( - - - - ) - expect(spy).toHaveBeenCalledTimes(0) - spy.mockRestore() + expect(tester.getByTestId('string')).toHaveTextContent('') + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) - expect(tester.getByTestId('string')).toHaveTextContent('') - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) + expect(tester.getByTestId('string')).toHaveTextContent('a') }) - expect(tester.getByTestId('string')).toHaveTextContent('a') - }) - - it('should handle dispatches before componentDidMount', () => { - const store = createStore(stringBuilder) + it('should throw an error if the store is not in the props or context', () => { + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - @connect(state => ({ string: state })) - class Container extends Component { - componentDidMount() { - store.dispatch({ type: 'APPEND', body: 'a' }) + class Container extends Component { + render() { + return + } } - render() { - return - } - } - const tester = rtl.render( - - - - ) - expect(tester.getByTestId('string')).toHaveTextContent('a') + const decorator = connect(() => {}) + const Decorated = decorator(Container) + + expect(() => rtl.render()).toThrow( + /Could not find "store"/ + ) + + spy.mockRestore() + }) }) - it('should handle additional prop changes in addition to slice', () => { - const store = createStore(() => ({ - foo: 'bar' - })) + describe('Prop merging', () => { + it('should handle additional prop changes in addition to slice', () => { + const store = createStore(() => ({ + foo: 'bar' + })) - @connect(state => state) - class ConnectContainer extends Component { - render() { - return + @connect(state => state) + class ConnectContainer extends Component { + render() { + return + } } - } - class Container extends Component { - constructor() { - super() - this.state = { - bar: { - baz: '' + class Container extends Component { + constructor() { + super() + this.state = { + bar: { + baz: '' + } } } - } - componentDidMount() { - this.setState({ - bar: Object.assign({}, this.state.bar, { baz: 'through' }) - }) - } + componentDidMount() { + this.setState({ + bar: Object.assign({}, this.state.bar, { baz: 'through' }) + }) + } - render() { - return ( - - - - ) + render() { + return ( + + + + ) + } } - } - const tester = rtl.render() + const tester = rtl.render() - expect(tester.getByTestId('foo')).toHaveTextContent('bar') - expect(tester.getByTestId('pass')).toHaveTextContent('through') - }) + expect(tester.getByTestId('foo')).toHaveTextContent('bar') + expect(tester.getByTestId('pass')).toHaveTextContent('through') + }) - it('should handle unexpected prop changes with forceUpdate()', () => { - const store = createStore(() => ({})) + it('should handle unexpected prop changes with forceUpdate()', () => { + const store = createStore(() => ({})) - @connect(state => state) - class ConnectContainer extends Component { - render() { - return + @connect(state => state) + class ConnectContainer extends Component { + render() { + return + } } - } - class Container extends Component { - constructor() { - super() - this.bar = 'baz' - } + class Container extends Component { + constructor() { + super() + this.bar = 'baz' + } - componentDidMount() { - this.bar = 'foo' - this.forceUpdate() - } + componentDidMount() { + this.bar = 'foo' + this.forceUpdate() + } - render() { - return ( - - - - ) + render() { + return ( + + + + ) + } } - } - const tester = rtl.render() + const tester = rtl.render() - expect(tester.getByTestId('bar')).toHaveTextContent('foo') - }) + expect(tester.getByTestId('bar')).toHaveTextContent('foo') + }) - it('should remove undefined props', () => { - const store = createStore(() => ({})) - let props = { x: true } - let container + it('should remove undefined props', () => { + const store = createStore(() => ({})) + let props = { x: true } + let container - @connect( - () => ({}), - () => ({}) - ) - class ConnectContainer extends Component { - render() { - return + @connect( + () => ({}), + () => ({}) + ) + class ConnectContainer extends Component { + render() { + return + } } - } - class HolderContainer extends Component { - render() { - return + class HolderContainer extends Component { + render() { + return + } } - } - const tester = rtl.render( - - (container = instance)} /> - - ) + const tester = rtl.render( + + (container = instance)} /> + + ) - expect(tester.getByTestId('x')).toHaveTextContent('true') + expect(tester.getByTestId('x')).toHaveTextContent('true') - props = {} - container.forceUpdate() + props = {} + container.forceUpdate() - expect(tester.queryByTestId('x')).toBe(null) - }) + expect(tester.queryByTestId('x')).toBe(null) + }) - it('should remove undefined props without mapDispatch', () => { - const store = createStore(() => ({})) - let props = { x: true } - let container + it('should remove undefined props without mapDispatch', () => { + const store = createStore(() => ({})) + let props = { x: true } + let container - @connect(() => ({})) - class ConnectContainer extends Component { - render() { - return + @connect(() => ({})) + class ConnectContainer extends Component { + render() { + return + } } - } - class HolderContainer extends Component { - render() { - return + class HolderContainer extends Component { + render() { + return + } } - } - const tester = rtl.render( - - (container = instance)} /> - - ) + const tester = rtl.render( + + (container = instance)} /> + + ) - expect(tester.getAllByTitle('prop').length).toBe(2) - expect(tester.getByTestId('dispatch')).toHaveTextContent( - '[function dispatch]' - ) - expect(tester.getByTestId('x')).toHaveTextContent('true') + expect(tester.getAllByTitle('prop').length).toBe(2) + expect(tester.getByTestId('dispatch')).toHaveTextContent( + '[function dispatch]' + ) + expect(tester.getByTestId('x')).toHaveTextContent('true') - props = {} - container.forceUpdate() + props = {} + container.forceUpdate() - expect(tester.getAllByTitle('prop').length).toBe(1) - expect(tester.getByTestId('dispatch')).toHaveTextContent( - '[function dispatch]' - ) - }) + expect(tester.getAllByTitle('prop').length).toBe(1) + expect(tester.getByTestId('dispatch')).toHaveTextContent( + '[function dispatch]' + ) + }) - it('should ignore deep mutations in props', () => { - const store = createStore(() => ({ - foo: 'bar' - })) + it('should ignore deep mutations in props', () => { + const store = createStore(() => ({ + foo: 'bar' + })) - @connect(state => state) - class ConnectContainer extends Component { - render() { - return + @connect(state => state) + class ConnectContainer extends Component { + render() { + return + } } - } - class Container extends Component { - constructor() { - super() - this.state = { - bar: { - baz: '' + class Container extends Component { + constructor() { + super() + this.state = { + bar: { + baz: '' + } } } - } - componentDidMount() { - // Simulate deep object mutation - const bar = this.state.bar - bar.baz = 'through' - this.setState({ - bar - }) - } + componentDidMount() { + // Simulate deep object mutation + const bar = this.state.bar + bar.baz = 'through' + this.setState({ + bar + }) + } - render() { - return ( - - - - ) + render() { + return ( + + + + ) + } } - } - const tester = rtl.render() - expect(tester.getByTestId('foo')).toHaveTextContent('bar') - expect(tester.getByTestId('pass')).toHaveTextContent('') - }) + const tester = rtl.render() + expect(tester.getByTestId('foo')).toHaveTextContent('bar') + expect(tester.getByTestId('pass')).toHaveTextContent('') + }) - it('should allow for merge to incorporate state and prop changes', () => { - const store = createStore(stringBuilder) + it('should allow for merge to incorporate state and prop changes', () => { + const store = createStore(stringBuilder) - function doSomething(thing) { - return { - type: 'APPEND', - body: thing + function doSomething(thing) { + return { + type: 'APPEND', + body: thing + } } - } - let merged - let externalSetState - @connect( - state => ({ stateThing: state }), - dispatch => ({ - doSomething: whatever => dispatch(doSomething(whatever)) - }), - (stateProps, actionProps, parentProps) => ({ - ...stateProps, - ...actionProps, - mergedDoSomething: (() => { - merged = function mergedDoSomething(thing) { - const seed = stateProps.stateThing === '' ? 'HELLO ' : '' - actionProps.doSomething(seed + thing + parentProps.extra) - } - return merged - })() - }) - ) - class Container extends Component { - render() { - return + let merged + let externalSetState + @connect( + state => ({ stateThing: state }), + dispatch => ({ + doSomething: whatever => dispatch(doSomething(whatever)) + }), + (stateProps, actionProps, parentProps) => ({ + ...stateProps, + ...actionProps, + mergedDoSomething: (() => { + merged = function mergedDoSomething(thing) { + const seed = stateProps.stateThing === '' ? 'HELLO ' : '' + actionProps.doSomething(seed + thing + parentProps.extra) + } + return merged + })() + }) + ) + class Container extends Component { + render() { + return + } } - } - class OuterContainer extends Component { - constructor() { - super() - this.state = { extra: 'z' } - externalSetState = this.setState.bind(this) - } + class OuterContainer extends Component { + constructor() { + super() + this.state = { extra: 'z' } + externalSetState = this.setState.bind(this) + } - render() { - return ( - - - - ) + render() { + return ( + + + + ) + } } - } - const tester = rtl.render() + const tester = rtl.render() - expect(tester.getByTestId('stateThing')).toHaveTextContent('') - rtl.act(() => { - merged('a') - }) + expect(tester.getByTestId('stateThing')).toHaveTextContent('') + rtl.act(() => { + merged('a') + }) - expect(tester.getByTestId('stateThing')).toHaveTextContent('HELLO az') - rtl.act(() => { - merged('b') - }) + expect(tester.getByTestId('stateThing')).toHaveTextContent('HELLO az') + rtl.act(() => { + merged('b') + }) - expect(tester.getByTestId('stateThing')).toHaveTextContent('HELLO azbz') - rtl.act(() => { - externalSetState({ extra: 'Z' }) - }) + expect(tester.getByTestId('stateThing')).toHaveTextContent('HELLO azbz') + rtl.act(() => { + externalSetState({ extra: 'Z' }) + }) - rtl.act(() => { - merged('c') - }) + rtl.act(() => { + merged('c') + }) - expect(tester.getByTestId('stateThing')).toHaveTextContent('HELLO azbzcZ') - }) + expect(tester.getByTestId('stateThing')).toHaveTextContent( + 'HELLO azbzcZ' + ) + }) - it('should merge actionProps into WrappedComponent', () => { - const store = createStore(() => ({ - foo: 'bar' - })) + it('should merge actionProps into WrappedComponent', () => { + const store = createStore(() => ({ + foo: 'bar' + })) - const exampleActionCreator = () => {} + const exampleActionCreator = () => {} - @connect( - state => state, - () => ({ exampleActionCreator }) - ) - class Container extends Component { - render() { - return + @connect( + state => state, + () => ({ exampleActionCreator }) + ) + class Container extends Component { + render() { + return + } } - } - - const tester = rtl.render( - - - - ) - - expect(tester.getByTestId('exampleActionCreator')).toHaveTextContent( - '[function exampleActionCreator]' - ) - expect(tester.getByTestId('foo')).toHaveTextContent('bar') - }) - it('should not invoke mapState when props change if it only has one argument', () => { - const store = createStore(stringBuilder) - - let invocationCount = 0 + const tester = rtl.render( + + + + ) - /*eslint-disable no-unused-vars */ - @connect(arg1 => { - invocationCount++ - return {} + expect(tester.getByTestId('exampleActionCreator')).toHaveTextContent( + '[function exampleActionCreator]' + ) + expect(tester.getByTestId('foo')).toHaveTextContent('bar') }) - /*eslint-enable no-unused-vars */ - class WithoutProps extends Component { - render() { - return - } - } - - class OuterComponent extends Component { - constructor() { - super() - this.state = { foo: 'FOO' } - } - setFoo(foo) { - this.setState({ foo }) - } + it('should throw an error if mapState, mapDispatch, or mergeProps returns anything but a plain object', () => { + const store = createStore(() => ({})) - render() { - return ( -
- -
+ function makeContainer(mapState, mapDispatch, mergeProps) { + @connect( + mapState, + mapDispatch, + mergeProps ) + class Container extends Component { + render() { + return + } + } + return React.createElement(Container) } - } - - let outerComponent - rtl.render( - - (outerComponent = c)} /> - - ) - outerComponent.setFoo('BAR') - outerComponent.setFoo('DID') - expect(invocationCount).toEqual(1) - }) - - it('should invoke mapState every time props are changed if it has zero arguments', () => { - const store = createStore(stringBuilder) + function AwesomeMap() {} - let invocationCount = 0 + let spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.render( + + {makeContainer(() => 1, () => ({}), () => ({}))} + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toMatch( + /mapStateToProps\(\) in Connect\(Container\) must return a plain object/ + ) + spy.mockRestore() + rtl.cleanup() - @connect(() => { - invocationCount++ - return {} - }) - class WithoutProps extends Component { - render() { - return - } - } + spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.render( + + {makeContainer(() => 'hey', () => ({}), () => ({}))} + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toMatch( + /mapStateToProps\(\) in Connect\(Container\) must return a plain object/ + ) + spy.mockRestore() + rtl.cleanup() - class OuterComponent extends Component { - constructor() { - super() - this.state = { foo: 'FOO' } - } + spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.render( + + {makeContainer(() => new AwesomeMap(), () => ({}), () => ({}))} + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toMatch( + /mapStateToProps\(\) in Connect\(Container\) must return a plain object/ + ) + spy.mockRestore() + rtl.cleanup() - setFoo(foo) { - this.setState({ foo }) - } + spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.render( + + {makeContainer(() => ({}), () => 1, () => ({}))} + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toMatch( + /mapDispatchToProps\(\) in Connect\(Container\) must return a plain object/ + ) + spy.mockRestore() + rtl.cleanup() - render() { - return ( -
- -
- ) - } - } + spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.render( + + {makeContainer(() => ({}), () => 'hey', () => ({}))} + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toMatch( + /mapDispatchToProps\(\) in Connect\(Container\) must return a plain object/ + ) + spy.mockRestore() + rtl.cleanup() - let outerComponent - rtl.render( - - (outerComponent = c)} /> - - ) - outerComponent.setFoo('BAR') - outerComponent.setFoo('DID') + spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.render( + + {makeContainer(() => ({}), () => new AwesomeMap(), () => ({}))} + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toMatch( + /mapDispatchToProps\(\) in Connect\(Container\) must return a plain object/ + ) + spy.mockRestore() + rtl.cleanup() + + spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.render( + + {makeContainer(() => ({}), () => ({}), () => 1)} + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toMatch( + /mergeProps\(\) in Connect\(Container\) must return a plain object/ + ) + spy.mockRestore() + rtl.cleanup() + + spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.render( + + {makeContainer(() => ({}), () => ({}), () => 'hey')} + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toMatch( + /mergeProps\(\) in Connect\(Container\) must return a plain object/ + ) + spy.mockRestore() + rtl.cleanup() - expect(invocationCount).toEqual(3) + spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.render( + + {makeContainer(() => ({}), () => ({}), () => new AwesomeMap())} + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(spy.mock.calls[0][0]).toMatch( + /mergeProps\(\) in Connect\(Container\) must return a plain object/ + ) + spy.mockRestore() + }) }) - it('should invoke mapState every time props are changed if it has a second argument', () => { - const store = createStore(stringBuilder) + describe('Invocation behavior for mapState/mapDispatch based on number of arguments', () => { + it('should not invoke mapState when props change if it only has one argument', () => { + const store = createStore(stringBuilder) - let propsPassedIn - let invocationCount = 0 + let invocationCount = 0 - @connect((state, props) => { - invocationCount++ - propsPassedIn = props - return {} - }) - class WithProps extends Component { - render() { - return + /*eslint-disable no-unused-vars */ + @connect(arg1 => { + invocationCount++ + return {} + }) + /*eslint-enable no-unused-vars */ + class WithoutProps extends Component { + render() { + return + } } - } - class OuterComponent extends Component { - constructor() { - super() - this.state = { foo: 'FOO' } - } + class OuterComponent extends Component { + constructor() { + super() + this.state = { foo: 'FOO' } + } - setFoo(foo) { - this.setState({ foo }) + setFoo(foo) { + this.setState({ foo }) + } + + render() { + return ( +
+ +
+ ) + } } - render() { - return ( -
- -
- ) + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) + outerComponent.setFoo('BAR') + outerComponent.setFoo('DID') + + expect(invocationCount).toEqual(1) + }) + + it('should invoke mapState every time props are changed if it has zero arguments', () => { + const store = createStore(stringBuilder) + + let invocationCount = 0 + + @connect(() => { + invocationCount++ + return {} + }) + class WithoutProps extends Component { + render() { + return + } } - } - let outerComponent - rtl.render( - - (outerComponent = c)} /> - - ) + class OuterComponent extends Component { + constructor() { + super() + this.state = { foo: 'FOO' } + } + + setFoo(foo) { + this.setState({ foo }) + } - outerComponent.setFoo('BAR') - outerComponent.setFoo('BAZ') + render() { + return ( +
+ +
+ ) + } + } - expect(invocationCount).toEqual(3) - expect(propsPassedIn).toEqual({ - foo: 'BAZ' + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) + outerComponent.setFoo('BAR') + outerComponent.setFoo('DID') + + expect(invocationCount).toEqual(3) }) - }) - it('should not invoke mapDispatch when props change if it only has one argument', () => { - const store = createStore(stringBuilder) + it('should invoke mapState every time props are changed if it has a second argument', () => { + const store = createStore(stringBuilder) - let invocationCount = 0 + let propsPassedIn + let invocationCount = 0 - /*eslint-disable no-unused-vars */ - @connect( - null, - arg1 => { + @connect((state, props) => { invocationCount++ + propsPassedIn = props return {} + }) + class WithProps extends Component { + render() { + return + } } - ) - /*eslint-enable no-unused-vars */ - class WithoutProps extends Component { - render() { - return - } - } - class OuterComponent extends Component { - constructor() { - super() - this.state = { foo: 'FOO' } + class OuterComponent extends Component { + constructor() { + super() + this.state = { foo: 'FOO' } + } + + setFoo(foo) { + this.setState({ foo }) + } + + render() { + return ( +
+ +
+ ) + } } - setFoo(foo) { - this.setState({ foo }) + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) + + outerComponent.setFoo('BAR') + outerComponent.setFoo('BAZ') + + expect(invocationCount).toEqual(3) + expect(propsPassedIn).toEqual({ + foo: 'BAZ' + }) + }) + + it('should not invoke mapDispatch when props change if it only has one argument', () => { + const store = createStore(stringBuilder) + + let invocationCount = 0 + + /*eslint-disable no-unused-vars */ + @connect( + null, + arg1 => { + invocationCount++ + return {} + } + ) + /*eslint-enable no-unused-vars */ + class WithoutProps extends Component { + render() { + return + } } - render() { - return ( -
- -
- ) + class OuterComponent extends Component { + constructor() { + super() + this.state = { foo: 'FOO' } + } + + setFoo(foo) { + this.setState({ foo }) + } + + render() { + return ( +
+ +
+ ) + } } - } - let outerComponent - rtl.render( - - (outerComponent = c)} /> - - ) + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) - outerComponent.setFoo('BAR') - outerComponent.setFoo('DID') + outerComponent.setFoo('BAR') + outerComponent.setFoo('DID') - expect(invocationCount).toEqual(1) - }) + expect(invocationCount).toEqual(1) + }) - it('should invoke mapDispatch every time props are changed if it has zero arguments', () => { - const store = createStore(stringBuilder) + it('should invoke mapDispatch every time props are changed if it has zero arguments', () => { + const store = createStore(stringBuilder) - let invocationCount = 0 + let invocationCount = 0 - @connect( - null, - () => { - invocationCount++ - return {} - } - ) - class WithoutProps extends Component { - render() { - return + @connect( + null, + () => { + invocationCount++ + return {} + } + ) + class WithoutProps extends Component { + render() { + return + } } - } - class OuterComponent extends Component { - constructor() { - super() - this.state = { foo: 'FOO' } + class OuterComponent extends Component { + constructor() { + super() + this.state = { foo: 'FOO' } + } + + setFoo(foo) { + this.setState({ foo }) + } + + render() { + return ( +
+ +
+ ) + } } - setFoo(foo) { - this.setState({ foo }) + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) + + outerComponent.setFoo('BAR') + outerComponent.setFoo('DID') + + expect(invocationCount).toEqual(3) + }) + + it('should invoke mapDispatch every time props are changed if it has a second argument', () => { + const store = createStore(stringBuilder) + + let propsPassedIn + let invocationCount = 0 + + @connect( + null, + (dispatch, props) => { + invocationCount++ + propsPassedIn = props + return {} + } + ) + class WithProps extends Component { + render() { + return + } } - render() { - return ( -
- -
- ) + class OuterComponent extends Component { + constructor() { + super() + this.state = { foo: 'FOO' } + } + + setFoo(foo) { + this.setState({ foo }) + } + + render() { + return ( +
+ +
+ ) + } } - } - let outerComponent - rtl.render( - - (outerComponent = c)} /> - - ) + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) - outerComponent.setFoo('BAR') - outerComponent.setFoo('DID') + outerComponent.setFoo('BAR') + outerComponent.setFoo('BAZ') - expect(invocationCount).toEqual(3) + expect(invocationCount).toEqual(3) + expect(propsPassedIn).toEqual({ + foo: 'BAZ' + }) + }) }) - it('should invoke mapDispatch every time props are changed if it has a second argument', () => { - const store = createStore(stringBuilder) + describe('React lifeycle interactions', () => { + it('should handle dispatches before componentDidMount', () => { + const store = createStore(stringBuilder) - let propsPassedIn - let invocationCount = 0 + @connect(state => ({ string: state })) + class Container extends Component { + componentDidMount() { + store.dispatch({ type: 'APPEND', body: 'a' }) + } - @connect( - null, - (dispatch, props) => { - invocationCount++ - propsPassedIn = props - return {} + render() { + return + } + } + const tester = rtl.render( + + + + ) + expect(tester.getByTestId('string')).toHaveTextContent('a') + }) + + it('should not attempt to notify unmounted child of state change', () => { + const store = createStore(stringBuilder) + + @connect(state => ({ hide: state === 'AB' })) + class App extends Component { + render() { + return this.props.hide ? null : + } } - ) - class WithProps extends Component { - render() { - return + + @connect(() => ({})) + class Container extends Component { + render() { + return + } } - } - class OuterComponent extends Component { - constructor() { - super() - this.state = { foo: 'FOO' } + @connect(state => ({ state })) + class Child extends Component { + componentDidMount() { + if (this.props.state === 'A') { + store.dispatch({ type: 'APPEND', body: 'B' }) + } + } + render() { + return null + } } - setFoo(foo) { - this.setState({ foo }) + const div = document.createElement('div') + ReactDOM.render( + + + , + div + ) + + try { + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'A' }) + }) + } finally { + ReactDOM.unmountComponentAtNode(div) } + }) + + it('should not attempt to set state after unmounting nested components', () => { + const store = createStore(() => ({})) + let mapStateToPropsCalls = 0 + + let linkA, linkB - render() { + let App = ({ children, setLocation }) => { + const onClick = to => event => { + event.preventDefault() + setLocation(to) + } + /* eslint-disable react/jsx-no-bind */ return ( ) + /* eslint-enable react/jsx-no-bind */ } - } + App = connect(() => ({}))(App) - let outerComponent - rtl.render( - - (outerComponent = c)} /> - - ) + let A = () =>

A

+ function mapState(state) { + const calls = ++mapStateToPropsCalls + return { calls, state } + } + A = connect(mapState)(A) - outerComponent.setFoo('BAR') - outerComponent.setFoo('BAZ') + const B = () =>

B

- expect(invocationCount).toEqual(3) - expect(propsPassedIn).toEqual({ - foo: 'BAZ' - }) - }) + class RouterMock extends React.Component { + constructor(...args) { + super(...args) + this.state = { location: { pathname: 'a' } } + this.setLocation = this.setLocation.bind(this) + } + + setLocation(pathname) { + this.setState({ location: { pathname } }) + store.dispatch({ type: 'TEST' }) + } - it('should pass dispatch and avoid subscription if arguments are falsy', () => { - const store = createStore(() => ({ - foo: 'bar' - })) + getChildComponent(location) { + switch (location) { + case 'a': + return + case 'b': + return + default: + throw new Error('Unknown location: ' + location) + } + } - function runCheck(...connectArgs) { - @connect(...connectArgs) - class Container extends Component { render() { - return + return ( + + {this.getChildComponent(this.state.location.pathname)} + + ) } } - const tester = rtl.render( + const div = document.createElement('div') + document.body.appendChild(div) + ReactDOM.render( - - - ) - expect(tester.getByTestId('dispatch')).toHaveTextContent( - '[function dispatch]' + + , + div ) - expect(tester.queryByTestId('foo')).toBe(null) - expect(tester.getByTestId('pass')).toHaveTextContent('through') - } - runCheck() - runCheck(null, null, null) - runCheck(false, false, false) - }) - - it('should not attempt to set state after unmounting', () => { - const store = createStore(stringBuilder) - let mapStateToPropsCalls = 0 + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - @connect( - () => ({ calls: ++mapStateToPropsCalls }), - dispatch => ({ dispatch }) - ) - class Container extends Component { - render() { - return - } - } + linkA.click() + linkB.click() + linkB.click() - const div = document.createElement('div') - store.subscribe(() => { - ReactDOM.unmountComponentAtNode(div) + document.body.removeChild(div) + // Called 3 times: + // - Initial mount + // - After first link click, stil mounted + // - After second link click, but the queued state update is discarded due to batching as it's unmounted + expect(mapStateToPropsCalls).toBe(3) + expect(spy).toHaveBeenCalledTimes(0) + spy.mockRestore() }) - rtl.act(() => { + it('should not attempt to set state when dispatching in componentWillUnmount', () => { + const store = createStore(stringBuilder) + let mapStateToPropsCalls = 0 + + /*eslint-disable no-unused-vars */ + @connect( + state => ({ calls: mapStateToPropsCalls++ }), + dispatch => ({ dispatch }) + ) + /*eslint-enable no-unused-vars */ + class Container extends Component { + componentWillUnmount() { + this.props.dispatch({ type: 'APPEND', body: 'a' }) + } + render() { + return + } + } + + const div = document.createElement('div') ReactDOM.render( , div ) - }) + expect(mapStateToPropsCalls).toBe(1) - expect(mapStateToPropsCalls).toBe(1) - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + ReactDOM.unmountComponentAtNode(div) + expect(spy).toHaveBeenCalledTimes(0) + expect(mapStateToPropsCalls).toBe(1) + spy.mockRestore() }) - expect(spy).toHaveBeenCalledTimes(0) - expect(mapStateToPropsCalls).toBe(1) - spy.mockRestore() - }) - - it('should not attempt to notify unmounted child of state change', () => { - const store = createStore(stringBuilder) + it('should not attempt to set state after unmounting', () => { + const store = createStore(stringBuilder) + let mapStateToPropsCalls = 0 - @connect(state => ({ hide: state === 'AB' })) - class App extends Component { - render() { - return this.props.hide ? null : + @connect( + () => ({ calls: ++mapStateToPropsCalls }), + dispatch => ({ dispatch }) + ) + class Container extends Component { + render() { + return + } } - } - @connect(() => ({})) - class Container extends Component { - render() { - return + const div = document.createElement('div') + store.subscribe(() => { + ReactDOM.unmountComponentAtNode(div) + }) + + rtl.act(() => { + ReactDOM.render( + + + , + div + ) + }) + + expect(mapStateToPropsCalls).toBe(1) + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + + expect(spy).toHaveBeenCalledTimes(0) + expect(mapStateToPropsCalls).toBe(1) + spy.mockRestore() + }) + + 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(state => ({ state })) - class Child extends Component { - componentDidMount() { - if (this.props.state === 'A') { - store.dispatch({ type: 'APPEND', body: 'B' }) + @connect(null) + class Parent extends React.Component { + componentWillUnmount() { + this.props.dispatch({ type: 'clean' }) + } + + render() { + return } } - render() { - return null + + function mapState(state) { + return { + profile: state.data.profile + } } - } - const div = document.createElement('div') - ReactDOM.render( - - - , - div - ) + @connect(mapState) + class Child extends React.Component { + render() { + return null + } + } - try { + const store = createStore(reducer) rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'A' }) + store.dispatch({ type: 'fetch' }) }) - } finally { + + const div = document.createElement('div') + ReactDOM.render( + + + , + div + ) + ReactDOM.unmountComponentAtNode(div) - } + }) }) - it('should not attempt to set state after unmounting nested components', () => { - const store = createStore(() => ({})) - let mapStateToPropsCalls = 0 - - let linkA, linkB + describe('Performance optimizations and bail-outs', () => { + it('should shallowly compare the selected state to prevent unnecessary updates', () => { + const store = createStore(stringBuilder) + const spy = jest.fn(() => ({})) + function render({ string }) { + spy() + return + } - let App = ({ children, setLocation }) => { - const onClick = to => event => { - event.preventDefault() - setLocation(to) + @connect( + state => ({ string: state }), + dispatch => ({ dispatch }) + ) + class Container extends Component { + render() { + return render(this.props) + } } - /* eslint-disable react/jsx-no-bind */ - return ( - - ) - /* eslint-enable react/jsx-no-bind */ - } - App = connect(() => ({}))(App) - let A = () =>

A

- function mapState(state) { - const calls = ++mapStateToPropsCalls - return { calls, state } - } - A = connect(mapState)(A) + const tester = rtl.render( + + + + ) + expect(spy).toHaveBeenCalledTimes(1) + expect(tester.getByTestId('string')).toHaveTextContent('') + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + + expect(spy).toHaveBeenCalledTimes(2) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'b' }) + }) - const B = () =>

B

+ expect(spy).toHaveBeenCalledTimes(3) + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: '' }) + }) - class RouterMock extends React.Component { - constructor(...args) { - super(...args) - this.state = { location: { pathname: 'a' } } - this.setLocation = this.setLocation.bind(this) - } + expect(spy).toHaveBeenCalledTimes(3) + }) - setLocation(pathname) { - this.setState({ location: { pathname } }) - store.dispatch({ type: 'TEST' }) + it('should shallowly compare the merged state to prevent unnecessary updates', () => { + const store = createStore(stringBuilder) + const spy = jest.fn(() => ({})) + const tree = {} + function render({ string, pass }) { + spy() + return } - getChildComponent(location) { - switch (location) { - case 'a': - return - case 'b': - return - default: - throw new Error('Unknown location: ' + location) + @connect( + state => ({ string: state }), + dispatch => ({ dispatch }), + (stateProps, dispatchProps, parentProps) => ({ + ...dispatchProps, + ...stateProps, + ...parentProps + }) + ) + class Container extends Component { + render() { + return render(this.props) } } - render() { - return ( - - {this.getChildComponent(this.state.location.pathname)} - - ) + class Root extends Component { + constructor(props) { + super(props) + this.state = { pass: '' } + tree.setState = this.setState.bind(this) + } + + render() { + return ( + + + + ) + } } - } - const div = document.createElement('div') - document.body.appendChild(div) - ReactDOM.render( - - - , - div - ) - - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - - linkA.click() - linkB.click() - linkB.click() - - document.body.removeChild(div) - // Called 3 times: - // - Initial mount - // - After first link click, stil mounted - // - After second link click, but the queued state update is discarded due to batching as it's unmounted - expect(mapStateToPropsCalls).toBe(3) - expect(spy).toHaveBeenCalledTimes(0) - spy.mockRestore() - }) + const tester = rtl.render() + expect(spy).toHaveBeenCalledTimes(1) + expect(tester.getByTestId('string')).toHaveTextContent('') + expect(tester.getByTestId('pass')).toHaveTextContent('') - it('should not attempt to set state when dispatching in componentWillUnmount', () => { - const store = createStore(stringBuilder) - let mapStateToPropsCalls = 0 + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) - /*eslint-disable no-unused-vars */ - @connect( - state => ({ calls: mapStateToPropsCalls++ }), - dispatch => ({ dispatch }) - ) - /*eslint-enable no-unused-vars */ - class Container extends Component { - componentWillUnmount() { - this.props.dispatch({ type: 'APPEND', body: 'a' }) - } - render() { - return - } - } + expect(spy).toHaveBeenCalledTimes(2) + expect(tester.getByTestId('string')).toHaveTextContent('a') + expect(tester.getByTestId('pass')).toHaveTextContent('') - const div = document.createElement('div') - ReactDOM.render( - - - , - div - ) - expect(mapStateToPropsCalls).toBe(1) - - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - ReactDOM.unmountComponentAtNode(div) - expect(spy).toHaveBeenCalledTimes(0) - expect(mapStateToPropsCalls).toBe(1) - spy.mockRestore() - }) + rtl.act(() => { + tree.setState({ pass: '' }) + }) - it('should shallowly compare the selected state to prevent unnecessary updates', () => { - const store = createStore(stringBuilder) - const spy = jest.fn(() => ({})) - function render({ string }) { - spy() - return - } + expect(spy).toHaveBeenCalledTimes(2) + expect(tester.getByTestId('string')).toHaveTextContent('a') + expect(tester.getByTestId('pass')).toHaveTextContent('') - @connect( - state => ({ string: state }), - dispatch => ({ dispatch }) - ) - class Container extends Component { - render() { - return render(this.props) - } - } + rtl.act(() => { + tree.setState({ pass: 'through' }) + }) - const tester = rtl.render( - - - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(tester.getByTestId('string')).toHaveTextContent('') - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) - }) + expect(spy).toHaveBeenCalledTimes(3) + expect(tester.getByTestId('string')).toHaveTextContent('a') + expect(tester.getByTestId('pass')).toHaveTextContent('through') - expect(spy).toHaveBeenCalledTimes(2) - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'b' }) - }) + rtl.act(() => { + tree.setState({ pass: 'through' }) + }) - expect(spy).toHaveBeenCalledTimes(3) - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: '' }) + expect(spy).toHaveBeenCalledTimes(3) + expect(tester.getByTestId('string')).toHaveTextContent('a') + expect(tester.getByTestId('pass')).toHaveTextContent('through') + + const obj = { prop: 'val' } + rtl.act(() => { + tree.setState({ pass: obj }) + }) + + expect(spy).toHaveBeenCalledTimes(4) + expect(tester.getByTestId('string')).toHaveTextContent('a') + expect(tester.getByTestId('pass')).toHaveTextContent('{"prop":"val"}') + + rtl.act(() => { + tree.setState({ pass: obj }) + }) + + expect(spy).toHaveBeenCalledTimes(4) + expect(tester.getByTestId('string')).toHaveTextContent('a') + expect(tester.getByTestId('pass')).toHaveTextContent('{"prop":"val"}') + + const obj2 = Object.assign({}, obj, { val: 'otherval' }) + rtl.act(() => { + tree.setState({ pass: obj2 }) + }) + + expect(spy).toHaveBeenCalledTimes(5) + expect(tester.getByTestId('string')).toHaveTextContent('a') + expect(tester.getByTestId('pass')).toHaveTextContent( + '{"prop":"val","val":"otherval"}' + ) + + obj2.val = 'mutation' + rtl.act(() => { + tree.setState({ pass: obj2 }) + }) + + expect(spy).toHaveBeenCalledTimes(5) + expect(tester.getByTestId('string')).toHaveTextContent('a') + expect(tester.getByTestId('pass')).toHaveTextContent( + '{"prop":"val","val":"otherval"}' + ) }) - expect(spy).toHaveBeenCalledTimes(3) - }) + it('should not render the wrapped component when mapState does not produce change', () => { + const store = createStore(stringBuilder) + let renderCalls = 0 + let mapStateCalls = 0 + + @connect(() => { + mapStateCalls++ + return {} // no change! + }) + class Container extends Component { + render() { + renderCalls++ + return + } + } + + rtl.render( + + + + ) + + expect(renderCalls).toBe(1) + expect(mapStateCalls).toBe(1) + + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) + + // After store a change mapState has been called + expect(mapStateCalls).toBe(2) + // But render is not because it did not make any actual changes + expect(renderCalls).toBe(1) + }) - it('should shallowly compare the merged state to prevent unnecessary updates', () => { - const store = createStore(stringBuilder) - const spy = jest.fn(() => ({})) - const tree = {} - function render({ string, pass }) { - spy() - return - } + it('should bail out early if mapState does not depend on props', () => { + const store = createStore(stringBuilder) + let renderCalls = 0 + let mapStateCalls = 0 - @connect( - state => ({ string: state }), - dispatch => ({ dispatch }), - (stateProps, dispatchProps, parentProps) => ({ - ...dispatchProps, - ...stateProps, - ...parentProps + @connect(state => { + mapStateCalls++ + return state === 'aaa' ? { change: 1 } : {} }) - ) - class Container extends Component { - render() { - return render(this.props) + class Container extends Component { + render() { + renderCalls++ + return + } } - } - class Root extends Component { - constructor(props) { - super(props) - this.state = { pass: '' } - tree.setState = this.setState.bind(this) - } + rtl.render( + + + + ) - render() { - return ( - - - - ) - } - } + expect(renderCalls).toBe(1) + expect(mapStateCalls).toBe(1) - const tester = rtl.render() - expect(spy).toHaveBeenCalledTimes(1) - expect(tester.getByTestId('string')).toHaveTextContent('') - expect(tester.getByTestId('pass')).toHaveTextContent('') + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) - }) + expect(mapStateCalls).toBe(2) + expect(renderCalls).toBe(1) - expect(spy).toHaveBeenCalledTimes(2) - expect(tester.getByTestId('string')).toHaveTextContent('a') - expect(tester.getByTestId('pass')).toHaveTextContent('') + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) - rtl.act(() => { - tree.setState({ pass: '' }) - }) + expect(mapStateCalls).toBe(3) + expect(renderCalls).toBe(1) - expect(spy).toHaveBeenCalledTimes(2) - expect(tester.getByTestId('string')).toHaveTextContent('a') - expect(tester.getByTestId('pass')).toHaveTextContent('') + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) - rtl.act(() => { - tree.setState({ pass: 'through' }) + expect(mapStateCalls).toBe(4) + expect(renderCalls).toBe(2) }) - expect(spy).toHaveBeenCalledTimes(3) - expect(tester.getByTestId('string')).toHaveTextContent('a') - expect(tester.getByTestId('pass')).toHaveTextContent('through') + it('should not call update if mergeProps return value has not changed', () => { + let mapStateCalls = 0 + let renderCalls = 0 + const store = createStore(stringBuilder) - rtl.act(() => { - tree.setState({ pass: 'through' }) - }) + @connect( + () => ({ a: ++mapStateCalls }), + null, + () => ({ changed: false }) + ) + class Container extends Component { + render() { + renderCalls++ + return + } + } - expect(spy).toHaveBeenCalledTimes(3) - expect(tester.getByTestId('string')).toHaveTextContent('a') - expect(tester.getByTestId('pass')).toHaveTextContent('through') + rtl.render( + + + + ) - const obj = { prop: 'val' } - rtl.act(() => { - tree.setState({ pass: obj }) - }) + expect(renderCalls).toBe(1) + expect(mapStateCalls).toBe(1) - expect(spy).toHaveBeenCalledTimes(4) - expect(tester.getByTestId('string')).toHaveTextContent('a') - expect(tester.getByTestId('pass')).toHaveTextContent('{"prop":"val"}') + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) - rtl.act(() => { - tree.setState({ pass: obj }) + expect(mapStateCalls).toBe(2) + expect(renderCalls).toBe(1) }) - expect(spy).toHaveBeenCalledTimes(4) - expect(tester.getByTestId('string')).toHaveTextContent('a') - expect(tester.getByTestId('pass')).toHaveTextContent('{"prop":"val"}') + it('should not swallow errors when bailing out early', () => { + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + 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 + } + } - const obj2 = Object.assign({}, obj, { val: 'otherval' }) - rtl.act(() => { - tree.setState({ pass: obj2 }) - }) + rtl.render( + + + + ) - expect(spy).toHaveBeenCalledTimes(5) - expect(tester.getByTestId('string')).toHaveTextContent('a') - expect(tester.getByTestId('pass')).toHaveTextContent( - '{"prop":"val","val":"otherval"}' - ) + expect(renderCalls).toBe(1) + expect(mapStateCalls).toBe(1) + expect(() => store.dispatch({ type: 'APPEND', body: 'a' })).toThrow() - obj2.val = 'mutation' - rtl.act(() => { - tree.setState({ pass: obj2 }) + spy.mockRestore() }) - - expect(spy).toHaveBeenCalledTimes(5) - expect(tester.getByTestId('string')).toHaveTextContent('a') - expect(tester.getByTestId('pass')).toHaveTextContent( - '{"prop":"val","val":"otherval"}' - ) - }) - - it('should throw an error if a component is not passed to the function returned by connect', () => { - expect(connect()).toThrow(/You must pass a component to the function/) }) - it('should throw an error if mapState, mapDispatch, or mergeProps returns anything but a plain object', () => { - const store = createStore(() => ({})) - - function makeContainer(mapState, mapDispatch, mergeProps) { + describe('HMR handling', () => { + it.skip('should recalculate the state and rebind the actions on hot update', () => { + const store = createStore(() => {}) @connect( - mapState, - mapDispatch, - mergeProps + null, + () => ({ scooby: 'doo' }) ) - class Container extends Component { + class ContainerBefore extends Component { render() { - return + return } } - return React.createElement(Container) - } - - function AwesomeMap() {} - - let spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.render( - - {makeContainer(() => 1, () => ({}), () => ({}))} - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(spy.mock.calls[0][0]).toMatch( - /mapStateToProps\(\) in Connect\(Container\) must return a plain object/ - ) - spy.mockRestore() - rtl.cleanup() - - spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.render( - - {makeContainer(() => 'hey', () => ({}), () => ({}))} - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(spy.mock.calls[0][0]).toMatch( - /mapStateToProps\(\) in Connect\(Container\) must return a plain object/ - ) - spy.mockRestore() - rtl.cleanup() - - spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.render( - - {makeContainer(() => new AwesomeMap(), () => ({}), () => ({}))} - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(spy.mock.calls[0][0]).toMatch( - /mapStateToProps\(\) in Connect\(Container\) must return a plain object/ - ) - spy.mockRestore() - rtl.cleanup() - - spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.render( - - {makeContainer(() => ({}), () => 1, () => ({}))} - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(spy.mock.calls[0][0]).toMatch( - /mapDispatchToProps\(\) in Connect\(Container\) must return a plain object/ - ) - spy.mockRestore() - rtl.cleanup() - - spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.render( - - {makeContainer(() => ({}), () => 'hey', () => ({}))} - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(spy.mock.calls[0][0]).toMatch( - /mapDispatchToProps\(\) in Connect\(Container\) must return a plain object/ - ) - spy.mockRestore() - rtl.cleanup() - - spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.render( - - {makeContainer(() => ({}), () => new AwesomeMap(), () => ({}))} - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(spy.mock.calls[0][0]).toMatch( - /mapDispatchToProps\(\) in Connect\(Container\) must return a plain object/ - ) - spy.mockRestore() - rtl.cleanup() - - spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.render( - - {makeContainer(() => ({}), () => ({}), () => 1)} - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(spy.mock.calls[0][0]).toMatch( - /mergeProps\(\) in Connect\(Container\) must return a plain object/ - ) - spy.mockRestore() - rtl.cleanup() - - spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.render( - - {makeContainer(() => ({}), () => ({}), () => 'hey')} - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(spy.mock.calls[0][0]).toMatch( - /mergeProps\(\) in Connect\(Container\) must return a plain object/ - ) - spy.mockRestore() - rtl.cleanup() - - spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - rtl.render( - - {makeContainer(() => ({}), () => ({}), () => new AwesomeMap())} - - ) - expect(spy).toHaveBeenCalledTimes(1) - expect(spy.mock.calls[0][0]).toMatch( - /mergeProps\(\) in Connect\(Container\) must return a plain object/ - ) - spy.mockRestore() - }) - it.skip('should recalculate the state and rebind the actions on hot update', () => { - const store = createStore(() => {}) - @connect( - null, - () => ({ scooby: 'doo' }) - ) - class ContainerBefore extends Component { - render() { - return - } - } - @connect( - () => ({ foo: 'baz' }), - () => ({ scooby: 'foo' }) - ) - class ContainerAfter extends Component { - render() { - return + @connect( + () => ({ foo: 'baz' }), + () => ({ scooby: 'foo' }) + ) + class ContainerAfter extends Component { + render() { + return + } } - } - @connect( - () => ({ foo: 'bar' }), - () => ({ scooby: 'boo' }) - ) - class ContainerNext extends Component { - render() { - return + @connect( + () => ({ foo: 'bar' }), + () => ({ scooby: 'boo' }) + ) + class ContainerNext extends Component { + render() { + return + } } - } - let container - const tester = rtl.render( - - (container = instance)} /> - - ) - expect(tester.queryByTestId('foo')).toBe(null) - expect(tester.getByTestId('scooby')).toHaveTextContent('doo') - imitateHotReloading(ContainerBefore, ContainerAfter, container) - expect(tester.getByTestId('foo')).toHaveTextContent('baz') - expect(tester.getByTestId('scooby')).toHaveTextContent('foo') - imitateHotReloading(ContainerBefore, ContainerNext, container) - expect(tester.getByTestId('foo')).toHaveTextContent('bar') - expect(tester.getByTestId('scooby')).toHaveTextContent('boo') - }) + let container + const tester = rtl.render( + + (container = instance)} /> + + ) + expect(tester.queryByTestId('foo')).toBe(null) + expect(tester.getByTestId('scooby')).toHaveTextContent('doo') + imitateHotReloading(ContainerBefore, ContainerAfter, container) + expect(tester.getByTestId('foo')).toHaveTextContent('baz') + expect(tester.getByTestId('scooby')).toHaveTextContent('foo') + imitateHotReloading(ContainerBefore, ContainerNext, container) + expect(tester.getByTestId('foo')).toHaveTextContent('bar') + expect(tester.getByTestId('scooby')).toHaveTextContent('boo') + }) - it.skip('should persist listeners through hot update', () => { - const ACTION_TYPE = 'ACTION' - const store = createStore((state = { actions: 0 }, action) => { - switch (action.type) { - case ACTION_TYPE: { - return { - actions: state.actions + 1 + it.skip('should persist listeners through hot update', () => { + const ACTION_TYPE = 'ACTION' + const store = createStore((state = { actions: 0 }, action) => { + switch (action.type) { + case ACTION_TYPE: { + return { + actions: state.actions + 1 + } } + default: + return state } - default: - return state - } - }) + }) - @connect(state => ({ actions: state.actions })) - class Child extends Component { - render() { - return + @connect(state => ({ actions: state.actions })) + class Child extends Component { + render() { + return + } } - } - @connect(() => ({ scooby: 'doo' })) - class ParentBefore extends Component { - render() { - return + @connect(() => ({ scooby: 'doo' })) + class ParentBefore extends Component { + render() { + return + } } - } - @connect(() => ({ scooby: 'boo' })) - class ParentAfter extends Component { - render() { - return + @connect(() => ({ scooby: 'boo' })) + class ParentAfter extends Component { + render() { + return + } } - } - let container - const tester = rtl.render( - - (container = instance)} /> - - ) + let container + const tester = rtl.render( + + (container = instance)} /> + + ) - imitateHotReloading(ParentBefore, ParentAfter, container) + imitateHotReloading(ParentBefore, ParentAfter, container) - rtl.act(() => { - store.dispatch({ type: ACTION_TYPE }) - }) + rtl.act(() => { + store.dispatch({ type: ACTION_TYPE }) + }) - expect(tester.getByTestId('actions')).toHaveTextContent('1') + expect(tester.getByTestId('actions')).toHaveTextContent('1') + }) }) - it('should set the displayName correctly', () => { - expect( - connect(state => state)( - class Foo extends Component { - render() { - return
- } - } - ).displayName - ).toBe('Connect(Foo)') + describe('Wrapped component and HOC handling', () => { + it('should throw an error if a component is not passed to the function returned by connect', () => { + expect(connect()).toThrow(/You must pass a component to the function/) + }) - expect( - connect(state => state)( - createClass({ - displayName: 'Bar', - render() { - return
- } + it('should not error on valid component with circular structure', () => { + const createComp = Tag => { + const Comp = React.forwardRef(function Comp(props) { + return {props.count} }) - ).displayName - ).toBe('Connect(Bar)') - - expect( - connect(state => state)( - // eslint: In this case, we don't want to specify a displayName because we're testing what - // happens when one isn't defined. - /* eslint-disable react/display-name */ - createClass({ - render() { - return
- } - }) - /* eslint-enable react/display-name */ - ).displayName - ).toBe('Connect(Component)') - }) - - it('should expose the wrapped component as WrappedComponent', () => { - class Container extends Component { - render() { - return + Comp.__real = Comp + return Comp } - } - const decorator = connect(state => state) - const decorated = decorator(Container) + expect(() => { + connect()(createComp('div')) + }).not.toThrow() + }) - expect(decorated.WrappedComponent).toBe(Container) - }) + it('Should work with a memo component, if it exists', () => { + if (React.memo) { + const store = createStore(() => ({ hi: 'there' })) - it('should hoist non-react statics from wrapped component', () => { - class Container extends Component { - render() { - return - } - } + const Container = React.memo(props => ) // eslint-disable-line + Container.displayName = 'Container' + const WrappedContainer = connect(state => state)(Container) - Container.howIsRedux = () => 'Awesome!' - Container.foo = 'bar' + const tester = rtl.render( + + + + ) - const decorator = connect(state => state) - const decorated = decorator(Container) + expect(tester.getByTestId('hi')).toHaveTextContent('there') + } + }) - expect(decorated.howIsRedux).toBeInstanceOf(Function) - expect(decorated.howIsRedux()).toBe('Awesome!') - expect(decorated.foo).toBe('bar') - }) + it('should set the displayName correctly', () => { + expect( + connect(state => state)( + class Foo extends Component { + render() { + return
+ } + } + ).displayName + ).toBe('Connect(Foo)') + + expect( + connect(state => state)( + createClass({ + displayName: 'Bar', + render() { + return
+ } + }) + ).displayName + ).toBe('Connect(Bar)') + + expect( + connect(state => state)( + // eslint: In this case, we don't want to specify a displayName because we're testing what + // happens when one isn't defined. + /* eslint-disable react/display-name */ + createClass({ + render() { + return
+ } + }) + /* eslint-enable react/display-name */ + ).displayName + ).toBe('Connect(Component)') + }) - it('should use a custom context provider and consumer if given as an option to connect', () => { - class Container extends Component { - render() { - return + it('should allow custom displayName', () => { + @connect( + null, + null, + null, + { getDisplayName: name => `Custom(${name})` } + ) + class MyComponent extends React.Component { + render() { + return
+ } } - } - - const context = React.createContext(null) - let actualState + expect(MyComponent.displayName).toEqual('Custom(MyComponent)') + }) - const expectedState = { foos: {} } - const ignoredState = { bars: {} } + it('should expose the wrapped component as WrappedComponent', () => { + class Container extends Component { + render() { + return + } + } - const decorator = connect( - state => { - actualState = state - return {} - }, - undefined, - undefined, - { context } - ) - const Decorated = decorator(Container) - - const store1 = createStore(() => expectedState) - const store2 = createStore(() => ignoredState) - - rtl.render( - - - - - - ) + const decorator = connect(state => state) + const decorated = decorator(Container) - expect(actualState).toEqual(expectedState) - }) + expect(decorated.WrappedComponent).toBe(Container) + }) - it('should use the store from the props instead of from the context if present', () => { - class Container extends Component { - render() { - return + it('should hoist non-react statics from wrapped component', () => { + class Container extends Component { + render() { + return + } } - } - let actualState + Container.howIsRedux = () => 'Awesome!' + Container.foo = 'bar' - const expectedState = { foos: {} } - const decorator = connect(state => { - actualState = state - return {} - }) - const Decorated = decorator(Container) - const mockStore = { - dispatch: () => {}, - subscribe: () => {}, - getState: () => expectedState - } + const decorator = connect(state => state) + const decorated = decorator(Container) - rtl.render() - - expect(actualState).toEqual(expectedState) + expect(decorated.howIsRedux).toBeInstanceOf(Function) + expect(decorated.howIsRedux()).toBe('Awesome!') + expect(decorated.foo).toBe('bar') + }) }) - it('should pass through ancestor subscription when store is given as a prop', () => { - const c3Spy = jest.fn() - const c2Spy = jest.fn() - const c1Spy = jest.fn() + describe('Store subscriptions and nesting', () => { + it('should pass dispatch and avoid subscription if arguments are falsy', () => { + const store = createStore(() => ({ + foo: 'bar' + })) - const Comp3 = ({ first }) => { - c3Spy() - return - } - const ConnectedComp3 = connect(state => state)(Comp3) + function runCheck(...connectArgs) { + @connect(...connectArgs) + class Container extends Component { + render() { + return + } + } - const Comp2 = ({ second }) => { - c2Spy() - return ( -
- - -
- ) - } - const ConnectedComp2 = connect(state => state)(Comp2) + const tester = rtl.render( + + + + ) + expect(tester.getByTestId('dispatch')).toHaveTextContent( + '[function dispatch]' + ) + expect(tester.queryByTestId('foo')).toBe(null) + expect(tester.getByTestId('pass')).toHaveTextContent('through') + } - const Comp1 = ({ children, first }) => { - c1Spy() - return ( -
- - {children} -
- ) - } - const ConnectedComp1 = connect(state => state)(Comp1) + runCheck() + runCheck(null, null, null) + runCheck(false, false, false) + }) - const reducer1 = (state = { first: '1' }, action) => { - switch (action.type) { - case 'CHANGE': - return { first: '2' } - default: - return state + it('should subscribe properly when a middle connected component does not subscribe', () => { + @connect(state => ({ count: state })) + class A extends React.Component { + render() { + return + } } - } - const reducer2 = (state = { second: '3' }, action) => { - switch (action.type) { - case 'CHANGE': - return { second: '4' } - default: - return state + @connect() // no mapStateToProps. therefore it should be transparent for subscriptions + class B extends React.Component { + render() { + return + } } - } - - const store1 = createStore(reducer1) - const store2 = createStore(reducer2) - - const tester = rtl.render( - - - - - - ) - - // Initial render: C1/C3 read from store 1, C2 reads from store 2, one render each - expect(tester.getByTestId('a')).toHaveTextContent('1') - expect(tester.getByTestId('b')).toHaveTextContent('3') - expect(tester.getByTestId('c')).toHaveTextContent('1') - - expect(c3Spy).toHaveBeenCalledTimes(1) - expect(c2Spy).toHaveBeenCalledTimes(1) - expect(c1Spy).toHaveBeenCalledTimes(1) - - rtl.act(() => { - store1.dispatch({ type: 'CHANGE' }) - }) - // Store 1 update: C1 and C3 should re-render, no updates for C2 - expect(tester.getByTestId('a')).toHaveTextContent('2') - expect(tester.getByTestId('b')).toHaveTextContent('3') - expect(tester.getByTestId('c')).toHaveTextContent('2') + @connect((state, props) => { + expect(props.count).toBe(state) + return { count: state * 10 + props.count } + }) + class C extends React.Component { + render() { + return
{this.props.count}
+ } + } - expect(c3Spy).toHaveBeenCalledTimes(2) - expect(c2Spy).toHaveBeenCalledTimes(1) - expect(c1Spy).toHaveBeenCalledTimes(2) + const store = createStore((state = 0, action) => + action.type === 'INC' ? (state += 1) : state + ) + rtl.render( + + + + ) - rtl.act(() => { - store2.dispatch({ type: 'CHANGE' }) + rtl.act(() => { + store.dispatch({ type: 'INC' }) + }) }) - // Store 2 update: C2 should re-render, no updates for C1 or C3 - expect(tester.getByTestId('a')).toHaveTextContent('2') - expect(tester.getByTestId('b')).toHaveTextContent('4') - expect(tester.getByTestId('c')).toHaveTextContent('2') - - expect(c3Spy).toHaveBeenCalledTimes(2) - expect(c2Spy).toHaveBeenCalledTimes(2) - expect(c1Spy).toHaveBeenCalledTimes(2) - }) + it('should notify nested components through a blocking component', () => { + @connect(state => ({ count: state })) + class Parent extends Component { + render() { + return ( + + + + ) + } + } - it('should use a custom context provider and consumer if passed as a prop to the component', () => { - class Container extends Component { - render() { - return + class BlockUpdates extends Component { + shouldComponentUpdate() { + return false + } + render() { + return this.props.children + } } - } - const context = React.createContext(null) + const mapStateToProps = jest.fn(state => ({ count: state })) + @connect(mapStateToProps) + class Child extends Component { + render() { + return
{this.props.count}
+ } + } - let actualState + const store = createStore((state = 0, action) => + action.type === 'INC' ? state + 1 : state + ) + rtl.render( + + + + ) - const expectedState = { foos: {} } - const ignoredState = { bars: {} } + expect(mapStateToProps).toHaveBeenCalledTimes(1) + rtl.act(() => { + store.dispatch({ type: 'INC' }) + }) - const decorator = connect(state => { - actualState = state - return {} + expect(mapStateToProps).toHaveBeenCalledTimes(2) }) - const Decorated = decorator(Container) - - const store1 = createStore(() => expectedState) - const store2 = createStore(() => ignoredState) - - rtl.render( - - - - - - ) - - expect(actualState).toEqual(expectedState) }) - it('should ignore non-react-context values that are passed as a prop to the component', () => { - class Container extends Component { - render() { - return + describe('Custom context and store-as-prop', () => { + it('should use a custom context provider and consumer if given as an option to connect', () => { + class Container extends Component { + render() { + return + } } - } - const nonContext = { someProperty: {} } + const context = React.createContext(null) - let actualState + let actualState - const expectedState = { foos: {} } + const expectedState = { foos: {} } + const ignoredState = { bars: {} } - const decorator = connect(state => { - actualState = state - return {} - }) - const Decorated = decorator(Container) - - const store = createStore(() => expectedState) + const decorator = connect( + state => { + actualState = state + return {} + }, + undefined, + undefined, + { context } + ) + const Decorated = decorator(Container) - rtl.render( - - - - ) + const store1 = createStore(() => expectedState) + const store2 = createStore(() => ignoredState) - expect(actualState).toEqual(expectedState) - }) + rtl.render( + + + + + + ) - it('should throw an error if the store is not in the props or context', () => { - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + expect(actualState).toEqual(expectedState) + }) - class Container extends Component { - render() { - return + it('should use a custom context provider and consumer if passed as a prop to the component', () => { + class Container extends Component { + render() { + return + } } - } - const decorator = connect(() => {}) - const Decorated = decorator(Container) + const context = React.createContext(null) - expect(() => rtl.render()).toThrow(/Could not find "store"/) + let actualState - spy.mockRestore() - }) - - it.skip('should throw when trying to access the wrapped instance if withRef is not specified', () => { - const store = createStore(() => ({})) - - class Container extends Component { - render() { - return - } - } + const expectedState = { foos: {} } + const ignoredState = { bars: {} } - const decorator = connect(state => state) - const Decorated = decorator(Container) + const decorator = connect(state => { + actualState = state + return {} + }) + const Decorated = decorator(Container) - class Wrapper extends Component { - render() { - return comp && comp.getWrappedInstance()} /> - } - } + const store1 = createStore(() => expectedState) + const store2 = createStore(() => ignoredState) - // TODO Remove this when React is fixed, per https://github.com/facebook/react/issues/11098 - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - expect(() => rtl.render( - - + + + + ) - ).toThrow( - `To access the wrapped instance, you need to specify { withRef: true } in the options argument of the connect() call` - ) - spy.mockRestore() - }) - - it('should return the instance of the wrapped component for use in calling child methods', async done => { - const store = createStore(() => ({})) - - const someData = { - some: 'data' - } - class Container extends Component { - someInstanceMethod() { - return someData - } + expect(actualState).toEqual(expectedState) + }) - render() { - return + it('should ignore non-react-context values that are passed as a prop to the component', () => { + class Container extends Component { + render() { + return + } } - } - const decorator = connect( - state => state, - null, - null, - { forwardRef: true } - ) - const Decorated = decorator(Container) + const nonContext = { someProperty: {} } - const ref = React.createRef() + let actualState - class Wrapper extends Component { - render() { - return - } - } + const expectedState = { foos: {} } - const tester = rtl.render( - - - - ) + const decorator = connect(state => { + actualState = state + return {} + }) + const Decorated = decorator(Container) - await rtl.waitForElement(() => tester.getByTestId('loaded')) + const store = createStore(() => expectedState) - expect(ref.current.someInstanceMethod()).toBe(someData) - done() - }) + rtl.render( + + + + ) - it('should correctly separate and pass through props to the wrapped component with a forwarded ref', async done => { - const store = createStore(() => ({})) + expect(actualState).toEqual(expectedState) + }) - class Container extends Component { - render() { - return + it('should use the store from the props instead of from the context if present', () => { + class Container extends Component { + render() { + return + } } - } - const decorator = connect( - state => state, - null, - null, - { forwardRef: true } - ) - const Decorated = decorator(Container) + let actualState - const ref = React.createRef() - - class Wrapper extends Component { - render() { - // The 'a' prop should eventually be passed to the wrapped component individually, - // not sent through as `wrapperProps={ {a : 42} }` - return + const expectedState = { foos: {} } + const decorator = connect(state => { + actualState = state + return {} + }) + const Decorated = decorator(Container) + const mockStore = { + dispatch: () => {}, + subscribe: () => {}, + getState: () => expectedState } - } - const tester = rtl.render( - - - - ) + rtl.render() - tester.debug() - await rtl.waitForElement(() => tester.getByTestId('a')) - expect(tester.getByTestId('a')).toHaveTextContent('42') + expect(actualState).toEqual(expectedState) + }) - done() - }) + it('should pass through ancestor subscription when store is given as a prop', () => { + const c3Spy = jest.fn() + const c2Spy = jest.fn() + const c1Spy = jest.fn() - it('should return the instance of the wrapped component for use in calling child methods, impure component', async done => { - const store = createStore(() => ({})) + const Comp3 = ({ first }) => { + c3Spy() + return + } + const ConnectedComp3 = connect(state => state)(Comp3) - const someData = { - some: 'data' - } + const Comp2 = ({ second }) => { + c2Spy() + return ( +
+ + +
+ ) + } + const ConnectedComp2 = connect(state => state)(Comp2) - class Container extends Component { - someInstanceMethod() { - return someData + const Comp1 = ({ children, first }) => { + c1Spy() + return ( +
+ + {children} +
+ ) } + const ConnectedComp1 = connect(state => state)(Comp1) - render() { - return + const reducer1 = (state = { first: '1' }, action) => { + switch (action.type) { + case 'CHANGE': + return { first: '2' } + default: + return state + } } - } - const decorator = connect( - state => state, - undefined, - undefined, - { forwardRef: true, pure: false } - ) - const Decorated = decorator(Container) + const reducer2 = (state = { second: '3' }, action) => { + switch (action.type) { + case 'CHANGE': + return { second: '4' } + default: + return state + } + } - const ref = React.createRef() + const store1 = createStore(reducer1) + const store2 = createStore(reducer2) - class Wrapper extends Component { - render() { - return - } - } + const tester = rtl.render( + + + + + + ) - const tester = rtl.render( - - - - ) + // Initial render: C1/C3 read from store 1, C2 reads from store 2, one render each + expect(tester.getByTestId('a')).toHaveTextContent('1') + expect(tester.getByTestId('b')).toHaveTextContent('3') + expect(tester.getByTestId('c')).toHaveTextContent('1') - await rtl.waitForElement(() => tester.getByTestId('loaded')) + expect(c3Spy).toHaveBeenCalledTimes(1) + expect(c2Spy).toHaveBeenCalledTimes(1) + expect(c1Spy).toHaveBeenCalledTimes(1) - expect(ref.current.someInstanceMethod()).toBe(someData) - done() - }) + rtl.act(() => { + store1.dispatch({ type: 'CHANGE' }) + }) - it('should wrap impure components without supressing updates', () => { - const store = createStore(() => ({})) + // Store 1 update: C1 and C3 should re-render, no updates for C2 + expect(tester.getByTestId('a')).toHaveTextContent('2') + expect(tester.getByTestId('b')).toHaveTextContent('3') + expect(tester.getByTestId('c')).toHaveTextContent('2') - class ImpureComponent extends Component { - render() { - return - } - } + expect(c3Spy).toHaveBeenCalledTimes(2) + expect(c2Spy).toHaveBeenCalledTimes(1) + expect(c1Spy).toHaveBeenCalledTimes(2) - ImpureComponent.contextTypes = { - statefulValue: PropTypes.number - } + rtl.act(() => { + store2.dispatch({ type: 'CHANGE' }) + }) + + // Store 2 update: C2 should re-render, no updates for C1 or C3 + expect(tester.getByTestId('a')).toHaveTextContent('2') + expect(tester.getByTestId('b')).toHaveTextContent('4') + expect(tester.getByTestId('c')).toHaveTextContent('2') + + expect(c3Spy).toHaveBeenCalledTimes(2) + expect(c2Spy).toHaveBeenCalledTimes(2) + expect(c1Spy).toHaveBeenCalledTimes(2) + }) - const decorator = connect( - state => state, - null, - null, - { pure: false } - ) - const Decorated = decorator(ImpureComponent) + it('should subscribe properly when a new store is provided via props', () => { + const store1 = createStore((state = 0, action) => + action.type === 'INC' ? state + 1 : state + ) + const store2 = createStore((state = 0, action) => + action.type === 'INC' ? state + 1 : state + ) + const customContext = React.createContext() - let externalSetState - class StatefulWrapper extends Component { - constructor() { - super() - this.state = { value: 0 } - externalSetState = this.setState.bind(this) + @connect( + state => ({ count: state }), + undefined, + undefined, + { context: customContext } + ) + class A extends Component { + render() { + return + } } - getChildContext() { - return { - statefulValue: this.state.value + const mapStateToPropsB = jest.fn(state => ({ count: state })) + @connect( + mapStateToPropsB, + undefined, + undefined, + { context: customContext } + ) + class B extends Component { + render() { + return } } - render() { - return + const mapStateToPropsC = jest.fn(state => ({ count: state })) + @connect( + mapStateToPropsC, + undefined, + undefined, + { context: customContext } + ) + class C extends Component { + render() { + return + } } - } - StatefulWrapper.childContextTypes = { - statefulValue: PropTypes.number - } + const mapStateToPropsD = jest.fn(state => ({ count: state })) + @connect(mapStateToPropsD) + class D extends Component { + render() { + return
{this.props.count}
+ } + } - const tester = rtl.render( - - - - ) + rtl.render( + + + + + + ) + expect(mapStateToPropsB).toHaveBeenCalledTimes(1) + expect(mapStateToPropsC).toHaveBeenCalledTimes(1) + expect(mapStateToPropsD).toHaveBeenCalledTimes(1) - expect(tester.getByTestId('statefulValue')).toHaveTextContent('0') - externalSetState({ value: 1 }) - expect(tester.getByTestId('statefulValue')).toHaveTextContent('1') - }) + rtl.act(() => { + store1.dispatch({ type: 'INC' }) + }) - it('calls mapState and mapDispatch for impure components', () => { - const store = createStore(() => ({ - foo: 'foo', - bar: 'bar' - })) + expect(mapStateToPropsB).toHaveBeenCalledTimes(1) + expect(mapStateToPropsC).toHaveBeenCalledTimes(1) + expect(mapStateToPropsD).toHaveBeenCalledTimes(2) - const mapStateSpy = jest.fn() - const mapDispatchSpy = jest.fn().mockReturnValue({}) - const impureRenderSpy = jest.fn() + rtl.act(() => { + store2.dispatch({ type: 'INC' }) + }) + expect(mapStateToPropsB).toHaveBeenCalledTimes(2) + expect(mapStateToPropsC).toHaveBeenCalledTimes(2) + expect(mapStateToPropsD).toHaveBeenCalledTimes(2) + }) + }) - class ImpureComponent extends Component { - render() { - impureRenderSpy() - return - } - } + describe('Refs', () => { + it.skip('should throw when trying to access the wrapped instance if withRef is not specified', () => { + const store = createStore(() => ({})) - const decorator = connect( - (state, { storeGetter }) => { - mapStateSpy() - return { value: state[storeGetter.storeKey] } - }, - mapDispatchSpy, - null, - { pure: false } - ) - const Decorated = decorator(ImpureComponent) - - let externalSetState - let storeGetter - class StatefulWrapper extends Component { - constructor() { - super() - storeGetter = { storeKey: 'foo' } - this.state = { - storeGetter - } - externalSetState = this.setState.bind(this) - } - render() { - return + class Container extends Component { + render() { + return + } } - } - const tester = rtl.render( - - - - ) - - // 1) Initial render - // 2) Post-mount check - // 3) After "wasted" re-render - expect(mapStateSpy).toHaveBeenCalledTimes(2) - expect(mapDispatchSpy).toHaveBeenCalledTimes(2) - - // 1) Initial render - // 2) Triggered by post-mount check with impure results - expect(impureRenderSpy).toHaveBeenCalledTimes(2) - expect(tester.getByTestId('statefulValue')).toHaveTextContent('foo') - - // Impure update - storeGetter.storeKey = 'bar' - externalSetState({ storeGetter }) - - // 4) After the the impure update - expect(mapStateSpy).toHaveBeenCalledTimes(3) - expect(mapDispatchSpy).toHaveBeenCalledTimes(3) - - // 3) Triggered by impure update - expect(impureRenderSpy).toHaveBeenCalledTimes(3) - expect(tester.getByTestId('statefulValue')).toHaveTextContent('bar') - }) + const decorator = connect(state => state) + const Decorated = decorator(Container) - it('should pass state consistently to mapState', () => { - const store = createStore(stringBuilder) + class Wrapper extends Component { + render() { + return comp && comp.getWrappedInstance()} /> + } + } - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) + // TODO Remove this when React is fixed, per https://github.com/facebook/react/issues/11098 + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + expect(() => + rtl.render( + + + + ) + ).toThrow( + `To access the wrapped instance, you need to specify { withRef: true } in the options argument of the connect() call` + ) + spy.mockRestore() }) - let childMapStateInvokes = 0 + it('should return the instance of the wrapped component for use in calling child methods', async done => { + const store = createStore(() => ({})) - @connect(state => ({ state })) - class Container extends Component { - emitChange() { - store.dispatch({ type: 'APPEND', body: 'b' }) + const someData = { + some: 'data' } - render() { - return ( -
- - -
- ) + class Container extends Component { + someInstanceMethod() { + return someData + } + + render() { + return + } } - } - const childCalls = [] - @connect((state, parentProps) => { - childMapStateInvokes++ - childCalls.push([state, parentProps.parentState]) - // The state from parent props should always be consistent with the current state - expect(state).toEqual(parentProps.parentState) - return {} - }) - class ChildContainer extends Component { - render() { - return + const decorator = connect( + state => state, + null, + null, + { forwardRef: true } + ) + const Decorated = decorator(Container) + + const ref = React.createRef() + + class Wrapper extends Component { + render() { + return + } } - } - const tester = rtl.render( - - - - ) + const tester = rtl.render( + + + + ) - expect(childMapStateInvokes).toBe(1) - expect(childCalls).toEqual([['a', 'a']]) + await rtl.waitForElement(() => tester.getByTestId('loaded')) - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'c' }) + expect(ref.current.someInstanceMethod()).toBe(someData) + done() }) - expect(childMapStateInvokes).toBe(2) - expect(childCalls).toEqual([['a', 'a'], ['ac', 'ac']]) - // setState calls DOM handlers are batched - const button = tester.getByText('change') - rtl.fireEvent.click(button) - expect(childMapStateInvokes).toBe(3) + it('should correctly separate and pass through props to the wrapped component with a forwarded ref', () => { + const store = createStore(() => ({})) - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'd' }) - }) + class Container extends Component { + render() { + return + } + } - expect(childMapStateInvokes).toBe(4) - expect(childCalls).toEqual([ - ['a', 'a'], - ['ac', 'ac'], - ['acb', 'acb'], - ['acbd', 'acbd'] - ]) - }) + const decorator = connect( + state => state, + null, + null, + { forwardRef: true } + ) + const Decorated = decorator(Container) - it('should not render the wrapped component when mapState does not produce change', () => { - const store = createStore(stringBuilder) - let renderCalls = 0 - let mapStateCalls = 0 + const ref = React.createRef() - @connect(() => { - mapStateCalls++ - return {} // no change! - }) - class Container extends Component { - render() { - renderCalls++ - return + class Wrapper extends Component { + render() { + // The 'a' prop should eventually be passed to the wrapped component individually, + // not sent through as `wrapperProps={ {a : 42} }` + return + } } - } - - rtl.render( - - - - ) - expect(renderCalls).toBe(1) - expect(mapStateCalls).toBe(1) + const tester = rtl.render( + + + + ) - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) + expect(tester.getByTestId('a')).toHaveTextContent('42') }) - - // After store a change mapState has been called - expect(mapStateCalls).toBe(2) - // But render is not because it did not make any actual changes - expect(renderCalls).toBe(1) }) - it('should bail out early if mapState does not depend on props', () => { - const store = createStore(stringBuilder) - let renderCalls = 0 - let mapStateCalls = 0 + describe('Impure behavior', () => { + it('should return the instance of the wrapped component for use in calling child methods, impure component', async done => { + const store = createStore(() => ({})) - @connect(state => { - mapStateCalls++ - return state === 'aaa' ? { change: 1 } : {} - }) - class Container extends Component { - render() { - renderCalls++ - return + const someData = { + some: 'data' } - } - rtl.render( - - - - ) + class Container extends Component { + someInstanceMethod() { + return someData + } - expect(renderCalls).toBe(1) - expect(mapStateCalls).toBe(1) + render() { + return + } + } - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) - }) + const decorator = connect( + state => state, + undefined, + undefined, + { forwardRef: true, pure: false } + ) + const Decorated = decorator(Container) - expect(mapStateCalls).toBe(2) - expect(renderCalls).toBe(1) + const ref = React.createRef() - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) - }) + class Wrapper extends Component { + render() { + return + } + } + + const tester = rtl.render( + + + + ) - expect(mapStateCalls).toBe(3) - expect(renderCalls).toBe(1) + await rtl.waitForElement(() => tester.getByTestId('loaded')) - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) + expect(ref.current.someInstanceMethod()).toBe(someData) + done() }) - expect(mapStateCalls).toBe(4) - expect(renderCalls).toBe(2) - }) + it('should wrap impure components without supressing updates', () => { + const store = createStore(() => ({})) - it('should not swallow errors when bailing out early', () => { - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - 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 + class ImpureComponent extends Component { + render() { + return + } } - } - rtl.render( - - - - ) + ImpureComponent.contextTypes = { + statefulValue: PropTypes.number + } - expect(renderCalls).toBe(1) - expect(mapStateCalls).toBe(1) - expect(() => store.dispatch({ type: 'APPEND', body: 'a' })).toThrow() + const decorator = connect( + state => state, + null, + null, + { pure: false } + ) + const Decorated = decorator(ImpureComponent) + + let externalSetState + class StatefulWrapper extends Component { + constructor() { + super() + this.state = { value: 0 } + externalSetState = this.setState.bind(this) + } - spy.mockRestore() - }) + getChildContext() { + return { + statefulValue: this.state.value + } + } - it('should allow providing a factory function to mapStateToProps', () => { - let updatedCount = 0 - let memoizedReturnCount = 0 - const store = createStore(() => ({ value: 1 })) - - const mapStateFactory = () => { - let lastProp, lastVal, lastResult - return (state, props) => { - if (props.name === lastProp && lastVal === state.value) { - memoizedReturnCount++ - return lastResult - } - lastProp = props.name - lastVal = state.value - return (lastResult = { - someObject: { prop: props.name, stateVal: state.value } - }) + render() { + return + } } - } - @connect(mapStateFactory) - class Container extends Component { - componentDidUpdate() { - updatedCount++ - } - render() { - return + StatefulWrapper.childContextTypes = { + statefulValue: PropTypes.number } - } - rtl.render( - -
- - -
- - ) + const tester = rtl.render( + + + + ) - rtl.act(() => { - store.dispatch({ type: 'test' }) + expect(tester.getByTestId('statefulValue')).toHaveTextContent('0') + externalSetState({ value: 1 }) + expect(tester.getByTestId('statefulValue')).toHaveTextContent('1') }) - expect(updatedCount).toBe(0) - expect(memoizedReturnCount).toBe(2) - }) + it('calls mapState and mapDispatch for impure components', () => { + const store = createStore(() => ({ + foo: 'foo', + bar: 'bar' + })) - it('should allow a mapStateToProps factory consuming just state to return a function that gets ownProps', () => { - const store = createStore(() => ({ value: 1 })) - - let initialState - let initialOwnProps - let secondaryOwnProps - const mapStateFactory = function(factoryInitialState) { - initialState = factoryInitialState - initialOwnProps = arguments[1] - return (state, props) => { - secondaryOwnProps = props - return {} + const mapStateSpy = jest.fn() + const mapDispatchSpy = jest.fn().mockReturnValue({}) + const impureRenderSpy = jest.fn() + + class ImpureComponent extends Component { + render() { + impureRenderSpy() + return + } } - } - @connect(mapStateFactory) - class Container extends Component { - render() { - return + const decorator = connect( + (state, { storeGetter }) => { + mapStateSpy() + return { value: state[storeGetter.storeKey] } + }, + mapDispatchSpy, + null, + { pure: false } + ) + const Decorated = decorator(ImpureComponent) + + let externalSetState + let storeGetter + class StatefulWrapper extends Component { + constructor() { + super() + storeGetter = { storeKey: 'foo' } + this.state = { + storeGetter + } + externalSetState = this.setState.bind(this) + } + render() { + return + } } - } - rtl.render( - -
- -
- - ) + const tester = rtl.render( + + + + ) + + // 1) Initial render + // 2) Post-mount check + // 3) After "wasted" re-render + expect(mapStateSpy).toHaveBeenCalledTimes(2) + expect(mapDispatchSpy).toHaveBeenCalledTimes(2) - rtl.act(() => { - store.dispatch({ type: 'test' }) - }) + // 1) Initial render + // 2) Triggered by post-mount check with impure results + expect(impureRenderSpy).toHaveBeenCalledTimes(2) + expect(tester.getByTestId('statefulValue')).toHaveTextContent('foo') - expect(initialOwnProps).toBe(undefined) - expect(initialState).not.toBe(undefined) - expect(secondaryOwnProps).not.toBe(undefined) - expect(secondaryOwnProps.name).toBe('a') - }) + // Impure update + storeGetter.storeKey = 'bar' + externalSetState({ storeGetter }) - it('should allow providing a factory function to mapDispatchToProps', () => { - let updatedCount = 0 - let memoizedReturnCount = 0 - const store = createStore(() => ({ value: 1 })) + // 4) After the the impure update + expect(mapStateSpy).toHaveBeenCalledTimes(3) + expect(mapDispatchSpy).toHaveBeenCalledTimes(3) - const mapDispatchFactory = () => { - let lastProp, lastResult - return (dispatch, props) => { - if (props.name === lastProp) { - memoizedReturnCount++ - return lastResult - } - lastProp = props.name - return (lastResult = { someObject: { dispatchFn: dispatch } }) - } - } - function mergeParentDispatch(stateProps, dispatchProps, parentProps) { - return { ...stateProps, ...dispatchProps, name: parentProps.name } - } + // 3) Triggered by impure update + expect(impureRenderSpy).toHaveBeenCalledTimes(3) + expect(tester.getByTestId('statefulValue')).toHaveTextContent('bar') + }) - @connect( - null, - mapDispatchFactory, - mergeParentDispatch - ) - class Passthrough extends Component { - componentDidUpdate() { - updatedCount++ - } - render() { - return
- } - } + it('should update impure components whenever the state of the store changes', () => { + const store = createStore(() => ({})) + let renderCount = 0 - class Container extends Component { - constructor(props) { - super(props) - this.state = { count: 0 } - } - componentDidMount() { - this.setState({ count: 1 }) - } - render() { - const { count } = this.state - return ( -
- - -
- ) + @connect( + () => ({}), + null, + null, + { pure: false } + ) + class ImpureComponent extends React.Component { + render() { + ++renderCount + return
+ } } - } - rtl.render( - - - - ) + rtl.render( + + + + ) + + const rendersBeforeStateChange = renderCount + rtl.act(() => { + store.dispatch({ type: 'ACTION' }) + }) - rtl.act(() => { - store.dispatch({ type: 'test' }) + expect(renderCount).toBe(rendersBeforeStateChange + 1) }) - expect(updatedCount).toBe(0) - expect(memoizedReturnCount).toBe(2) - }) + it('should update impure components with custom mergeProps', () => { + let store = createStore(() => ({})) + let renderCount = 0 - it('should not call update if mergeProps return value has not changed', () => { - let mapStateCalls = 0 - let renderCalls = 0 - const store = createStore(stringBuilder) - - @connect( - () => ({ a: ++mapStateCalls }), - null, - () => ({ changed: false }) - ) - class Container extends Component { - render() { - renderCalls++ - return + @connect( + null, + null, + () => ({ a: 1 }), + { pure: false } + ) + class Container extends React.Component { + render() { + ++renderCount + return
+ } } - } - rtl.render( - - - - ) + class Parent extends React.Component { + componentDidMount() { + this.forceUpdate() + } + render() { + return + } + } - expect(renderCalls).toBe(1) - expect(mapStateCalls).toBe(1) + rtl.render( + + + + + + ) - rtl.act(() => { - store.dispatch({ type: 'APPEND', body: 'a' }) + expect(renderCount).toBe(2) }) - - expect(mapStateCalls).toBe(2) - expect(renderCalls).toBe(1) }) - it('should update impure components with custom mergeProps', () => { - let store = createStore(() => ({})) - let renderCount = 0 - - @connect( - null, - null, - () => ({ a: 1 }), - { pure: false } - ) - class Container extends React.Component { - render() { - ++renderCount - return
- } - } + describe('Factory functions for mapState/mapDispatch', () => { + it('should allow providing a factory function to mapStateToProps', () => { + let updatedCount = 0 + let memoizedReturnCount = 0 + const store = createStore(() => ({ value: 1 })) - class Parent extends React.Component { - componentDidMount() { - this.forceUpdate() + const mapStateFactory = () => { + let lastProp, lastVal, lastResult + return (state, props) => { + if (props.name === lastProp && lastVal === state.value) { + memoizedReturnCount++ + return lastResult + } + lastProp = props.name + lastVal = state.value + return (lastResult = { + someObject: { prop: props.name, stateVal: state.value } + }) + } } - render() { - return + + @connect(mapStateFactory) + class Container extends Component { + componentDidUpdate() { + updatedCount++ + } + render() { + return + } } - } - rtl.render( - - - - - - ) + rtl.render( + +
+ + +
+ + ) - expect(renderCount).toBe(2) - }) + rtl.act(() => { + store.dispatch({ type: 'test' }) + }) - 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 - } - } + expect(updatedCount).toBe(0) + expect(memoizedReturnCount).toBe(2) + }) - @connect(null) - class Parent extends React.Component { - componentWillUnmount() { - this.props.dispatch({ type: 'clean' }) + it('should allow a mapStateToProps factory consuming just state to return a function that gets ownProps', () => { + const store = createStore(() => ({ value: 1 })) + + let initialState + let initialOwnProps + let secondaryOwnProps + const mapStateFactory = function(factoryInitialState) { + initialState = factoryInitialState + initialOwnProps = arguments[1] + return (state, props) => { + secondaryOwnProps = props + return {} + } } - render() { - return + @connect(mapStateFactory) + class Container extends Component { + render() { + return + } } - } - function mapState(state) { - return { - profile: state.data.profile - } - } + rtl.render( + +
+ +
+ + ) - @connect(mapState) - class Child extends React.Component { - render() { - return null - } - } + rtl.act(() => { + store.dispatch({ type: 'test' }) + }) - const store = createStore(reducer) - rtl.act(() => { - store.dispatch({ type: 'fetch' }) + expect(initialOwnProps).toBe(undefined) + expect(initialState).not.toBe(undefined) + expect(secondaryOwnProps).not.toBe(undefined) + expect(secondaryOwnProps.name).toBe('a') }) - const div = document.createElement('div') - ReactDOM.render( - - - , - div - ) - - ReactDOM.unmountComponentAtNode(div) - }) - - it('should allow custom displayName', () => { - @connect( - null, - null, - null, - { getDisplayName: name => `Custom(${name})` } - ) - class MyComponent extends React.Component { - render() { - return
+ it('should allow providing a factory function to mapDispatchToProps', () => { + let updatedCount = 0 + let memoizedReturnCount = 0 + const store = createStore(() => ({ value: 1 })) + + const mapDispatchFactory = () => { + let lastProp, lastResult + return (dispatch, props) => { + if (props.name === lastProp) { + memoizedReturnCount++ + return lastResult + } + lastProp = props.name + return (lastResult = { someObject: { dispatchFn: dispatch } }) + } } - } - - expect(MyComponent.displayName).toEqual('Custom(MyComponent)') - }) - - it('should update impure components whenever the state of the store changes', () => { - const store = createStore(() => ({})) - let renderCount = 0 - - @connect( - () => ({}), - null, - null, - { pure: false } - ) - class ImpureComponent extends React.Component { - render() { - ++renderCount - return
+ function mergeParentDispatch(stateProps, dispatchProps, parentProps) { + return { ...stateProps, ...dispatchProps, name: parentProps.name } } - } - rtl.render( - - - - ) - - const rendersBeforeStateChange = renderCount - rtl.act(() => { - store.dispatch({ type: 'ACTION' }) - }) - - expect(renderCount).toBe(rendersBeforeStateChange + 1) - }) + @connect( + null, + mapDispatchFactory, + mergeParentDispatch + ) + class Passthrough extends Component { + componentDidUpdate() { + updatedCount++ + } + render() { + return
+ } + } - function renderWithBadConnect(Component) { - const store = createStore(() => ({})) - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + class Container extends Component { + constructor(props) { + super(props) + this.state = { count: 0 } + } + componentDidMount() { + this.setState({ count: 1 }) + } + render() { + const { count } = this.state + return ( +
+ + +
+ ) + } + } - try { rtl.render( - + ) - return null - } catch (error) { - return error.message - } finally { - spy.mockRestore() - } - } - it('should throw a helpful error for invalid mapStateToProps arguments', () => { - @connect('invalid') - class InvalidMapState extends React.Component { - render() { - return
- } - } + rtl.act(() => { + store.dispatch({ type: 'test' }) + }) - const error = renderWithBadConnect(InvalidMapState) - expect(error).toContain('string') - expect(error).toContain('mapStateToProps') - expect(error).toContain('InvalidMapState') + expect(updatedCount).toBe(0) + expect(memoizedReturnCount).toBe(2) + }) }) - it('should throw a helpful error for invalid mapDispatchToProps arguments', () => { - @connect( - null, - 'invalid' - ) - class InvalidMapDispatch extends React.Component { - render() { - return
+ describe('Error handling for invalid arguments', () => { + function renderWithBadConnect(Component) { + const store = createStore(() => ({})) + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + + try { + rtl.render( + + + + ) + return null + } catch (error) { + return error.message + } finally { + spy.mockRestore() } } - const error = renderWithBadConnect(InvalidMapDispatch) - expect(error).toContain('string') - expect(error).toContain('mapDispatchToProps') - expect(error).toContain('InvalidMapDispatch') - }) - - it('should throw a helpful error for invalid mergeProps arguments', () => { - @connect( - null, - null, - 'invalid' - ) - class InvalidMerge extends React.Component { - render() { - return
+ it('should throw a helpful error for invalid mapStateToProps arguments', () => { + @connect('invalid') + class InvalidMapState extends React.Component { + render() { + return
+ } } - } - const error = renderWithBadConnect(InvalidMerge) - expect(error).toContain('string') - expect(error).toContain('mergeProps') - expect(error).toContain('InvalidMerge') - }) + const error = renderWithBadConnect(InvalidMapState) + expect(error).toContain('string') + expect(error).toContain('mapStateToProps') + expect(error).toContain('InvalidMapState') + }) - it('should notify nested components through a blocking component', () => { - @connect(state => ({ count: state })) - class Parent extends Component { - render() { - return ( - - - - ) + it('should throw a helpful error for invalid mapDispatchToProps arguments', () => { + @connect( + null, + 'invalid' + ) + class InvalidMapDispatch extends React.Component { + render() { + return
+ } } - } - class BlockUpdates extends Component { - shouldComponentUpdate() { - return false - } - render() { - return this.props.children - } - } + const error = renderWithBadConnect(InvalidMapDispatch) + expect(error).toContain('string') + expect(error).toContain('mapDispatchToProps') + expect(error).toContain('InvalidMapDispatch') + }) - const mapStateToProps = jest.fn(state => ({ count: state })) - @connect(mapStateToProps) - class Child extends Component { - render() { - return
{this.props.count}
+ it('should throw a helpful error for invalid mergeProps arguments', () => { + @connect( + null, + null, + 'invalid' + ) + class InvalidMerge extends React.Component { + render() { + return
+ } } - } - - const store = createStore((state = 0, action) => - action.type === 'INC' ? state + 1 : state - ) - rtl.render( - - - - ) - expect(mapStateToProps).toHaveBeenCalledTimes(1) - rtl.act(() => { - store.dispatch({ type: 'INC' }) + const error = renderWithBadConnect(InvalidMerge) + expect(error).toContain('string') + expect(error).toContain('mergeProps') + expect(error).toContain('InvalidMerge') }) - - expect(mapStateToProps).toHaveBeenCalledTimes(2) }) - it('should subscribe properly when a middle connected component does not subscribe', () => { - @connect(state => ({ count: state })) - class A extends React.Component { - render() { - return - } - } - - @connect() // no mapStateToProps. therefore it should be transparent for subscriptions - class B extends React.Component { - render() { - return + describe('Error handling for removed API options and StrictMode', () => { + it('should error on withRef=true', () => { + class Container extends Component { + render() { + return
hi
+ } } - } - - @connect((state, props) => { - expect(props.count).toBe(state) - return { count: state * 10 + props.count } + expect(() => + connect( + undefined, + undefined, + undefined, + { withRef: true } + )(Container) + ).toThrow(/withRef is removed/) }) - class C extends React.Component { - render() { - return
{this.props.count}
- } - } - const store = createStore((state = 0, action) => - action.type === 'INC' ? (state += 1) : state - ) - rtl.render( - - - - ) + it('should error on receiving a custom store key', () => { + const connectOptions = { storeKey: 'customStoreKey' } - rtl.act(() => { - store.dispatch({ type: 'INC' }) + expect(() => { + @connect( + undefined, + undefined, + undefined, + connectOptions + ) + class Container extends Component { + render() { + return + } + } + new Container() + }).toThrow(/storeKey has been removed/) }) - }) - it('should subscribe properly when a new store is provided via props', () => { - const store1 = createStore((state = 0, action) => - action.type === 'INC' ? state + 1 : state - ) - const store2 = createStore((state = 0, action) => - action.type === 'INC' ? state + 1 : state - ) - const customContext = React.createContext() - - @connect( - state => ({ count: state }), - undefined, - undefined, - { context: customContext } - ) - class A extends Component { - render() { - return + it.skip('should error on custom store', () => { + function Comp() { + return
hi
} - } - - const mapStateToPropsB = jest.fn(state => ({ count: state })) - @connect( - mapStateToPropsB, - undefined, - undefined, - { context: customContext } - ) - class B extends Component { - render() { - return + const Container = connect()(Comp) + function Oops() { + return } - } + expect(() => { + rtl.render() + }).toThrow(/Passing redux store/) + }) - const mapStateToPropsC = jest.fn(state => ({ count: state })) - @connect( - mapStateToPropsC, - undefined, - undefined, - { context: customContext } - ) - class C extends Component { - render() { - return - } - } + it('should error on renderCount prop if specified in connect options', () => { + function Comp(props) { + return
{props.count}
+ } + expect(() => { + connect( + undefined, + undefined, + undefined, + { renderCountProp: 'count' } + )(Comp) + }).toThrow(/renderCountProp is removed/) + }) - const mapStateToPropsD = jest.fn(state => ({ count: state })) - @connect(mapStateToPropsD) - class D extends Component { - render() { - return
{this.props.count}
+ it('works in without warnings (React 16.3+)', () => { + if (!React.StrictMode) { + return } - } - - rtl.render( - - - - - - ) - expect(mapStateToPropsB).toHaveBeenCalledTimes(1) - expect(mapStateToPropsC).toHaveBeenCalledTimes(1) - expect(mapStateToPropsD).toHaveBeenCalledTimes(1) + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + const store = createStore(stringBuilder) - rtl.act(() => { - store1.dispatch({ type: 'INC' }) - }) + @connect(state => ({ string: state })) + class Container extends Component { + render() { + return + } + } - expect(mapStateToPropsB).toHaveBeenCalledTimes(1) - expect(mapStateToPropsC).toHaveBeenCalledTimes(1) - expect(mapStateToPropsD).toHaveBeenCalledTimes(2) + rtl.render( + + + + + + ) - rtl.act(() => { - store2.dispatch({ type: 'INC' }) + expect(spy).not.toHaveBeenCalled() }) - expect(mapStateToPropsB).toHaveBeenCalledTimes(2) - expect(mapStateToPropsC).toHaveBeenCalledTimes(2) - expect(mapStateToPropsD).toHaveBeenCalledTimes(2) }) - it('works in without warnings (React 16.3+)', () => { - if (!React.StrictMode) { - return - } - const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) - const store = createStore(stringBuilder) + describe('Subscription and update timing correctness', () => { + it('should pass state consistently to mapState', () => { + const store = createStore(stringBuilder) - @connect(state => ({ string: state })) - class Container extends Component { - render() { - return - } - } + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'a' }) + }) - rtl.render( - - - - - - ) + let childMapStateInvokes = 0 - expect(spy).not.toHaveBeenCalled() - }) + @connect(state => ({ state })) + class Container extends Component { + emitChange() { + store.dispatch({ type: 'APPEND', body: 'b' }) + } - it('should error on withRef=true', () => { - class Container extends Component { - render() { - return
hi
+ render() { + return ( +
+ + +
+ ) + } } - } - expect(() => - connect( - undefined, - undefined, - undefined, - { withRef: true } - )(Container) - ).toThrow(/withRef is removed/) - }) - it('should error on receiving a custom store key', () => { - const connectOptions = { storeKey: 'customStoreKey' } - - expect(() => { - @connect( - undefined, - undefined, - undefined, - connectOptions - ) - class Container extends Component { + const childCalls = [] + @connect((state, parentProps) => { + childMapStateInvokes++ + childCalls.push([state, parentProps.parentState]) + // The state from parent props should always be consistent with the current state + expect(state).toEqual(parentProps.parentState) + return {} + }) + class ChildContainer extends Component { render() { return } } - new Container() - }).toThrow(/storeKey has been removed/) - }) - it.skip('should error on custom store', () => { - function Comp() { - return
hi
- } - const Container = connect()(Comp) - function Oops() { - return - } - expect(() => { - rtl.render() - }).toThrow(/Passing redux store/) - }) + const tester = rtl.render( + + + + ) - it('should error on renderCount prop if specified in connect options', () => { - function Comp(props) { - return
{props.count}
- } - expect(() => { - connect( - undefined, - undefined, - undefined, - { renderCountProp: 'count' } - )(Comp) - }).toThrow(/renderCountProp is removed/) - }) + expect(childMapStateInvokes).toBe(1) + expect(childCalls).toEqual([['a', 'a']]) - it('should not error on valid component with circular structure', () => { - const createComp = Tag => { - const Comp = React.forwardRef(function Comp(props) { - return {props.count} + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'c' }) }) - Comp.__real = Comp - return Comp - } - - expect(() => { - connect()(createComp('div')) - }).not.toThrow() - }) + expect(childMapStateInvokes).toBe(2) + expect(childCalls).toEqual([['a', 'a'], ['ac', 'ac']]) - it('should invoke mapState always with latest props', () => { - const store = createStore((state = 0) => state + 1) + // setState calls DOM handlers are batched + const button = tester.getByText('change') + rtl.fireEvent.click(button) + expect(childMapStateInvokes).toBe(3) - let propsPassedIn + rtl.act(() => { + store.dispatch({ type: 'APPEND', body: 'd' }) + }) - @connect(reduxCount => { - return { reduxCount } + expect(childMapStateInvokes).toBe(4) + expect(childCalls).toEqual([ + ['a', 'a'], + ['ac', 'ac'], + ['acb', 'acb'], + ['acbd', 'acbd'] + ]) }) - class InnerComponent extends Component { - render() { - propsPassedIn = this.props - return - } - } - class OuterComponent extends Component { - constructor() { - super() - this.state = { count: 0 } - } + it('should invoke mapState always with latest props', () => { + const store = createStore((state = 0) => state + 1) - render() { - return + let propsPassedIn + + @connect(reduxCount => { + return { reduxCount } + }) + class InnerComponent extends Component { + render() { + propsPassedIn = this.props + return + } } - } - let outerComponent - rtl.render( - - (outerComponent = c)} /> - - ) - outerComponent.setState(({ count }) => ({ count: count + 1 })) - store.dispatch({ type: '' }) - - expect(propsPassedIn.count).toEqual(1) - expect(propsPassedIn.reduxCount).toEqual(2) - }) + class OuterComponent extends Component { + constructor() { + super() + this.state = { count: 0 } + } - it('should use the latest props when updated between actions', () => { - const store = applyMiddleware(store => { - let callback - return next => action => { - if (action.type === 'SET_COMPONENT_CALLBACK') { - callback = action.payload + render() { + return } - if (callback && action.type === 'INC1') { + } + + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) + outerComponent.setState(({ count }) => ({ count: count + 1 })) + store.dispatch({ type: '' }) + + expect(propsPassedIn.count).toEqual(1) + expect(propsPassedIn.reduxCount).toEqual(2) + }) + + it('should use the latest props when updated between actions', () => { + const reactCallbackMiddleware = store => { + let callback + + return next => action => { + if (action.type === 'SET_COMPONENT_CALLBACK') { + callback = action.payload + } + + if (callback && action.type === 'INC1') { + // Deliberately create multiple updates of different types in a row: + // 1) Store update causes subscriber notifications + next(action) + // 2) React setState outside batching causes a sync re-render. + // Because we're not using `act()`, this won't flush pending passive effects, + // simulating + callback() + // 3) Second dispatch causes subscriber notifications again. If `connect` is working + // correctly, nested subscriptions won't execute until the parents have rendered, + // to ensure that the subscriptions have access to the latest wrapper props. + store.dispatch({ type: 'INC2' }) + return + } + next(action) - callback() - store.dispatch({ type: 'INC2' }) - return } - next(action) - } - })(createStore)((state = 0, action) => { - if (action.type === 'INC1') { - return state + 1 - } else if (action.type === 'INC2') { - return state + 2 } - return state - }) - const Child = connect(count => ({ count }))(function(props) { - return ( -
- ) - }) - class Parent extends Component { - constructor() { - super() - this.state = { - prop: 'a' - } - this.inc1 = () => store.dispatch({ type: 'INC1' }) - store.dispatch({ - type: 'SET_COMPONENT_CALLBACK', - payload: () => this.setState({ prop: 'b' }) - }) + + const counter = (state = 0, action) => { + if (action.type === 'INC1') { + return state + 1 + } else if (action.type === 'INC2') { + return state + 2 + } + return state } - render() { + const store = createStore( + counter, + applyMiddleware(reactCallbackMiddleware) + ) + + const Child = connect(count => ({ count }))(function(props) { return ( - - - +
) + }) + class Parent extends Component { + constructor() { + super() + this.state = { + prop: 'a' + } + this.inc1 = () => store.dispatch({ type: 'INC1' }) + store.dispatch({ + type: 'SET_COMPONENT_CALLBACK', + payload: () => this.setState({ prop: 'b' }) + }) + } + + render() { + return ( + + + + ) + } } - } - let parent - const rendered = rtl.render( (parent = ref)} />) - expect(rendered.getByTestId('child').dataset.count).toEqual('0') - expect(rendered.getByTestId('child').dataset.prop).toEqual('a') - parent.inc1() - expect(rendered.getByTestId('child').dataset.count).toEqual('3') - expect(rendered.getByTestId('child').dataset.prop).toEqual('b') + + let parent + const rendered = rtl.render( (parent = ref)} />) + expect(rendered.getByTestId('child').dataset.count).toEqual('0') + expect(rendered.getByTestId('child').dataset.prop).toEqual('a') + + // Force the multi-update sequence by running this bound action creator + parent.inc1() + + // The connected child component _should_ have rendered with the latest Redux + // store value (3) _and_ the latest wrapper prop ('b'). + expect(rendered.getByTestId('child').dataset.count).toEqual('3') + expect(rendered.getByTestId('child').dataset.prop).toEqual('b') + }) }) }) }) From c9145809b19367b291a0290e8f13fea8f7e80ae8 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Thu, 21 Mar 2019 20:37:04 -0400 Subject: [PATCH 25/39] Clean up dead code in Provider tests --- test/components/Provider.spec.js | 38 +------------------------------- 1 file changed, 1 insertion(+), 37 deletions(-) diff --git a/test/components/Provider.spec.js b/test/components/Provider.spec.js index d6dbf135d..200c5c87a 100644 --- a/test/components/Provider.spec.js +++ b/test/components/Provider.spec.js @@ -14,24 +14,8 @@ describe('React', () => { afterEach(() => rtl.cleanup()) const createChild = (storeKey = 'store') => { - /* class Child extends Component { render() { - return ( - - {({ storeState }) => { - return ( -
{`${storeKey} - ${storeState}`}
- ) - }} - - ) - } - } - */ - class Child extends Component { - render() { - //const store = this.context[storeKey]; return ( {({ store }) => { @@ -49,20 +33,6 @@ describe('React', () => { }} ) - - /* - let text = ''; - - if(store) { - text = store.getState().toString() - } - - return ( -
- {storeKey} - {text} -
- ) - */ } } @@ -103,7 +73,7 @@ describe('React', () => { Provider.propTypes = propTypes }) - it('should add the store state to context', () => { + it('should add the store to context', () => { const store = createStore(createExampleTextReducer()) const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) @@ -145,12 +115,6 @@ describe('React', () => { const tester = rtl.render() expect(tester.getByTestId('store')).toHaveTextContent('store - 11') - /* - rtl.act(() => { - store1.dispatch({ type: 'hi' }) - }) - expect(tester.getByTestId('store')).toHaveTextContent('store - 12') -*/ rtl.act(() => { externalSetState({ store: store2 }) }) From efeadb5b417ced8cd3dc1e61c9fb71092c731983 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Thu, 21 Mar 2019 21:39:03 -0400 Subject: [PATCH 26/39] Add tests to verify errors are thrown for bad mapState functions --- test/components/connect.spec.js | 109 +++++++++++++++++++++- test/integration/server-rendering.spec.js | 1 + 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 150dc0732..9d9c21005 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1696,7 +1696,7 @@ describe('React', () => { it('should not error on valid component with circular structure', () => { const createComp = Tag => { - const Comp = React.forwardRef(function Comp(props) { + const Comp = React.forwardRef(function Comp(props, ref) { return {props.count} }) Comp.__real = Comp @@ -3111,5 +3111,112 @@ describe('React', () => { expect(rendered.getByTestId('child').dataset.prop).toEqual('b') }) }) + + it("should enforce top-down updates to ensure a deleted child's mapState doesn't throw errors", () => { + const initialState = { + a: { id: 'a', name: 'Item A' }, + b: { id: 'b', name: 'Item B' }, + c: { id: 'c', name: 'Item C' } + } + + const reducer = (state = initialState, action) => { + switch (action.type) { + case 'DELETE_B': { + const newState = { ...state } + delete newState.b + return newState + } + default: + return state + } + } + + const store = createStore(reducer) + + const ListItem = ({ name }) =>
Name: {name}
+ + let thrownError = null + + const listItemMapState = (state, ownProps) => { + try { + const item = state[ownProps.id] + // If this line executes when item B has been deleted, it will throw an error. + // For this test to succeed, we should never execute mapState for item B after the item + // has been deleted, because the parent should re-render the component out of existence. + const { name } = item + return { name } + } catch (e) { + thrownError = e + } + } + + const ConnectedListItem = connect(listItemMapState)(ListItem) + + const appMapState = state => { + const itemIds = Object.keys(state) + return { itemIds } + } + + function App({ itemIds, deleteB }) { + const items = itemIds.map(id => ) + + return ( +
+ {items} + +
+ ) + } + + const ConnectedApp = connect(appMapState)(App) + + const tester = rtl.render( + + + + ) + + // This should execute without throwing an error by itself + rtl.act(() => { + store.dispatch({ type: 'DELETE_B' }) + }) + + expect(thrownError).toBe(null) + }) + + it('should re-throw errors that occurred in a mapState/mapDispatch function', () => { + const counter = (state = 0, action) => + action.type === 'INCREMENT' ? state + 1 : state + + const store = createStore(counter) + + const appMapState = state => { + if (state >= 1) { + throw new Error('KABOOM!') + } + + return { counter: state } + } + + const App = ({ counter }) =>
Count: {counter}
+ const ConnectedApp = connect(appMapState)(App) + + const tester = rtl.render( + + + + ) + + // Turn off extra console logging + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + + expect(() => { + rtl.act(() => { + store.dispatch({ type: 'INCREMENT' }) + }) + }).toThrow('KABOOM!') + + spy.mockRestore() + }) }) }) diff --git a/test/integration/server-rendering.spec.js b/test/integration/server-rendering.spec.js index 5bf44e20e..c97fb4513 100644 --- a/test/integration/server-rendering.spec.js +++ b/test/integration/server-rendering.spec.js @@ -1,5 +1,6 @@ /** * @jest-environment node + * * Set this so that `window` is undefined to correctly mimic a Node SSR scenario. * That allows connectAdvanced to fall back to `useEffect` instead of `useLayoutEffect` * to avoid ugly console warnings when used with SSR. From 96fec159e2afb8649321d8cd61841b9cf406ae8f Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Thu, 21 Mar 2019 21:46:15 -0400 Subject: [PATCH 27/39] Fix edge cases around saved wrapper props and error handling Changed to useLayoutEffect to ensure that the lastWrapperProps ref is written to synchronously when a render is committed. However, because React warns about this in SSR, added a check to fall back to plain useEffect under Node so we don't print warnings. Also added logic to ensure that if an error is thrown during a mapState function, but the component is unmounted before it can render, that the error will still be thrown. This shouldn't happen given our top-down subscriptions, but this will help surface the problem in our tests if we ever break the top-down behavior. --- src/components/connectAdvanced.js | 37 ++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index aa8ea6460..4858bd8d5 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -4,6 +4,7 @@ import React, { useContext, useMemo, useEffect, + useLayoutEffect, useRef, useReducer } from 'react' @@ -31,6 +32,14 @@ function storeStateUpdatesReducer(state, action) { const initStateUpdates = () => [null, 0] +// React currently throws a warning when using useLayoutEffect on the server. +// To get around it, we can conditionally useEffect on the server (no-op) and +// useLayoutEffect in the browser. We need useLayoutEffect because we want +// `connect` to perform sync updates to a ref to save the latest props after +// a render is actually committed to the DOM. +const useIsomorphicLayoutEffect = + typeof window !== 'undefined' ? useLayoutEffect : useEffect + export default function connectAdvanced( /* selectorFactory is a func that is responsible for returning the selector function used to @@ -266,8 +275,10 @@ export default function connectAdvanced( return childPropsSelector(store.getState(), wrapperProps) }, [store, previousStateUpdateResult, wrapperProps]) - // Every time we do re-render: - useEffect(() => { + // We need this to execute synchronously every time we re-render. However, React warns + // about useLayoutEffect in SSR, so we try to detect environment and fall back to + // just useEffect instead to avoid the warning, since neither will run anyway. + useIsomorphicLayoutEffect(() => { // We want to capture the wrapper props and child props we used for later comparisons lastWrapperProps.current = wrapperProps lastChildProps.current = actualChildProps @@ -284,7 +295,9 @@ export default function connectAdvanced( // If we're not subscribed to the store, nothing to do here if (!shouldHandleStateChanges) return + // Capture values for checking if and when this component unmounts let didUnsubscribe = false + let lastThrownError = null // We'll run this callback every time a store subscription update propagates to this component const checkForUpdates = () => { @@ -306,6 +319,11 @@ export default function connectAdvanced( ) } catch (e) { error = e + lastThrownError = e + } + + if (!error) { + lastThrownError = null } // If the child props haven't changed, nothing to do here - cascade the subscription update @@ -330,17 +348,26 @@ export default function connectAdvanced( } } - // Pull data from the store after first render in case the store has - // changed since we began. - + // Actually subscribe to the nearest connected ancestor (or store) subscription.onStateChange = checkForUpdates subscription.trySubscribe() + // Pull data from the store after first render in case the store has + // changed since we began. checkForUpdates() const unsubscribeWrapper = () => { didUnsubscribe = true subscription.tryUnsubscribe() + + if (lastThrownError) { + // It's possible that we caught an error due to a bad mapState function, but the + // parent re-rendered without this component and we're about to unmount. + // This shouldn't happen as long as we do top-down subscriptions correctly, but + // if we ever do those wrong, this throw will surface the error in our tests. + // In that case, throw the error from here so it doesn't get lost. + throw lastThrownError + } } return unsubscribeWrapper From 847977431c96b19afa957bc19fc4c7ae1f3cf154 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Thu, 21 Mar 2019 21:46:28 -0400 Subject: [PATCH 28/39] Formatting --- docs/api/connect.md | 2 +- docs/introduction/quick-start.md | 1 - docs/using-react-redux/accessing-store.md | 8 ++++---- test/components/connect.spec.js | 8 ++++---- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/docs/api/connect.md b/docs/api/connect.md index 39105ade6..4ad61e58c 100644 --- a/docs/api/connect.md +++ b/docs/api/connect.md @@ -121,7 +121,7 @@ The second parameter is normally referred to as `ownProps` by convention. ```js // binds on component re-rendering -
) + class Dispatcher extends React.Component { + constructor(props) { + super(props) + if (props.constructAction) { + props.dispatch(props.constructAction) + } + } + UNSAFE_componentWillMount() { + if (this.props.willMountAction) { + this.props.dispatch(this.props.willMountAction) + } + } + render() { + if (this.props.renderAction) { + this.props.dispatch(this.props.renderAction) + } + + return + } + } + const ConnectedDispatcher = connect()(Dispatcher) + it('should be able to render connected component with props and state from store', () => { const store = createStore(greetingReducer) @@ -71,7 +93,7 @@ describe('React', () => { expect(store.getState().greeting).toContain('Hi') }) - it.skip('should render children with original state even if actions are dispatched in ancestor', () => { + it('should render children with updated state if actions are dispatched in ancestor', () => { /* Dispatching during construct, render or willMount is almost always a bug with SSR (or otherwise) @@ -79,40 +101,35 @@ describe('React', () => { This behaviour is undocumented and is likely to change between implementations, this test only verifies current behaviour - Note: this test passes in v6, because we use context to propagate the store state, and the entire + Note: this test fails in v6, because we use context to propagate the store state, and the entire tree will see the same state during the render pass. - In all other versions, including v7, the store state may change as actions are dispatched during lifecycle methods, and components will see that new state immediately as they read it. */ const store = createStore(greetingReducer) - class Dispatcher extends React.Component { - constructor(props) { - super(props) - props.dispatch(props.action) - } - UNSAFE_componentWillMount() { - this.props.dispatch(this.props.action) - } - render() { - this.props.dispatch(this.props.action) - - return - } - } - const ConnectedDispatcher = connect()(Dispatcher) - - const action = { type: 'Update', payload: { greeting: 'Hi' } } + const constructAction = { type: 'Update', payload: { greeting: 'Hi' } } + const willMountAction = { type: 'Update', payload: { greeting: 'Hiya' } } + const renderAction = { type: 'Update', payload: { greeting: 'Hey' } } const markup = renderToString( - + + + ) - expect(markup).toContain('Hello world') - expect(store.getState().greeting).toContain('Hi') + expect(markup).toContain('Hi world') + expect(markup).toContain('Hiya world') + expect(markup).toContain('Hey world') + expect(store.getState().greeting).toContain('Hey') }) it('should render children with changed state if actions are dispatched in ancestor and new Provider wraps children', () => { @@ -122,35 +139,12 @@ describe('React', () => { This behaviour is undocumented and is likely to change between implementations, this test only verifies current behaviour + + This test works both when state is fetched directly in connected + components and when it is fetched in a Provider and placed on context */ const store = createStore(greetingReducer) - class Dispatcher extends React.Component { - constructor(props) { - super(props) - if (props.constructAction) { - props.dispatch(props.constructAction) - } - } - UNSAFE_componentWillMount() { - if (this.props.willMountAction) { - this.props.dispatch(this.props.willMountAction) - } - } - render() { - if (this.props.renderAction) { - this.props.dispatch(this.props.renderAction) - } - - return ( - - - - ) - } - } - const ConnectedDispatcher = connect()(Dispatcher) - const constructAction = { type: 'Update', payload: { greeting: 'Hi' } } const willMountAction = { type: 'Update', payload: { greeting: 'Hiya' } } const renderAction = { type: 'Update', payload: { greeting: 'Hey' } } From 79982c912d03c55ac7059b61806a1387352e91f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20H=C3=B6glund?= Date: Fri, 22 Mar 2019 18:44:20 +0100 Subject: [PATCH 33/39] Added test for injecting dynamic reducers on client and server (#1211) --- test/integration/dynamic-reducers.spec.js | 165 ++++++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 test/integration/dynamic-reducers.spec.js diff --git a/test/integration/dynamic-reducers.spec.js b/test/integration/dynamic-reducers.spec.js new file mode 100644 index 000000000..dc2e3561b --- /dev/null +++ b/test/integration/dynamic-reducers.spec.js @@ -0,0 +1,165 @@ +/*eslint-disable react/prop-types*/ + +import React from 'react' +import ReactDOMServer from 'react-dom/server' +import { createStore, combineReducers } from 'redux' +import { connect, Provider, ReactReduxContext } from '../../src/index.js' +import * as rtl from 'react-testing-library' + +describe('React', () => { + /* + For SSR to work, there are three options for injecting + dynamic reducers: + + 1. Make sure all dynamic reducers are known before rendering + (requires keeping knowledge about this outside of the + React component-tree) + 2. Double rendering (first render injects required reducers) + 3. Inject reducers as a side effect during the render phase + (in construct or render), and try to control for any + issues with that. This requires grabbing the store from + context and possibly patching any storeState that exists + on there, these are undocumented APIs that might change + at any time. + + Because the tradeoffs in 1 and 2 are quite hefty and also + because it's the popular approach, this test targets nr 3. + */ + describe('dynamic reducers', () => { + const InjectReducersContext = React.createContext(null) + + function ExtraReducersProvider({ children, reducers }) { + return ( + + {injectReducers => ( + + {reduxContext => { + const latestState = reduxContext.store.getState() + const contextState = reduxContext.storeState + + let shouldInject = false + let shouldPatch = false + + for (const key of Object.keys(reducers)) { + // If any key does not exist in the latest version + // of the state, we need to inject reducers + if (!(key in latestState)) { + shouldInject = true + } + // If state exists on the context, and if any reducer + // key is not included there, we need to patch it up + // Only patching if storeState exists makes this test + // work with multiple React-Redux approaches + if (contextState && !(key in contextState)) { + shouldPatch = true + } + } + + if (shouldInject) { + injectReducers(reducers) + } + + if (shouldPatch) { + // A safer way to do this would be to patch the storeState + // manually with the state from the new reducers, since + // this would better avoid tearing in a future concurrent world + const patchedReduxContext = { + ...reduxContext, + storeState: reduxContext.store.getState() + } + return ( + + {children} + + ) + } + + return children + }} + + )} + + ) + } + + const initialReducer = { + initial: (state = { greeting: 'Hello world' }) => state + } + const dynamicReducer = { + dynamic: (state = { greeting: 'Hello dynamic world' }) => state + } + + function Greeter({ greeting }) { + return
{greeting}
+ } + + const InitialGreeting = connect(state => ({ + greeting: state.initial.greeting + }))(Greeter) + const DynamicGreeting = connect(state => ({ + greeting: state.dynamic.greeting + }))(Greeter) + + function createInjectReducers(store, initialReducer) { + let reducers = initialReducer + return function injectReducers(newReducers) { + reducers = { ...reducers, ...newReducers } + store.replaceReducer(combineReducers(reducers)) + } + } + + let store + let injectReducers + + beforeEach(() => { + // These could be singletons on the client, but + // need to be separate per request on the server + store = createStore(combineReducers(initialReducer)) + injectReducers = createInjectReducers(store, initialReducer) + }) + + it('should render child with initial state on the client', () => { + const { getByText } = rtl.render( + + + + + + + + + ) + + getByText('Hello world') + getByText('Hello dynamic world') + }) + it('should render child with initial state on the server', () => { + // In order to keep these tests together in the same file, + // we aren't currently rendering this test in the node test + // environment + // This generates errors for using useLayoutEffect in v7 + // We hide that error by disabling console.error here + + jest.spyOn(console, 'error') + // eslint-disable-next-line no-console + console.error.mockImplementation(() => {}) + + const markup = ReactDOMServer.renderToString( + + + + + + + + + ) + + expect(markup).toContain('Hello world') + expect(markup).toContain('Hello dynamic world') + + // eslint-disable-next-line no-console + console.error.mockRestore() + }) + }) +}) From 73c0fbdd59ec03ea1810e20f0d30c1dbb81a7475 Mon Sep 17 00:00:00 2001 From: Tim Dorr Date: Tue, 2 Apr 2019 21:29:40 -0400 Subject: [PATCH 34/39] Remove WebStorm gitignore This goes in a global gitignore file, not a project. --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index 827b359e3..251f3eb95 100644 --- a/.gitignore +++ b/.gitignore @@ -12,7 +12,6 @@ test/react/*/test/**/*.spec.js test/react/**/src test/jest-config.json lcov.info -.idea/ lib/core/metadata.js lib/core/MetadataBlog.js From 17451c11d429f1bf376650384a2de090de5935a0 Mon Sep 17 00:00:00 2001 From: Josep M Sobrepere Date: Thu, 4 Apr 2019 06:35:19 +0200 Subject: [PATCH 35/39] [FIX]: #1219 Save references before update (#1220) --- src/components/connectAdvanced.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 4858bd8d5..6169fcb9a 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -330,6 +330,13 @@ export default function connectAdvanced( if (newChildProps === lastChildProps.current) { notifyNestedSubs() } else { + // Save references to the new child props. Note that we track the "child props from store update" + // as a ref instead of a useState/useReducer because we need a way to determine if that value has + // been processed. If this went into useState/useReducer, we couldn't clear out the value without + // forcing another re-render, which we don't want. + lastChildProps.current = newChildProps + childPropsFromStoreUpdate.current = newChildProps + // If the child props _did_ change (or we caught an error), this wrapper component needs to re-render forceComponentUpdateDispatch({ type: 'STORE_UPDATED', @@ -338,13 +345,6 @@ export default function connectAdvanced( error } }) - - // Save references to the new child props. Note that we track the "child props from store update" - // as a ref instead of a useState/useReducer because we need a way to determine if that value has - // been processed. If this went into useState/useReducer, we couldn't clear out the value without - // forcing another re-render, which we don't want. - lastChildProps.current = newChildProps - childPropsFromStoreUpdate.current = newChildProps } } From 6c0f301b407021bc81c85e54cfbb3397d2d2af94 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Wed, 3 Apr 2019 21:45:22 -0700 Subject: [PATCH 36/39] Re-ignore .idea/ --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 251f3eb95..827b359e3 100644 --- a/.gitignore +++ b/.gitignore @@ -12,6 +12,7 @@ test/react/*/test/**/*.spec.js test/react/**/src test/jest-config.json lcov.info +.idea/ lib/core/metadata.js lib/core/MetadataBlog.js From 1097eedebe8536b12d516d568bbd0ed96a6b16e2 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Wed, 3 Apr 2019 21:45:41 -0700 Subject: [PATCH 37/39] 7.0.0-beta.1 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 996c4fedf..d90ddac25 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "react-redux", - "version": "7.0.0-beta.0", + "version": "7.0.0-beta.1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 3131ece7a..46edfb5d8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "react-redux", - "version": "7.0.0-beta.0", + "version": "7.0.0-beta.1", "description": "Official React bindings for Redux", "keywords": [ "react", From cd99c72982b3b5f7126be19101d57d975d3bf9d7 Mon Sep 17 00:00:00 2001 From: Tim Dorr Date: Thu, 4 Apr 2019 13:12:24 -0400 Subject: [PATCH 38/39] Update the codecov config to be more forgiving. --- codecov.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/codecov.yml b/codecov.yml index 69cb76019..76279fd1b 100644 --- a/codecov.yml +++ b/codecov.yml @@ -1 +1,7 @@ -comment: false +comment: off +coverage: + status: + project: + default: + threshold: 5% + patch: off From d8cd1a2b4f2ae0d39149c233a0bff2ac443e27ee Mon Sep 17 00:00:00 2001 From: Jonathan Ziller Date: Thu, 4 Apr 2019 22:00:01 +0200 Subject: [PATCH 39/39] add test to verify that mapStateToProps is always called with latest store state (#1215) --- test/components/connect.spec.js | 40 +++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 570c69d8d..9e8a3a9c6 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -3110,6 +3110,46 @@ describe('React', () => { expect(rendered.getByTestId('child').dataset.count).toEqual('3') expect(rendered.getByTestId('child').dataset.prop).toEqual('b') }) + + it('should invoke mapState always with latest store state', () => { + const store = createStore((state = 0) => state + 1) + + let reduxCountPassedToMapState + + @connect(reduxCount => { + reduxCountPassedToMapState = reduxCount + return reduxCount < 2 ? { a: 'a' } : { a: 'b' } + }) + class InnerComponent extends Component { + render() { + return + } + } + + class OuterComponent extends Component { + constructor() { + super() + this.state = { count: 0 } + } + + render() { + return + } + } + + let outerComponent + rtl.render( + + (outerComponent = c)} /> + + ) + + store.dispatch({ type: '' }) + store.dispatch({ type: '' }) + outerComponent.setState(({ count }) => ({ count: count + 1 })) + + expect(reduxCountPassedToMapState).toEqual(3) + }) }) it("should enforce top-down updates to ensure a deleted child's mapState doesn't throw errors", () => {