Allow nested reducer map in handleActions #218

zcei · 2017-06-25T01:52:54Z

Fixes #215

Introduces flattenReducerMap with same behavior as flattenActionMap - except a guard, so that generator alike { next, throw } objects are treated as a reducer.

As a side-note I would recommend to distinguish between reducerMap for a single reducer (the generator alike interface in handleAction) and a namespaced reducerMap in handleActions.
Can't come up with something better than reducerObject / namedReducers myself though.

yangmillstheory · 2017-06-25T04:23:19Z

Can we get the build passing before review? Thanks @zcei.

zcei · 2017-06-25T13:45:17Z

@yangmillstheory oops, Node 4 has no includes - uses the lodash equivalent now

yangmillstheory · 2017-06-25T17:35:31Z

Thanks. Can we include some README documentation?

yangmillstheory · 2017-06-27T05:13:32Z

Thanks. I will get to this; but to set expectations, I'm usually busy with other things during weeknights. It might be the case that the earliest that I can review this is this weekend.

yangmillstheory · 2017-07-01T17:28:23Z

src/__tests__/handleActions-test.js

+          next: ({ counter, message }, { payload }) => ({
+            counter,
+            message: `${message}---${payload.message}`
+          })


Can we add a throw handler and a test for that code path?

yangmillstheory · 2017-07-01T17:30:27Z

