From dde703271b864d41561e1c174ecca1ab1e563a01 Mon Sep 17 00:00:00 2001 From: Neal Granger Date: Thu, 22 Dec 2016 19:03:09 -0800 Subject: [PATCH] Simplify actions and selectors. Remove the prop merging magic from the selectors. The original intention was to allow multiple instances of a component with a given id to exist, but to have them collapsed down and merged in the selector. This would have been useful for a component that was the product of props from a route component _and_ a dispatched component. However the complexity this added far outweighed the benefit, and the behavior was not intuitive. The new version of `addComponent` no longer has the option for a custom id, and will not result in props merging with existing components. A new `setComponent` action is used when the id must be explicitly specified. If an existing component with the given id exists, it is replaced rather than merged on to. What does remain of the component merging logic is a new `updateComponent` action that can be used to update the props of an existing component. Props dispatched in the `updateComponent` _are_ merged into the existing props of the component with the matching id. An `update` function is also now included alongside the `remove` function on each component returned through the connector. This offers an interface for updating the "state" of the component where `update` is used like react `setState`. --- package.json | 1 - src/action.js | 19 +++++++++-- src/connect.js | 89 +++++++++++++------------------------------------ src/reducer.js | 29 ++++++++++++---- src/selector.js | 52 +++++++++-------------------- 5 files changed, 77 insertions(+), 113 deletions(-) diff --git a/package.json b/package.json index b5a03d2..a81dbca 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,6 @@ }, "license": "CC0-1.0", "dependencies": { - "core-js": "^2.4.1", "cuid": "^1.3.8", "fbjs": "^0.8.4", "hoist-non-react-statics": "^1.2.0" diff --git a/src/action.js b/src/action.js index c4df23b..a94454c 100644 --- a/src/action.js +++ b/src/action.js @@ -1,15 +1,30 @@ import cuid from 'cuid'; export const ADD_COMPONENT = '@@relocation/ADD_COMPONENT'; +export const SET_COMPONENT = '@@relocation/SET_COMPONENT'; +export const UPDATE_COMPONENT = '@@relocation/UPDATE_COMPONENT'; export const REMOVE_COMPONENT = '@@relocation/REMOVE_COMPONENT'; export const SET_ROUTE_COMPONENTS = '@@relocation/SET_ROUTE_COMPONENTS'; -export const addComponent = ({type, props, id = cuid()}) => ({ +export const addComponent = (type, props) => ({ type: ADD_COMPONENT, + payload: {id: cuid(), type, props}, +}); + +export const setComponent = (type, id = type, props) => ({ + type: SET_COMPONENT, payload: {id, type, props}, }); -export const removeComponent = (id) => ({type: REMOVE_COMPONENT, payload: id}); +export const updateComponent = (id, props) => ({ + type: UPDATE_COMPONENT, + payload: {id, props}, +}); + +export const removeComponent = (id) => ({ + type: REMOVE_COMPONENT, + payload: {id}, +}); export const setRouteComponents = (components) => ({ type: SET_ROUTE_COMPONENTS, diff --git a/src/connect.js b/src/connect.js index 6e2f5e7..4c84086 100644 --- a/src/connect.js +++ b/src/connect.js @@ -2,8 +2,8 @@ import {Component, createElement, PropTypes} from 'react'; import hoistStatics from 'hoist-non-react-statics'; import {connect} from 'react-redux'; -import {getMergedComponents} from './selector'; -import {removeComponent} from './action'; +import {getComponents} from './selector'; +import {removeComponent, updateComponent} from './action'; import {componentsShape, renderMapShape, getDisplayName} from './util'; /** @@ -43,80 +43,36 @@ export default ({scope, ...defaultProps} = {}) => (WrappedComponent) => { render() { const {components, renderMap} = this.props.___relocationState___; - const {removeComponent} = this.props.___relocationDispatch___; + const { + removeComponent, + updateComponent, + } = this.props.___relocationDispatch___; const inRenderMap = (component) => typeof renderMap[component.type] === 'function'; - const assignRender = (component) => ({ - ...component, - render: renderMap[component.type], - }); - - const assignScope = (component) => ({...component, scope}); - - const assignRemoveHandler = (component) => { - let removeHandler = null; - - if (typeof component.remove === 'function') { - // The component object remove property is already a function. - // We don't want to override this behavior. - removeHandler = component.remove; - } else if (component.remove === undefined || component.remove) { - // The component object does not have a `remove` property, or it has - // a truthy value that is not a function. Either case indicates that - // it should use the default remove handler. - removeHandler = () => removeComponent(component.id); - } - - let pathRemoveHandler = null; - - if (typeof component.removePath === 'string') { - // Create a function that will change the history state when removing - // the component. - pathRemoveHandler = () => this.navigateToPath(component.removePath); - } - - if (pathRemoveHandler && removeHandler) { - // A remove handler function and a - return { - ...component, - remove: () => { - pathRemoveHandler(); - return removeHandler(); - }, - }; - } - - if (pathRemoveHandler && !removeHandler) { - return { - ...component, - remove: pathRemoveHandler, - }; - } - - if (!pathRemoveHandler && removeHandler !== component.remove) { - return { - ...component, - remove: removeHandler, - }; + const assign = (component) => { + const result = { + ...component, + render: renderMap[component.type], + update: (props) => updateComponent(component.id, props), + remove: typeof component.removePath === 'string' + ? () => this.navigateToPath(component.removePath) + : () => removeComponent(component.id), + }; + + if (scope) { + result.scope = scope; } - // `!pathRemoveHandler && removeHandler === component.remove` is true. - // This means `remove` was set and `removePath` was not set on the - // component object. No modification is necessary. - return component; + return result; }; const currentComponents = components // Remove components not included in the render function map. .filter(inRenderMap) - // Assign render functions. - .map(assignRender) - // Assign scope, if configured. - .map(scope ? assignScope : (component) => component) - // Assign remove handler functions. - .map(assignRemoveHandler); + // Assign render update and remove functions and scope if it is defined. + .map(assign); /* eslint-disable no-unused-vars */ const { @@ -155,7 +111,7 @@ export default ({scope, ...defaultProps} = {}) => (WrappedComponent) => { // Put everything in a ___relocationState___ namespace to avoid possible // conflict with existing props. ___relocationState___: { - components: getMergedComponents(state, selectorProps), + components: getComponents(state, selectorProps), renderMap: components, }, }; @@ -166,6 +122,7 @@ export default ({scope, ...defaultProps} = {}) => (WrappedComponent) => { // possible conflict with existing props. ___relocationDispatch___: { removeComponent: (id) => dispatch(removeComponent(id)), + updateComponent: (id, props) => dispatch(updateComponent(id, props)), }, }); diff --git a/src/reducer.js b/src/reducer.js index 8f5ca5d..5315bef 100644 --- a/src/reducer.js +++ b/src/reducer.js @@ -1,18 +1,33 @@ import {combineReducers} from 'redux'; import { - REMOVE_COMPONENT, ADD_COMPONENT, + SET_COMPONENT, + UPDATE_COMPONENT, + REMOVE_COMPONENT, SET_ROUTE_COMPONENTS, } from './action'; -const createReducer = (type, initial) => - (state = initial, action) => action.type === type ? action.payload : state; - export default combineReducers({ components: (state = [], action) => ({ - [ADD_COMPONENT]: (state, {payload}) => [...state, payload], + [ADD_COMPONENT]: (state, {payload}) => + [...state, payload], + + [SET_COMPONENT]: (state, {payload}) => + [...state.filter((item) => item.id !== payload.id), payload], + + [UPDATE_COMPONENT]: (state, {payload}) => + state.map((item) => item.id === payload.id + ? {...item, props: {...item.props, ...payload.props}} + : item + ), + [REMOVE_COMPONENT]: (state, {payload}) => - state.filter((item) => item.id !== payload), + state.filter((item) => item.id !== payload.id), + }[action.type] || (() => state))(state, action), - routeComponents: createReducer(SET_ROUTE_COMPONENTS, []), + + routeComponents: (state = [], action) => + action.type === SET_ROUTE_COMPONENTS + ? action.payload + : state, }); diff --git a/src/selector.js b/src/selector.js index e92094d..f73c302 100644 --- a/src/selector.js +++ b/src/selector.js @@ -1,38 +1,16 @@ -import findIndex from 'core-js/library/fn/array/find-index'; - export const getRelocation = (state, props) => - (props && props.getRelocationState) ? - props.getRelocationState(state, props) : - state.relocation; - -export const getComponents = (state, props) => - getRelocation(state, props).components; - -export const getRouteComponents = (state, props) => - getRelocation(state, props).routeComponents; - -export const getMergedComponents = (state, props) => - [...getRouteComponents(state, props), ...getComponents(state, props)]. - reduce((components, component) => { - const index = findIndex(components, ({id}) => id === component.id); - - if (index === -1) { - components.push(component); - return components; - } - - const existing = components[index]; - - components[index] = { - ...existing, - ...component, - ...existing.props && component.props ? { - props: { - ...existing.props, - ...component.props, - }, - } : null, - }; - - return components; - }, []); + (props && props.getRelocationState) + ? props.getRelocationState(state, props) + : state.relocation; + +export const getComponents = (state, props) => { + const routeComponents = getRelocation(state, props).routeComponents; + const components = getRelocation(state, props).components; + + // Concat components and route components uniquely by id. + return routeComponents + .filter(({id: routeId}) => + !components.filter(({id}) => id === routeId).length + ) + .concat(components); +};