Simplified implementation of handleAction #97

Lucretiel · 2016-06-28T21:40:27Z

Moved the function-or-object check on the reducers parameter to outside the closure function
Removed the use of magic strings
Removed the mutation of the passed-in reducers argument

- Moved the function-or-object check on the `reducers` parameter to outside the closure function - Removed the use of magic strings - Removed the mutation of the passed-in `reducers` argument

yangmillstheory · 2016-06-30T20:22:32Z

src/handleAction.js

    : type;

+  const nextReducer = isFunction(reducers) ? reducers : reducers.next;
+  const throwReducer = isFunction(reducers) ? reducers : reducers.throw;


What do you think about using multiple assignment to DRY things up here?

In the Babel REPL with our .babelrc:

let foo let bar const o = { foo: 'baz', bar: 'qux' } if (flag) { [ foo, bar ] = [ o.foo, o.bar ] } else { [ foo, bar ] = [ o, o ] } // flag === true => baz qux // flag === false => {"foo":"baz","bar":"qux"} {"foo":"baz","bar":"qux"} console.log(foo, bar)

so we could be more DRY here with

let nextReducer = throwReducer = reducers; if (!isFunction(reducers)) { [ nextReducer, throwReducer ] = [ reducers.next, reducers.throw ] }

Oh that's perfect. I was actually wishing I could do that but I couldn't work out a clean way to do it... I didn't know you could do [a, b] = ... in addition to {a, b} = ... Will update, probably with:

const [nextReducer, throwReducer] = isFunction(reducers) ? [reducers, reducers] : [reducers.next, reducers.throw]

yangmillstheory · 2016-06-30T20:54:59Z

src/handleAction.js

+    const reducer = action.error === true ? throwReducer : nextReducer;

    return isFunction(reducer)
      ? reducer(state, action)


I was thinking this might not be necessary with your changes, but I guess either reducers.throw or reducers.next might not be functions.

I was wondering if we should fail earlier if !isFunction(reducers) and reducers is not an object with function properties next and throw (not necessarily suggesting it be implemented in this PR), since that's the documented API. It would eliminate the need for more downstream checks.

I can add that; this PR actually makes that more natural anyway. Check to make sure that nextReducer and throwReducer !== undefined, and throw otherwise?

We should assume that doing something like this is an error, right?