src/handleActions.js


 export default function handleActions(handlers, defaultState) {
  invariant(
    isPlainObject(handlers),
    'Expected handlers to be an plain object.'
  );
-  const reducers = ownKeys(handlers).map(type =>
+  const flatHandlers = flattenReducerMap(handlers);


Can we call this flattenedReducerMap?

yangmillstheory · 2017-07-01T17:31:16Z

src/namespaceActions.js


 const defaultNamespace = '/';

-function flattenActionMap(
-  actionMap,
+function hasGeneratorInterface(handler) {


Maybe put this in a separate module, like ownKeys, camelCase, and arrayToObject?

yangmillstheory · 2017-07-01T17:35:06Z

src/namespaceActions.js

+function hasGeneratorInterface(handler) {
+  const generatorFnNames = ['next', 'throw'];
+  const keys = Object.getOwnPropertyNames(handler);
+  const onlyInterfaceFns = keys.every((fnName) => includes(generatorFnNames, fnName));


Can just say, using https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/every,

const hasOnlyInterfaceNames = Object.getOwnPropertyNames(handler).every(ownPropertyName => ownPropertyName === 'next' || ownPropertyName === 'throw)

yangmillstheory · 2017-07-01T17:39:48Z

README.md

@@ -207,6 +207,8 @@ import { handleActions } from 'redux-actions';

 Creates multiple reducers using `handleAction()` and combines them into a single reducer that handles multiple actions. Accepts a map where the keys are passed as the first parameter to `handleAction()` (the action type), and the values are passed as the second parameter (either a reducer or reducer map). The map must not be empty.

+If `reducerMap` has a recursive structure, its leaves are used as reducers, and the action type for each leaf is the path to that leaf. If a node's only children are `next()` and `throw()`, the node will be treated as a reducer.
+


For leaves I think we should also support:

array form [payload, meta]

function form (this will be used as the reducer)

undefined or null (reducer just returns previous state)

This will keep compatibility with the previous API.

array form [payload, meta]

Isn't this exclusively for action creators? 🤔
I think undefined, null & 'fn' as leaves are all possible, as they're just passed to handleAction.

I'll add tests for this!

Sorry, you're right about the action creators. Thanks for correcting.

yangmillstheory · 2017-07-01T17:40:11Z

src/__tests__/handleActions-test.js

+            counter,
+            message: `${message}---${payload.message}`
+          })
+        }


Also, we should implement and add tests for https://github.com/acdlite/redux-actions/pull/218/files#r125166038.

yangmillstheory · 2017-07-01T17:40:46Z

src/namespaceActions.js

-  actionMap,
+function hasGeneratorInterface(handler) {
+  const generatorFnNames = ['next', 'throw'];
+  const keys = Object.getOwnPropertyNames(handler);


Why not use ownKeys?

yangmillstheory · 2017-07-01T17:41:34Z

src/namespaceActions.js

+  return partialFlatMap;
+};
+
+const flattenActionMap = flattenBy((node) => isPlainObject(node));


Can we just pass in the predicate?

flattenBy(isPlainObject)

Sure, just wanted to keep it consistent with the line below

yangmillstheory · 2017-07-01T17:41:52Z

src/namespaceActions.js

+};
+
+const flattenActionMap = flattenBy((node) => isPlainObject(node));
+const flattenReducerMap = flattenBy((node) => isPlainObject(node) && !hasGeneratorInterface(node));


See https://github.com/acdlite/redux-actions/pull/218/files#r125166038, I think this will have to change.

yangmillstheory · 2017-07-01T17:42:52Z

This is great, thanks! I should have started a review to not e-mail spam you, sorry. I have some comments and questions; let me know what you think.

yangmillstheory · 2017-07-02T19:22:27Z

Thanks @zcei, this looks good ~~now~~; is there anything else you want to add?

yangmillstheory · 2017-07-02T19:32:31Z

src/handleActions.js

@@ -3,16 +3,18 @@ import reduceReducers from 'reduce-reducers';
 import invariant from 'invariant';
 import handleAction from './handleAction';
 import ownKeys from './ownKeys';
+import { flattenReducerMap } from './namespaceActions';

 export default function handleActions(handlers, defaultState) {


I'm wondering if this should now take a namespace character like createActions does...thoughts? If so, it should probably be documented and tested.

Was thinking about it earlier, but left it out, as it made readability worse, as you now have to objects after each other and on a quick glimpse you might think { namespace } would be default state.

Just added tests for it, seems to work "out of the box" by just passing the (maybe undefined) namespace property through.

I'm guessing most people wouldn't even want to use the custom namespace property anyway. We can add later if requested.

zcei · 2017-07-02T20:30:43Z

src/namespaceActions.js

@@ -32,7 +32,9 @@ const flattenWhenNode = predicate => function flatten(
 };

 const flattenActionMap = flattenWhenNode(isPlainObject);
-const flattenReducerMap = flattenWhenNode(node => isPlainObject(node) && !hasGeneratorInterface(node));
+const flattenReducerMap = flattenWhenNode(
+  node => isPlainObject(node) && !hasGeneratorInterface(node)


@yangmillstheory the renaming to flattenWhenNode broke the tests because line was too long.
Is this notation fine, or do you want to have only the expression in separate line?

yangmillstheory · 2017-07-02T20:42:29Z

Looks good to me 👍🏿

On Sun, Jul 2, 2017 at 1:30 PM Stephan Schneider ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/namespaceActions.js <#218 (comment)>: > @@ -32,7 +32,9 @@ const flattenWhenNode = predicate => function flatten( }; const flattenActionMap = flattenWhenNode(isPlainObject); -const flattenReducerMap = flattenWhenNode(node => isPlainObject(node) && !hasGeneratorInterface(node)); +const flattenReducerMap = flattenWhenNode( + node => isPlainObject(node) && !hasGeneratorInterface(node) @yangmillstheory <https://github.com/yangmillstheory> the renaming to flattenWhenNode broke the tests because line was too long. Is this notation fine, or do you want to have only the expression in separate line? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#218 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACmkd9a5qsE7ua0gdTqiYM1TCttibqkeks5sJ_30gaJpZM4OEfTX> .

-- Victor Alvarez

yangmillstheory · 2017-07-02T20:44:49Z

I'm at the beach 🏖, will merge this when I get back home later. Thanks @zcei!

yangmillstheory · 2017-07-02T21:50:30Z

福 ~/c/p/redux-actions (e170d6a)|master✓
±  [i]: npm publish
npm WARN prepublish-on-install As of npm@5, `prepublish` scripts will run only for `npm publish`.
npm WARN prepublish-on-install (In npm@4 and previous versions, it also runs for `npm install`.)
npm WARN prepublish-on-install See the deprecation note in `npm help scripts` for more information.

> redux-actions@2.2.0 prepublish /Users/VictorAlvarez/code/private/redux-actions
> npm run lint && npm run test && npm run build


> redux-actions@2.2.0 lint /Users/VictorAlvarez/code/private/redux-actions
> esw build src webpack.config --color

✓ Clean (2:49:45 PM)

> redux-actions@2.2.0 test /Users/VictorAlvarez/code/private/redux-actions
> mocha --compilers js:babel-register src/**/*-test.js



  camelCase
    ✓ should camel case a conventional action type
    ✓ should include forward slashes in words
    ✓ should do nothing to an already camel-cased action type

  combineActions
    ✓ should throw an error if any action is not a function or string
    ✓ should accept action creators and action type strings
    ✓ should return a stringifiable object

  createAction()
    resulting action creator
      ✓ returns a valid FSA
      ✓ uses return value as payload
      ✓ should throw an error if payloadCreator is not a function, undefined, null
      ✓ uses identity function if payloadCreator is undefined
      ✓ uses identity function if payloadCreator is null
      ✓ accepts a second parameter for adding meta to object
      ✓ sets error to true if payload is an Error object
      ✓ sets error to true if payload is an Error object and meta is provided
      ✓ sets payload only when defined
      ✓ bypasses payloadCreator if payload is an Error object
      ✓ set error to true if payloadCreator return an Error object

  createActions
    ✓ should throw an error when given arguments that contain a non-string
    ✓ should throw an error when given bad payload creators
    ✓ should throw an error when given a bad payload or meta creator in array form
    ✓ should throw an error when no meta creator is given in array form
    ✓ should return a map of camel-cased action types to action creators
    ✓ should honor special delimiters in action types
    ✓ should use the identity if the payload creator is undefined in array form
    ✓ should use the identity and meta creators in array form
    ✓ should use identity payload creators for trailing string action types
    ✓ should create actions from an action map and action types
    ✓ should create actions from a namespaced action map
    ✓ should create namespaced actions with payload creators in array form
    ✓ should create namespaced actions with a chosen namespace string

  handleAction()
    ✓ should throw an error if the reducer is the wrong type
    ✓ uses the identity if the specified reducer is undefined
    single handler form
      ✓ should throw an error if defaultState is not specified
      resulting reducer
        ✓ returns previous state if type does not match
        ✓ returns default state if type does not match
        ✓ accepts single function as handler
        ✓ accepts action function as action type
        ✓ accepts a default state used when the previous state is undefined
        ✓ should work with createActions action creators
        ✓ should not throw and return state when action is non-FSA
    map of handlers form
      ✓ should throw an error if defaultState is not specified
      resulting reducer
        ✓ returns previous state if type does not match
        ✓ uses `next()` if action does not represent an error
        ✓ uses `throw()` if action represents an error
        ✓ returns previous state if matching handler is not function
    with combined actions
      ✓ should handle combined actions in reducer form
      ✓ should handle combined actions in next/throw form
      ✓ should handle combined error actions
      ✓ should return previous state if action is not one of the combined actions
      ✓ should use the default state if the initial state is undefined
      ✓ should handle combined actions with symbols

  handleActions
    ✓ should throw an error when defaultState is not defined
    ✓ should throw an error when defaultState is not defined for combinedActions
    ✓ create a single handler from a map of multiple action handlers
    ✓ works with symbol action types
    ✓ accepts a default state used when previous state is undefined
    ✓ accepts action function as action type
    ✓ should accept combined actions as action types in single reducer form
    ✓ should accept combined actions as action types in the next/throw form
    ✓ should work with createActions action creators
    ✓ should work with namespaced actions
    ✓ should return default state with empty handlers and undefined previous state
    ✓ should return previous defined state with empty handlers
    ✓ should throw an error if handlers object has the wrong type
    ✓ should work with nested reducerMap
    ✓ should work with nested reducerMap and namespace
    ✓ should work with nested reducerMap and identity handlers

  namespacing actions
    flattenActionMap
      ✓ should flatten an action map with the default namespacer
      ✓ should do nothing to an already flattened map
      ✓ should be case-sensitive
      ✓ should use a custom namespace string
    unflattenActionCreators
      ✓ should unflatten a flattened action map and camel-case keys
      ✓ should unflatten a flattened action map with custom namespace


  73 passing (416ms)


> redux-actions@2.2.0 build /Users/VictorAlvarez/code/private/redux-actions
> npm run clean && npm run build:es && npm run build:commonjs && npm run build:umd && npm run build:umd:min


> redux-actions@2.2.0 clean /Users/VictorAlvarez/code/private/redux-actions
> rimraf lib es


> redux-actions@2.2.0 build:es /Users/VictorAlvarez/code/private/redux-actions
> cross-env BABEL_ENV=es babel src --out-dir es --ignore *-test.js

src/arrayToObject.js -> es/arrayToObject.js
src/camelCase.js -> es/camelCase.js
src/combineActions.js -> es/combineActions.js
src/createAction.js -> es/createAction.js
src/createActions.js -> es/createActions.js
src/handleAction.js -> es/handleAction.js
src/handleActions.js -> es/handleActions.js
src/hasGeneratorInterface.js -> es/hasGeneratorInterface.js
src/index.js -> es/index.js
src/namespaceActions.js -> es/namespaceActions.js
src/ownKeys.js -> es/ownKeys.js

> redux-actions@2.2.0 build:commonjs /Users/VictorAlvarez/code/private/redux-actions
> babel src --out-dir lib --ignore *-test.js

src/arrayToObject.js -> lib/arrayToObject.js
src/camelCase.js -> lib/camelCase.js
src/combineActions.js -> lib/combineActions.js
src/createAction.js -> lib/createAction.js
src/createActions.js -> lib/createActions.js
src/handleAction.js -> lib/handleAction.js
src/handleActions.js -> lib/handleActions.js
src/hasGeneratorInterface.js -> lib/hasGeneratorInterface.js
src/index.js -> lib/index.js
src/namespaceActions.js -> lib/namespaceActions.js
src/ownKeys.js -> lib/ownKeys.js

> redux-actions@2.2.0 build:umd /Users/VictorAlvarez/code/private/redux-actions
> cross-env NODE_ENV=development webpack

Hash: b013f89b0ede1e8a1404
Version: webpack 1.13.1
Time: 1126ms
           Asset     Size  Chunks             Chunk Names
redux-actions.js  72.4 kB       0  [emitted]  main
    + 71 hidden modules

> redux-actions@2.2.0 build:umd:min /Users/VictorAlvarez/code/private/redux-actions
> cross-env NODE_ENV=production webpack

Hash: 0e96ae086fa280e4ef7e
Version: webpack 1.13.1
Time: 1665ms
               Asset     Size  Chunks             Chunk Names
redux-actions.min.js  19.6 kB       0  [emitted]  main
    + 71 hidden modules
+ redux-actions@2.2.0

yangmillstheory · 2017-07-02T21:52:32Z

https://github.com/acdlite/redux-actions/releases/tag/v2.2.0

yangmillstheory self-requested a review June 25, 2017 04:22

Allow nested reducer map in handleActions

a8c6add

zcei force-pushed the feat/nested-reducer-map branch from b4d1421 to a8c6add Compare June 25, 2017 13:38

yangmillstheory self-assigned this Jun 25, 2017

Update documentation for handleActions

e3df1ae

yangmillstheory reviewed Jul 1, 2017

View reviewed changes

Update README.md

880bef8

yangmillstheory reviewed Jul 1, 2017

View reviewed changes

fixup! Allow nested reducer map in handleActions

75e51aa

zcei force-pushed the feat/nested-reducer-map branch from 40cb54d to 75e51aa Compare July 2, 2017 18:40

minor naming

9ed6baf

yangmillstheory reviewed Jul 2, 2017

View reviewed changes

Allow namespace in handleActions

b92c778

zcei commented Jul 2, 2017

View reviewed changes

Update README.md

8299102

yangmillstheory merged commit a71aa96 into redux-utilities:master Jul 2, 2017

zcei deleted the feat/nested-reducer-map branch July 2, 2017 21:41

lsanwick mentioned this pull request Nov 13, 2017

[redux-actions] Update definition to support nested reducer maps DefinitelyTyped/DefinitelyTyped#21498

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow nested reducer map in handleActions #218

Allow nested reducer map in handleActions #218

zcei commented Jun 25, 2017

yangmillstheory commented Jun 25, 2017

zcei commented Jun 25, 2017

yangmillstheory commented Jun 25, 2017

yangmillstheory commented Jun 27, 2017

yangmillstheory Jul 1, 2017

yangmillstheory Jul 1, 2017

yangmillstheory Jul 1, 2017

yangmillstheory Jul 1, 2017

yangmillstheory Jul 1, 2017

zcei Jul 2, 2017

yangmillstheory Jul 2, 2017

yangmillstheory Jul 1, 2017

yangmillstheory Jul 1, 2017

yangmillstheory Jul 1, 2017

zcei Jul 2, 2017

yangmillstheory Jul 1, 2017 •

edited

Loading

yangmillstheory commented Jul 1, 2017

yangmillstheory commented Jul 2, 2017 •

edited

Loading

yangmillstheory Jul 2, 2017 •

edited

Loading

zcei Jul 2, 2017

yangmillstheory Jul 2, 2017

zcei Jul 2, 2017

yangmillstheory commented Jul 2, 2017 via email

yangmillstheory commented Jul 2, 2017

yangmillstheory commented Jul 2, 2017

yangmillstheory commented Jul 2, 2017

		@@ -207,6 +207,8 @@ import { handleActions } from 'redux-actions';

		Creates multiple reducers using `handleAction()` and combines them into a single reducer that handles multiple actions. Accepts a map where the keys are passed as the first parameter to `handleAction()` (the action type), and the values are passed as the second parameter (either a reducer or reducer map). The map must not be empty.

		If `reducerMap` has a recursive structure, its leaves are used as reducers, and the action type for each leaf is the path to that leaf. If a node's only children are `next()` and `throw()`, the node will be treated as a reducer.

Allow nested reducer map in handleActions #218

Allow nested reducer map in handleActions #218

Conversation

zcei commented Jun 25, 2017

yangmillstheory commented Jun 25, 2017

zcei commented Jun 25, 2017

yangmillstheory commented Jun 25, 2017

yangmillstheory commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangmillstheory Jul 1, 2017 • edited Loading

Choose a reason for hiding this comment

yangmillstheory commented Jul 1, 2017

yangmillstheory commented Jul 2, 2017 • edited Loading

yangmillstheory Jul 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangmillstheory commented Jul 2, 2017 via email

yangmillstheory commented Jul 2, 2017

yangmillstheory commented Jul 2, 2017

yangmillstheory commented Jul 2, 2017

yangmillstheory Jul 1, 2017 •

edited

Loading

yangmillstheory commented Jul 2, 2017 •

edited

Loading

yangmillstheory Jul 2, 2017 •

edited

Loading