From 0fbe4feff81b7740909070e8b4313b96b76d243c Mon Sep 17 00:00:00 2001 From: Josep M Sobrepere Date: Wed, 21 Jun 2017 00:10:00 +0200 Subject: [PATCH] Supporting factory selectors in the object mapToProps argument --- docs/api.md | 37 +++++- package-lock.json | 34 +++--- src/connect/mapStateToProps.js | 20 +++- src/connect/wrapMapToProps.js | 40 ++++++- test/components/connect.spec.js | 204 +++++++++++++++++++++++++++----- 5 files changed, 280 insertions(+), 55 deletions(-) diff --git a/docs/api.md b/docs/api.md index 5d86ae715..451b418a7 100644 --- a/docs/api.md +++ b/docs/api.md @@ -52,10 +52,12 @@ It does not modify the component class passed to it; instead, it *returns* a new #### Arguments -* [`mapStateToProps(state, [ownProps]): stateProps`] \(*Function*): If this argument is specified, the new component will subscribe to Redux store updates. This means that any time the store is updated, `mapStateToProps` will be called. The results of `mapStateToProps` must be a plain object, which will be merged into the component’s props. If you don't want to subscribe to store updates, pass `null` or `undefined` in place of `mapStateToProps`. +* [`mapStateToProps(state, [ownProps]): stateProps`] \(*Function* or *Object*): If this argument is specified, the new component will subscribe to Redux store updates. This means that any time the store is updated, `mapStateToProps` will be called. The results of `mapStateToProps` must be a plain object, which will be merged into the component’s props. If you don't want to subscribe to store updates, pass `null` or `undefined` in place of `mapStateToProps`. If your `mapStateToProps` function is declared as taking two parameters, it will be called with the store state as the first parameter and the props passed to the connected component as the second parameter, and will also be re-invoked whenever the connected component receives new props as determined by shallow equality comparisons. (The second parameter is normally referred to as `ownProps` by convention.) + If an object is passed, each function inside it is assumed to be a Redux selector (or -for advanced scenarios- a Factory function, see below). An object with the same keys, but with the result of invoking each one of its values like the normal `mapStateToProps` function, will be merged into the component’s props. + >Note: in advanced scenarios where you need more control over the rendering performance, `mapStateToProps()` can also return a function. In this case, *that* function will be used as `mapStateToProps()` for a particular component instance. This allows you to do per-instance memoization. You can refer to [#279](https://github.com/reactjs/react-redux/pull/279) and the tests it adds for more details. Most apps never need this. >The `mapStateToProps` function's first argument is the entire Redux store’s state and it returns an object to be passed as props. It is often called a **selector**. Use [reselect](https://github.com/reactjs/reselect) to efficiently compose selectors and [compute derived data](http://redux.js.org/docs/recipes/ComputingDerivedData.html). @@ -222,8 +224,8 @@ export default connect(mapStateToProps, mapDispatchToProps)(TodoApp) ```js import { addTodo, deleteTodo } from './actionCreators' -function mapStateToProps(state) { - return { todos: state.todos } +const mapStateToProps = { + todos: state => state.todos } const mapDispatchToProps = { @@ -357,6 +359,35 @@ function mapDispatchToPropsFactory(initialState, initialProps) { export default connect(mapStateToPropsFactory, mapDispatchToPropsFactory)(TodoApp) ``` +The object shorthand of `stateToProps` also accepts Factory functions + +```js +import { addTodo } from './actionCreators' + +const always = x => () => x; + +const mapStateToProps = { + anotherProperty: (initialState, initialProps) => + always(200 + initialState[initialProps.another]), + someProperty: (initialState, initilProps) => createSelector(...), + todos: state => state.todos, +} + +function mapDispatchToPropsFactory(initialState, initialProps) { + function goToSomeLink(){ + initialProps.history.push('some/link'); + } + return function(dispatch){ + return { + addTodo + } + } +} + + +export default connect(mapStateToProps, mapDispatchToPropsFactory)(TodoApp) +``` + ### `connectAdvanced(selectorFactory, [connectOptions])` diff --git a/package-lock.json b/package-lock.json index 2ec57fa7e..da3f1ce75 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2637,14 +2637,6 @@ } } }, - "string_decoder": { - "version": "1.0.1", - "bundled": true, - "dev": true, - "requires": { - "safe-buffer": "5.0.1" - } - }, "string-width": { "version": "1.0.2", "bundled": true, @@ -2655,6 +2647,14 @@ "strip-ansi": "3.0.1" } }, + "string_decoder": { + "version": "1.0.1", + "bundled": true, + "dev": true, + "requires": { + "safe-buffer": "5.0.1" + } + }, "stringstream": { "version": "0.0.5", "bundled": true, @@ -6620,15 +6620,6 @@ "integrity": "sha1-NbCYdbT/SfJqd35QmzCQoyJr8ks=", "dev": true }, - "string_decoder": { - "version": "1.0.3", - "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-1.0.3.tgz", - "integrity": "sha512-4AH6Z5fzNNBcH+6XDMfA/BTt87skxqJlO0lAh3Dker5zThcAxG6mKz+iGu308UKoPPQ8Dcqx/4JhujzltRa+hQ==", - "dev": true, - "requires": { - "safe-buffer": "5.1.1" - } - }, "string-width": { "version": "2.1.1", "resolved": "https://registry.npmjs.org/string-width/-/string-width-2.1.1.tgz", @@ -6656,6 +6647,15 @@ } } }, + "string_decoder": { + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-1.0.3.tgz", + "integrity": "sha512-4AH6Z5fzNNBcH+6XDMfA/BTt87skxqJlO0lAh3Dker5zThcAxG6mKz+iGu308UKoPPQ8Dcqx/4JhujzltRa+hQ==", + "dev": true, + "requires": { + "safe-buffer": "5.1.1" + } + }, "stringstream": { "version": "0.0.5", "resolved": "https://registry.npmjs.org/stringstream/-/stringstream-0.0.5.tgz", diff --git a/src/connect/mapStateToProps.js b/src/connect/mapStateToProps.js index 039291b0a..18d9e0cfb 100644 --- a/src/connect/mapStateToProps.js +++ b/src/connect/mapStateToProps.js @@ -1,4 +1,8 @@ -import { wrapMapToPropsConstant, wrapMapToPropsFunc } from './wrapMapToProps' +import { + wrapMapToPropsConstant, + wrapMapToPropsFunc, + wrapMapToPropsObject +} from './wrapMapToProps' export function whenMapStateToPropsIsFunction(mapStateToProps) { return (typeof mapStateToProps === 'function') @@ -6,6 +10,19 @@ export function whenMapStateToPropsIsFunction(mapStateToProps) { : undefined } +function isValidmapStateToPropsObj(mapStateToProps) { + return typeof mapStateToProps === 'object' && Object + .keys(mapStateToProps) + .map(function (key) { return mapStateToProps[key] }) + .every(function (val) { return typeof val === 'function' }) +} + +export function whenMapStateToPropsIsObject(mapStateToProps) { + return (isValidmapStateToPropsObj(mapStateToProps)) ? + wrapMapToPropsObject(mapStateToProps) : + undefined +} + export function whenMapStateToPropsIsMissing(mapStateToProps) { return (!mapStateToProps) ? wrapMapToPropsConstant(() => ({})) @@ -14,5 +31,6 @@ export function whenMapStateToPropsIsMissing(mapStateToProps) { export default [ whenMapStateToPropsIsFunction, + whenMapStateToPropsIsObject, whenMapStateToPropsIsMissing ] diff --git a/src/connect/wrapMapToProps.js b/src/connect/wrapMapToProps.js index 6b39c7961..a5a8564d1 100644 --- a/src/connect/wrapMapToProps.js +++ b/src/connect/wrapMapToProps.js @@ -35,7 +35,7 @@ export function getDependsOnOwnProps(mapToProps) { // * On first call, verifies the first result is a plain object, in order to warn // the developer that their mapToProps function is not returning a valid result. // -export function wrapMapToPropsFunc(mapToProps, methodName) { +export function wrapMapToPropsFunc(mapToProps, methodName, isObjMapToProps = false) { return function initProxySelector(dispatch, { displayName }) { const proxy = function mapToPropsProxy(stateOrDispatch, ownProps) { return proxy.dependsOnOwnProps @@ -57,7 +57,7 @@ export function wrapMapToPropsFunc(mapToProps, methodName) { props = proxy(stateOrDispatch, ownProps) } - if (process.env.NODE_ENV !== 'production') + if (process.env.NODE_ENV !== 'production' && !isObjMapToProps) verifyPlainObject(props, displayName, methodName) return props @@ -66,3 +66,39 @@ export function wrapMapToPropsFunc(mapToProps, methodName) { return proxy } } + +function mapObject (obj, mapFn) { + const result = {} + Object + .keys(obj) + .forEach(function(key) { result[key] = mapFn(obj[key], key, obj) }) + return result +} + +export function wrapMapToPropsObject (mapStateToProps) { + const wrappedMapToProps = mapObject(mapStateToProps, function (fn) { + return wrapMapToPropsFunc(fn, 'mapStateToProps', true) + }) + + const dependsOnOwnProps = Object + .keys(wrappedMapToProps) + .map(key => wrappedMapToProps[key]) + .some(getDependsOnOwnProps) + + function initObjectSelector(...initArgs) { + const initializedWraps = mapObject(wrappedMapToProps, function (fn) { + return fn(...initArgs) + }) + + function objectSelector (...selectorArgs) { + return mapObject(initializedWraps, function (fn) { + return fn(...selectorArgs) + }) + } + + objectSelector.dependsOnOwnProps = dependsOnOwnProps + return objectSelector + } + + return initObjectSelector +} diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 91ab71fb7..eaf27432e 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -9,6 +9,9 @@ import TestUtils from 'react-dom/test-utils' import { createStore } from 'redux' import { connect } from '../../src/index' +const withMapStateToProps = (...args) => test => + args.forEach((mapStateToProps) => { test(mapStateToProps) }) + describe('React', () => { describe('connect', () => { class Passthrough extends Component { @@ -93,6 +96,10 @@ describe('React', () => { expect(container.context.store).toBe(store) }) + withMapStateToProps( + ({ foo, baz }) => ({ foo, baz }), + ({ foo: state => state.foo, baz: state => state.baz }) + )(mapStateToProps => it('should pass state and props to the given component', () => { const store = createStore(() => ({ foo: 'bar', @@ -100,7 +107,7 @@ describe('React', () => { hello: 'world' })) - @connect(({ foo, baz }) => ({ foo, baz })) + @connect(mapStateToProps) class Container extends Component { render() { return @@ -120,12 +127,16 @@ describe('React', () => { expect(() => TestUtils.findRenderedComponentWithType(container, Container) ).toNotThrow() - }) + })) + withMapStateToProps( + state => ({ string: state }), + { string: state => state } + )(mapStateToProps => it('should subscribe class components to the store changes', () => { const store = createStore(stringBuilder) - @connect(state => ({ string: state }) ) + @connect(mapStateToProps) class Container extends Component { render() { return @@ -144,13 +155,17 @@ describe('React', () => { expect(stub.props.string).toBe('a') store.dispatch({ type: 'APPEND', body: 'b' }) expect(stub.props.string).toBe('ab') - }) + })) + withMapStateToProps( + state => ({ string: state }), + { string: state => state } + )(mapStateToProps => it('should subscribe pure function components to the store changes', () => { const store = createStore(stringBuilder) let Container = connect( - state => ({ string: state }) + mapStateToProps )(function Container(props) { return }) @@ -170,13 +185,17 @@ describe('React', () => { expect(stub.props.string).toBe('a') store.dispatch({ type: 'APPEND', body: 'b' }) expect(stub.props.string).toBe('ab') - }) + })) + withMapStateToProps( + state => ({ string: state }), + { string: state => state } + )(mapStateToProps => it('should retain the store\'s context', () => { const store = new ContextBoundStore(stringBuilder) let Container = connect( - state => ({ string: state }) + mapStateToProps )(function Container(props) { return }) @@ -194,12 +213,16 @@ describe('React', () => { expect(stub.props.string).toBe('') store.dispatch({ type: 'APPEND', body: 'a' }) expect(stub.props.string).toBe('a') - }) + })) + withMapStateToProps( + state => ({ string: state }), + { string: state => state } + )(mapStateToProps => it('should handle dispatches before componentDidMount', () => { const store = createStore(stringBuilder) - @connect(state => ({ string: state }) ) + @connect(mapStateToProps) class Container extends Component { componentWillMount() { store.dispatch({ type: 'APPEND', body: 'a' }) @@ -218,14 +241,18 @@ describe('React', () => { const stub = TestUtils.findRenderedComponentWithType(tree, Passthrough) expect(stub.props.string).toBe('a') - }) + })) + withMapStateToProps( + state => state, + { foo: state => state.foo } + )(mapStateToProps => it('should handle additional prop changes in addition to slice', () => { const store = createStore(() => ({ foo: 'bar' })) - @connect(state => state) + @connect(mapStateToProps) class ConnectContainer extends Component { render() { return ( @@ -263,12 +290,16 @@ describe('React', () => { const stub = TestUtils.findRenderedComponentWithType(container, Passthrough) expect(stub.props.foo).toEqual('bar') expect(stub.props.pass).toEqual('through') - }) + })) + withMapStateToProps( + state => state, + {} + )(mapStateToProps => it('should handle unexpected prop changes with forceUpdate()', () => { const store = createStore(() => ({})) - @connect(state => state) + @connect(mapStateToProps) class ConnectContainer extends Component { render() { return ( @@ -301,14 +332,18 @@ describe('React', () => { const container = TestUtils.renderIntoDocument() const stub = TestUtils.findRenderedComponentWithType(container, Passthrough) expect(stub.props.bar).toEqual('foo') - }) + })) + withMapStateToProps( + () => ({}), + {} + )(mapStateToProps => it('should remove undefined props', () => { const store = createStore(() => ({})) let props = { x: true } let container - @connect(() => ({}), () => ({})) + @connect(mapStateToProps, () => ({})) class ConnectContainer extends Component { render() { return ( @@ -344,14 +379,18 @@ describe('React', () => { expect(propsBefore.x).toEqual(true) expect('x' in propsAfter).toEqual(false, 'x prop must be removed') - }) + })) + withMapStateToProps( + () => ({}), + {} + )(mapStateToProps => it('should remove undefined props without mapDispatch', () => { const store = createStore(() => ({})) let props = { x: true } let container - @connect(() => ({})) + @connect(mapStateToProps) class ConnectContainer extends Component { render() { return ( @@ -387,14 +426,18 @@ describe('React', () => { expect(propsBefore.x).toEqual(true) expect('x' in propsAfter).toEqual(false, 'x prop must be removed') - }) + })) + withMapStateToProps( + state => state, + { foo: state => state.foo } + )(mapStateToProps => it('should ignore deep mutations in props', () => { const store = createStore(() => ({ foo: 'bar' })) - @connect(state => state) + @connect(mapStateToProps) class ConnectContainer extends Component { render() { return ( @@ -435,8 +478,12 @@ describe('React', () => { const stub = TestUtils.findRenderedComponentWithType(container, Passthrough) expect(stub.props.foo).toEqual('bar') expect(stub.props.pass).toEqual('') - }) + })) + withMapStateToProps( + state => ({ stateThing: state }), + { stateThing: state => state } + )(mapStateToProps => it('should allow for merge to incorporate state and prop changes', () => { const store = createStore(stringBuilder) @@ -448,7 +495,7 @@ describe('React', () => { } @connect( - state => ({ stateThing: state }), + mapStateToProps, dispatch => ({ doSomething: (whatever) => dispatch(doSomething(whatever)) }), @@ -492,15 +539,19 @@ describe('React', () => { tree.setState({ extra: 'Z' }) stub.props.mergedDoSomething('c') expect(stub.props.stateThing).toBe('HELLO azbzcZ') - }) + })) + withMapStateToProps( + state => state, + { foo: state => state.foo } + )(mapStateToProps => it('should merge actionProps into WrappedComponent', () => { const store = createStore(() => ({ foo: 'bar' })) @connect( - state => state, + mapStateToProps, dispatch => ({ dispatch }) ) class Container extends Component { @@ -522,7 +573,7 @@ describe('React', () => { ).toNotThrow() const decorated = TestUtils.findRenderedComponentWithType(container, Container) expect(decorated.isSubscribed()).toBe(true) - }) + })) it('should not invoke mapState when props change if it only has one argument', () => { const store = createStore(stringBuilder) @@ -854,6 +905,10 @@ describe('React', () => { runCheck(false, false, false) }) + withMapStateToProps( + state => ({ string: state }), + { string: state => state } + )(mapStateToProps => it('should unsubscribe before unmounting', () => { const store = createStore(stringBuilder) const subscribe = store.subscribe @@ -869,7 +924,7 @@ describe('React', () => { } @connect( - state => ({ string: state }), + mapStateToProps, dispatch => ({ dispatch }) ) class Container extends Component { @@ -889,7 +944,7 @@ describe('React', () => { expect(spy.calls.length).toBe(0) ReactDOM.unmountComponentAtNode(div) expect(spy.calls.length).toBe(1) - }) + })) it('should not attempt to set state after unmounting', () => { const store = createStore(stringBuilder) @@ -924,10 +979,14 @@ describe('React', () => { expect(mapStateToPropsCalls).toBe(1) }) + withMapStateToProps( + (state) => ({ hide: state === 'AB' }), + { hide: state => state === 'AB' } + )(mapStateToProps => it('should not attempt to notify unmounted child of state change', () => { const store = createStore(stringBuilder) - @connect((state) => ({ hide: state === 'AB' })) + @connect(mapStateToProps) class App extends Component { render() { return this.props.hide ? null : @@ -968,7 +1027,7 @@ describe('React', () => { } finally { ReactDOM.unmountComponentAtNode(div) } - }) + })) it('should not attempt to set state after unmounting nested components', () => { const store = createStore(() => ({})) @@ -1085,6 +1144,10 @@ describe('React', () => { expect(mapStateToPropsCalls).toBe(1) }) + withMapStateToProps( + state => ({ string: state }), + { string: state => state } + )(mapStateToProps => it('should shallowly compare the selected state to prevent unnecessary updates', () => { const store = createStore(stringBuilder) const spy = expect.createSpy(() => ({})) @@ -1094,7 +1157,7 @@ describe('React', () => { } @connect( - state => ({ string: state }), + mapStateToProps, dispatch => ({ dispatch }) ) class Container extends Component { @@ -1118,8 +1181,12 @@ describe('React', () => { expect(spy.calls.length).toBe(3) store.dispatch({ type: 'APPEND', body: '' }) expect(spy.calls.length).toBe(3) - }) + })) + withMapStateToProps( + state => ({ string: state }), + { string: state => state } + )(mapStateToProps => it('should shallowly compare the merged state to prevent unnecessary updates', () => { const store = createStore(stringBuilder) const spy = expect.createSpy(() => ({})) @@ -1129,7 +1196,7 @@ describe('React', () => { } @connect( - state => ({ string: state }), + mapStateToProps, dispatch => ({ dispatch }), (stateProps, dispatchProps, parentProps) => ({ ...dispatchProps, @@ -1206,7 +1273,7 @@ describe('React', () => { expect(spy.calls.length).toBe(5) expect(stub.props.string).toBe('a') expect(stub.props.passVal).toBe('otherval') - }) + })) it('should throw an error if a component is not passed to the function returned by connect', () => { expect(connect()).toThrow( @@ -1944,6 +2011,47 @@ describe('React', () => { expect(memoizedReturnCount).toBe(2) }) + it('should allow providing a factory function to mapStateToProps object', () => { + let updatedCount = 0 + let memoizedReturnCount = 0 + const store = createStore(() => ({ value: 1 })) + + const mapStateFactory = () => { + let lastVal, lastProp + return (state, props) => { + if (props.name === lastProp && lastVal === state.value) { + memoizedReturnCount++ + return lastProp + } + lastVal = state.value + return lastProp = props.name + } + } + + @connect({ name: mapStateFactory }) + class Container extends Component { + componentWillUpdate() { + updatedCount++ + } + render() { + return + } + } + + TestUtils.renderIntoDocument( + +
+ + +
+ + ) + + store.dispatch({ type: 'test' }) + expect(updatedCount).toBe(0) + expect(memoizedReturnCount).toBe(2) + }) + it('should allow a mapStateToProps factory consuming just state to return a function that gets ownProps', () => { const store = createStore(() => ({ value: 1 })) @@ -2312,5 +2420,37 @@ describe('React', () => { expect(mapStateToPropsC.calls.length).toBe(2) expect(mapStateToPropsD.calls.length).toBe(2) }) + + it('should allow for objects in the `mapState` and `mapDispatch` params', () => { + const store = createStore((state = {foo: 1, bar: 'test'}, action) => ( + action.type === 'INC' ? {...state, foo: state.foo + 1} : + action.type === 'BAR' ? {...state, bar: action.payload} : state + )) + + const prop = p => obj => obj[p]; + const selectors = { + foo: prop('foo'), + bar: prop('bar'), + } + + const raiseAction = type => payload => ({type, payload}); + const actionCreators = { + onInc: raiseAction('INC'), + onChangeBar: raiseAction('BAR'), + } + + const A = connect(selectors, actionCreators)(Passthrough); + const tree = TestUtils.renderIntoDocument( + + ); + + const stub = TestUtils.findRenderedComponentWithType(tree, Passthrough) + expect(stub.props.foo).toBe(1) + expect(stub.props.bar).toBe('test') + stub.props.onChangeBar('test2') + expect(stub.props.bar).toBe('test2') + stub.props.onInc() + expect(stub.props.foo).toBe(2) + }) }) })