const reducer = handleAction(myAction, { next: ... // No throw reducer })

I think this is a case of a breaking change, but it's also the library doing what the documentation says

If a single reducer is passed, it is used to handle both normal actions and failed actions. (A failed action is analogous to a rejected promise.) You can use this form if you know a certain type of action will never fail, like the increment example above.

Otherwise, you can specify separate reducers for next() and throw(). This API is inspired by the ES6 generator interface.

So it seems it doesn't make sense to not pass throw and just pass next in the object form, since the former case would handle that. This would break any code that was passing just next and not throw.

For your implementation, why not do an isFunction check (for both next and throw) instead of checking undefined?

Also, you could ensure that you can dot into reducers first before checking the types of reducers.throw and reducers.next. This would have thrown errors previously, so it's not a breaking change, we'd just be being more explicit about why it failed here.

Another positive effect of this change would be that handleAction would fail earlier at the definition level, and not the invocation level. Failing earlier is 👍 .

Added a patch; you can tell me what you think. We're definitely into a place where my grasp of precise javascript semantics is pretty fuzzy.

As for the breaking change thing– I had the same question about a python library a long time ago. It turns out that the documentation is really the only source of truth; changing behavior that is undocumented, or errors thrown because of use the library incorrectly or in an undocumented way, isn't considered making a breaking change; it's either a bug fix or a new feature, depending on the change. I'll leave it up to you guys whether you think it counts in this case.

See also the question I asked here: semver/semver#248. Basically, I was worried that adding new arguments to a python function would constitute a breaking change, because people who were calling the function with those arguments in a prior version and seeing a TypeError wouldn't see that anymore.

Note that these checks would make a use case of factoring out...

That reminds me– are we trying to keep this project minimally dependent on others? Because lodash has good implementations of both isFunction and ownKeys we could use, and has modular, one-function libraries we can depend on to minimize how much gets pulled in.

Yeah, it would seem that it's explicitly trying to not take dependencies. There's also https://github.com/acdlite/redux-actions/blob/master/src/createAction.js#L1 which is included in _.

IMO we could quite easily factor out isFunction though, especially since it's already in the codebase. I think this would be more preferable than taking a dependency (which is a choice that I'm not qualified to encourage here).

That's fine; it runs against my personal grain, but it's a perfectly valid standpoint. Doing that refactoring seems outside the scope of this PR, though, so we'll do it after this is resolved.

On my fork, I've started using lodash micro-packages.

Since I'm making faster progress there, I'm considering renaming it and publishing to NPM.

yangmillstheory · 2016-06-30T20:55:14Z

👍 nice improvement of hygiene. ping @timche

Lucretiel · 2016-06-30T21:05:02Z

src/handleAction.js

-
-    // Otherwise, assume an action map was passed
-    const reducer = reducers[handlerKey];
+    const reducer = action.error === true ? throwReducer : nextReducer;


Just a quick note: I double checked the FSA spec, and confirmed that this === true behavior is in fact the most correct (as opposed to doing something like action.error ? throwReducer : nextReducer):

If error has any other value besides true, including undefined and null, the action MUST NOT be interpreted as an error.

So 👍 to whoever got that right.

Cool, I was going to ask why we weren't checking truthiness, but that settles that.

yangmillstheory · 2016-06-30T21:35:52Z

src/handleAction.js

  if (nextReducer === undefined || throwReducer === undefined) {
-    throw TypeError("reducers argument must be either a function or an object of shape {next, throw}");
+    throw new TypeError(
+      'reducers argument must be either a function or an object of shape {next, throw}'


reducers must be a reducer function or an object with shape {next, throw} where 'next' and 'throw' are reducer functions

I know you were broken with ESLint for long lines, so you could use string concatenation:

throw new TypeError( 'reducers must be a reducer function or an object with ' + 'shape {next, throw} where "next" and "throw" are reducer functions' )

Lucretiel · 2016-06-30T21:41:11Z

Hmmm. So the tests that are failing are, in fact, using the partial object style:

it('uses `next()` if action does not represent an error', () => {
  const reducer = handleAction(type, {
    next: (state, action) => ({
      counter: state.counter + action.payload
    })
  });
  expect(reducer(prevState, { type, payload: 7 }))
    .to.deep.equal({
      counter: 10
    });
});

The documentation makes no mention of this style being valid, and in practice I can't think of any reasonable behavior for it. Silently skipping actions that are (or aren't) errors seems like an accident waiting to happen, and we already have a syntax for a unified reducer. It's my belief, therefore, that this test and its ilk are buggy, and there should at least be a placeholder reducer added to make it pass. However, I'll await advice from the rest of you before making this change.

If you agree and I fix this test, I'll add an extra pair of tests that ensure that it does throw when either key is missing.

yangmillstheory · 2016-06-30T21:46:13Z

Looked at the Travis failures and I agree that placeholder reducers (https://github.com/acdlite/redux-actions/blob/master/src/createAction.js#L1) should be added to those tests. Thinking of tests as a form of API documentation, this makes even more sense. Does adding them make the tests pass?

Lucretiel · 2016-06-30T22:13:19Z

Oh, dear. It actually appears that, based on the tests (thought not the written documentation), the defined behavior is in fact to do nothing if no function is attached to either the next or throw keys:

https://github.com/acdlite/redux-actions/blob/master/src/__tests__/handleAction-test.js#L87-L92

There is no similar test in place for passing a null as the reducer argument itself. This, in fact, was changed from the prior behavior, which DID raise an exception if a function wasn't passed in: e659159#diff-13523acaa823765ceef717519f2dcc3dL75

It's not clear to me if this change was made as an explicit API change, or if it was just changed with some subtle javascript thing. Pinging @acdlite for his input, as he wrote the change.

It's one thing for it to be a subtle side effect; it's another thing entirely if there's an explicitly described test describing the behavior. At the time, the addition of this (and possibly other) changes were seen as only needing a minor version bump. Semver itself leaves it up to our interpretation what counts as the "documented public API." It's still my preference that this behavior be changed to raise as error, especially because that error would be raised from handleAction and not from the returned action creator. It's also my preference that, because the written documentation in the README does say either way, we consider this only a patch change, or perhaps a new minor feature because of the added type checking. But I'll defer to the wisdom of the rest of you on that.

yangmillstheory · 2016-07-01T15:18:36Z

src/handleAction.js

-
-    // If function is passed instead of map, use as reducer
-    if (isFunction(reducers)) {
-      reducers.next = reducers.throw = reducers;


FYI, on my fork with stricter ESLint rules, this fails with

/Users/VictorAlvarez/code/private/redux-actions/src/handleAction.js 18:7 error Assignment to property of function parameter 'reducers' no-param-reassign 18:23 error Assignment to property of function parameter 'reducers' no-param-reassign

If this rule were enabled here, your change would pass it.

Yeah. That was the original problem I saw that compelled me to make this fix– mutating/assigning to the input parameter seemed like a VERY bad idea.

Removed improper type check Added earlier type check on reducers

Update README with docs about optional reducers.

Lucretiel · 2016-07-01T17:45:46Z

So, after sleeping on it, I decided that in this case it's best to defer to the behavior explicitly described by the tests. I also decided that only specifying one of next or throw could make sense in certain situations, like when you want to ignore errors. With this in mind, I reverted the type check, and added some extra supporting code to further do as much preprocessing of the reducers as possible, minimizing the work done by the actual returned reducer. I also added some extra docs to the README about the optional reducers.

yangmillstheory · 2016-07-01T18:50:43Z

Nice. Does the case of passing in a reducer function as opposed to an object handle the case of wanting to ignore errors?

Lucretiel · 2016-07-01T18:57:55Z

Not by itself, no. As per the docs:

If a single reducer is passed, it is used to handle both normal actions and failed actions.

If you pass a single reducer function, instead of an object, the function is invoked for all actions of that type, whether or not they are errors.

Lucretiel · 2016-07-01T19:04:08Z

Keep in mind that that's the typical case anyway. Most users won't ever have a need to create or handle error actions. I myself am already not a fan of async or thunk action creators, because they don't give you a useful handle to the in-progress action that you can cancel or do something else with.

yangmillstheory · 2016-07-01T20:06:33Z

Yes, I've not found many uses for this myself, yet.

What I've used it for, e.g., is force logging a user out if log in or log out fails, for some reason.

After reading #97 (comment) again, I completely agree with the reasoning behind allow any defined/undefined combination of next and throw to be defaulted.

yangmillstheory · 2016-07-09T10:55:31Z

src/handleAction.js

 }

+function coerceReducer(reducer) {
+  return isFunction(reducer) ? reducer : state => state;


This isn't a blocking concern, but I still wish we were more intentional about the argument type passed in.

null and undefined seem reasonable to coerce to a default, but 1, {}, [], true, false seems like a misuse of the API that we should throw TypeError on.

IMO, the most ideal situation would be to not conflate null and undefined, and just pick one to coerce, preferably undefined, so we could use default argument syntax like https://github.com/yangmillstheory/redux-standard-actions/blob/master/src/actionReducer.js#L27.

yangmillstheory · 2016-07-13T04:42:11Z

@timche could you take a look?

@Lucretiel do you mind if I push some minor formatting changes to your branch? (I know you've really been patient with this, so whatever I can do to take some of the extra work off your hands would be great).

Thanks for your patience. I'd love to get this small PR (that's generated some good useful discussion) in soon!

Made the type checking of the reducers more strict, requiring either a function or `undefined`.

Replaced strings with templates Fixed trailing whitespace

yangmillstheory · 2016-07-13T14:23:05Z

src/handleAction.js

@@ -1,20 +1,32 @@
 function isFunction(val) {
  return typeof val === 'function';


Sorry, #97 (comment) wasn't meant to suggest this change - it's more of a longer-term vision for this library. You can revert this commit if you like.

(Also, you can use ES6 string interpolation below.)

yangmillstheory · 2016-07-14T05:17:30Z

This has gotten a bit stale and has got tons of discussion, let's move this over to #109.

Simplified implementation of handleAction

dc54b68

- Moved the function-or-object check on the `reducers` parameter to outside the closure function - Removed the use of magic strings - Removed the mutation of the passed-in `reducers` argument

yangmillstheory reviewed Jun 30, 2016
View reviewed changes

Cleaned up reducer extraction to be more DRY

ecdf628

yangmillstheory reviewed Jun 30, 2016
View reviewed changes

Lucretiel mentioned this pull request Jun 30, 2016

FSA Sequence #22

Closed

Lucretiel reviewed Jun 30, 2016
View reviewed changes

Lucretiel added 2 commits June 30, 2016 17:15

Added type check to the handleAction reducers

15edce5

Fixed linter troubs

566bf59

yangmillstheory reviewed Jun 30, 2016
View reviewed changes

yangmillstheory reviewed Jul 1, 2016
View reviewed changes

Lucretiel added 4 commits July 1, 2016 13:29

Update handleAction.js

8193410

Removed improper type check Added earlier type check on reducers

Update README with docs about optional reducers.

2843315

Merge pull request #1 from Lucretiel/master

2a1709a

Update README with docs about optional reducers.

Fixed linter troubs

5a5e623

yangmillstheory reviewed Jul 9, 2016
View reviewed changes

Lucretiel added 2 commits July 13, 2016 09:26

Updated type checking of the reducers

49efd35

Made the type checking of the reducers more strict, requiring either a function or `undefined`.

Fixed linter troubs

664bb85

Replaced strings with templates Fixed trailing whitespace

yangmillstheory reviewed Jul 13, 2016
View reviewed changes

yangmillstheory mentioned this pull request Jul 14, 2016

Improved implementation of handleAction - Take 2 #109

Merged

yangmillstheory closed this Jul 14, 2016

yangmillstheory mentioned this pull request Sep 8, 2016

Omit payload on undefined only #128

Merged

		@@ -1,20 +1,32 @@
		function isFunction(val) {
		return typeof val === 'function';

Simplified implementation of handleAction #97

Simplified implementation of handleAction #97

Uh oh!

Conversation

Lucretiel commented Jun 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangmillstheory Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lucretiel Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangmillstheory Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lucretiel Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lucretiel Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangmillstheory Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangmillstheory commented Jun 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangmillstheory Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lucretiel commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yangmillstheory commented Jun 30, 2016

Uh oh!

Lucretiel commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yangmillstheory Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lucretiel commented Jul 1, 2016

Uh oh!

yangmillstheory commented Jul 1, 2016

Uh oh!

Lucretiel commented Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lucretiel commented Jul 1, 2016

Uh oh!

yangmillstheory commented Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangmillstheory Jun 30, 2016 •

edited

Loading

Lucretiel Jun 30, 2016 •

edited

Loading

yangmillstheory Jun 30, 2016 •

edited

Loading

Lucretiel Jun 30, 2016 •

edited

Loading

Lucretiel Jun 30, 2016 •

edited

Loading

yangmillstheory Jun 30, 2016 •

edited

Loading

yangmillstheory Jun 30, 2016 •

edited

Loading

Lucretiel commented Jun 30, 2016 •

edited

Loading

Lucretiel commented Jun 30, 2016 •

edited

Loading

yangmillstheory Jul 1, 2016 •

edited

Loading

Lucretiel commented Jul 1, 2016 •

edited

Loading

yangmillstheory commented Jul 1, 2016 •

edited

Loading

yangmillstheory commented Jul 13, 2016 •

edited

Loading