fix(handleAction): Add descriptive error for missing or invalid actions #141

JaKXz · 2016-10-06T16:38:39Z

@yangmillstheory this might be redundant or overkill so feel free to close - but it might be helpful to folks like me to see a more descriptive error when we forget the action type (with the helpful suggestion to use creatAction or createAction(s))

I also took the liberty of adding an .editorconfig and ignoring the JetBrains .idea folder. And removing Node 5 from testing since it's past it's useful life. If any of these bother you I can quickly rebase this and take those changes out.

Resolves #145.

yangmillstheory · 2016-10-06T16:40:34Z

src/handleAction.js

    if (!includes(actionTypes, action.type.toString())) {
      return state;
    }

    return (action.error === true ? throwReducer : nextReducer)(state, action);
  };
 }
+
+function isValidFSA(action) {


Can we use https://github.com/acdlite/flux-standard-action/#isfsaaction?

Ah - yes. Will change.

Though, that method doesn't check if the type is a String or Symbol. I'll go make a PR there ;) [unless you're opposed]

Well... actually...

The type of an action identifies to the consumer the nature of the action that has occurred. Two actions with the same type MUST be strictly equivalent (using ===). By convention, type is usually string constant or a Symbol.

The wording here tells me I probably shouldn't enforce types on the type property? Say if someone wanted to use numbers for their types for whatever reason. What do you think?

Sounds good. Though, we may be dropping Symbol support soon #126.

yangmillstheory · 2016-10-06T16:41:12Z

src/handleAction.js

@@ -12,10 +14,18 @@ export default function handleAction(actionType, reducers, defaultState) {
    : [reducers.next, reducers.throw].map(reducer => (isNil(reducer) ? identity : reducer));

  return (state = defaultState, action) => {
+    if (!isValidFSA(action)) {


Can we standardize on invariant?

Could you teach me what invariant does/is for?

It's a way to ensure that application the application is in the right state, and throw errors otherwise. React and Redux use this quite a bit. We're starting to standardize on this in #127, #129, and #131.

yangmillstheory · 2016-10-06T16:42:23Z

.editorconfig

@@ -0,0 +1,16 @@
+root = true


I'm not familiar with .editorconfig. Can you quickly teach me what value this adds?

Sure! EditorConfig is really good for keeping line endings, indentation, trailing spaces and so on and so forth in sync between different IDEs. It's just a quick headache reducer as long as contributors have it installed in their editors (some of which have it bundled out of the box).

Cool. I recently switched from Webstorm back to Sublime Text, so it looks like I'll have to download a plugin.

What happens if a rule here is broken? Does the build fail? What value does this add beyond linting?

It doesn't do linting, it just works inside the editor make those things happen (eg. tabs = 2 spaces, or all files get a newline at the end).

JaKXz · 2016-10-06T19:29:33Z

Upstream PR for enforcing the action.type's type: redux-utilities/flux-standard-action#36

yangmillstheory · 2016-10-06T20:30:07Z

Could we make a separate PR and issue for the unrelated changes (adding .editorconfig and removing [email protected] from the build?

That would help clarify the scope of this PR and make it more targeted.

JaKXz · 2016-10-06T20:42:08Z

@yangmillstheory made separate PRs :) #142 and #143.

timche · 2016-10-06T20:50:24Z

.gitignore

@@ -3,3 +3,4 @@ coverage
 *.log
 lib
 dist
+.idea


Can you create a single commit or pr for this as well pls?

@timche #144 :)

yangmillstheory · 2016-10-07T00:55:12Z

src/__tests__/handleAction-test.js

+      const action1 = createAction('ACTION_1');
+      const reducer = handleAction(
+        action1,
+        (state) => state


For this and the tests below:

can we use the identity function from lodash?

can we inline the call to createAction (action1 is not used elsewhere)?

Also, unless there's a good reason to disablemax-len, let's stick to it. Since we're testing an exact match, can we use the string directly?

max-len is because the message is longer than 100 characters, so I think it's reasonable to disable it. The regex was in case any of you wanted to change the message edit: ie. have a little less dependence in the test on the exact message... but obviously ATM it's just a direct copy/paste.

yangmillstheory · 2016-10-07T00:58:10Z

The build is failing. I'd like to get this in (great contribution that should have been there at the outset since we're explicitly for FSA), so let's make sure that it passes as soon as we can.

JaKXz · 2016-10-07T01:54:31Z

@yangmillstheory I've made redux-utilities/flux-standard-action#36 pass CI, but it needs to be merged and released to unblock CI here...

edit: of course that being said I got carried away again and it may need to be split into several PRs. Although, I do think it's valuable because it's a smaller library and it upgrades everything in one shot and takes care of all the JS-fatigue-babel-and-dependencies things [I found myself laughing]

cc @acdlite

yangmillstheory · 2016-10-11T16:53:05Z

We're planning on dropping Symbol support soon #126 (please see the linked discussions there). I'm not sure it's worth waiting on upstream support for Symbols.

JaKXz · 2016-10-11T16:56:21Z

@yangmillstheory good point, but upstream only checks if action.type is truthy, not explicitly a string. That's why the last test is failing.

It will be easy enough to remove support for Symbols in the next ~~major~~ (breaking) version. :)

yangmillstheory · 2016-10-11T17:15:21Z

I'm not sure that the last test (or the upstream PR) is a requirement for this merge. While I agree it's technically correct to enforce that the action type is a string (per the FSA spec IIRC), anyone who would explicitly pass false for an action type might reasonably be referred to library documentation.

What other tests are failing beyond that one?

JaKXz · 2016-10-11T18:16:25Z

@yangmillstheory that's the only test that's failing, but I would say that the explicit type-check is blocking since it's not spec-compliant. I think I see what you're saying about being able to merge without that test passing since it's not necessarily a "valid use case" (i.e. nobody will actually do it) but I'd personally rather be explicit first.

Up to you all as maintainers though.

JaKXz · 2016-10-27T04:12:27Z

~~@yangmillstheory @timche I've published my fork of flux-standard-action (the irony is strong with this one) since @acdlite has not responded.~~

~~This should be ready to merge now. :)~~

I'll publish a new version of flux-standard-action tomorrow, and then update this PR.

JaKXz · 2016-10-27T15:57:10Z

@yangmillstheory @timche updated :) really ready to merge now.

yangmillstheory · 2016-10-28T01:09:48Z

src/__tests__/handleAction-test.js

+      const reducer = handleAction(createAction('ACTION_1'), identity);
+      expect(() => reducer(undefined))
+      // eslint-disable-next-line max-len
+        .to.throw(/The FSA spec mandates an action object with a type. Try using the createAction\(s\) method./);


I'd like to push back again on the eslint-disable-next-line max-len and the use of a regular expression here.

I'd like to use these overrides only where they must be used. In a 20K line production application, I've seen this happen only in two places:

directly consuming snake-cased fields from the API on the client (eslint-disable-next-line camelcase)

early returns from iterators in lodash (eslint-disable-next-line consistent-return)

It wouldn't be a rule if we were permissive about bypassing it; the value of the rule comes from a widespread adherence.

I also think we should strive for consistency with respect to similar tests in the repo.

To that end, let's test the error class thrown, so callers know what to catch, and the exact string (I don't think the flexibility afforded by using a regular expression object buys us much).

Done. Thanks for the feedback!

invariant only throws a regular Error and I didn't want to introduce matchers just to check for the error name to be "Invariant Violation". I didn't see it being much value, but the format should be consistent now.

yangmillstheory · 2016-10-28T02:20:46Z

I'm "dismissing" @timche's review since all asks have been addressed. I'm also changing the merge target branch from master to v1.0.0.

In the future, let's strive to create more targeted PR's with smaller diffs. That will ensure that reviews are more focused and less noisy.

Thanks for the contribution!

All comments have been addressed

yangmillstheory · 2016-10-28T02:24:51Z

@JaKXz can you fetch the upstream v1.0.0 branch, merge those changes into your branch, and update the PR?

according to the FSA spec

and fix lint issues

JaKXz · 2016-10-28T02:27:43Z

@yangmillstheory done. Anything I can do to help #127 get merged?

yangmillstheory · 2016-10-28T02:31:38Z

Thanks. I'll take care of #127 next.

Now that you're a collaborator on FSA, do you have any opinions on #126?

…ns (#141) close #145

With #141 we made other libraries incompatible with redux-actions and results to an unexpected behavior. If an action is not a FSA, handleAction should not handle it and just return it to the next reducer. This fix will remove the FSA check.\n\nCloses #164, #167

With #141 we made other libraries incompatible with redux-actions and results to an unexpected behavior. If an action is not a FSA, handleAction should not handle it and just return it to the next reducer. This fix will remove the FSA check. Closes #164, #167

With #141 we have made other libraries incompatible with redux-actions and it resulted to an unexpected behavior (#164, #167). If an action is not a FSA, handleAction should not handle it and just pass it to the next reducer. This fix will remove the FSA check to support Non-FSA. Closes #164, #167

yangmillstheory suggested changes Oct 6, 2016

View reviewed changes

yangmillstheory assigned yangmillstheory and timche Oct 6, 2016

timche previously requested changes Oct 6, 2016

View reviewed changes

yangmillstheory suggested changes Oct 7, 2016

View reviewed changes

yangmillstheory mentioned this pull request Oct 8, 2016

Fail early for non-FSA actions #145

Closed

JaKXz mentioned this pull request Oct 11, 2016

handleAction should accept an empty action i.e., {} #139

Closed

yangmillstheory suggested changes Oct 28, 2016

View reviewed changes

yangmillstheory approved these changes Oct 28, 2016

View reviewed changes

yangmillstheory changed the base branch from master to v1.0.0 October 28, 2016 02:21

JaKXz added 5 commits October 27, 2016 22:27

fix(handleAction): Add descriptive error for missing or invalid actions

a9fae83

according to the FSA spec

Use flux-standard-action & invariant

f0a5b22

and fix lint issues

Address PR comments

a3369a7

use updated flux-standard-action library

cf17f71

more consistent formatting, drop regex assertion

f16ab05

yangmillstheory merged commit d8d7f63 into redux-utilities:v1.0.0 Oct 28, 2016

yangmillstheory mentioned this pull request Oct 28, 2016

Make defaultState required when creating reducers #127

Merged

JaKXz deleted the fix/invalid-action-error branch October 28, 2016 04:18

timche pushed a commit that referenced this pull request Nov 18, 2016

fix(handleAction): Add descriptive error for missing or invalid actio…

ea04ccf

…ns (#141) close #145

noahsilas mentioned this pull request Nov 18, 2016

Non-FSA actions cause handleAction reudcers to emit confusing error messages #164

Closed

timche mentioned this pull request Nov 21, 2016

Remove FSA check in handleAction(s) #168

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(handleAction): Add descriptive error for missing or invalid actions #141

fix(handleAction): Add descriptive error for missing or invalid actions #141

JaKXz commented Oct 6, 2016 •

edited

Loading

yangmillstheory Oct 6, 2016

JaKXz Oct 6, 2016

JaKXz Oct 6, 2016 •

edited

Loading

JaKXz Oct 6, 2016

yangmillstheory Oct 6, 2016

yangmillstheory Oct 6, 2016

JaKXz Oct 6, 2016

yangmillstheory Oct 6, 2016

JaKXz Oct 7, 2016

yangmillstheory Oct 6, 2016

JaKXz Oct 6, 2016

yangmillstheory Oct 6, 2016

JaKXz Oct 6, 2016

JaKXz commented Oct 6, 2016

yangmillstheory commented Oct 6, 2016

JaKXz commented Oct 6, 2016

timche Oct 6, 2016

JaKXz Oct 6, 2016

yangmillstheory Oct 7, 2016 •

edited

Loading

JaKXz Oct 7, 2016 •

edited

Loading

yangmillstheory commented Oct 7, 2016 •

edited

Loading

JaKXz commented Oct 7, 2016 •

edited

Loading

yangmillstheory commented Oct 11, 2016

JaKXz commented Oct 11, 2016 •

edited

Loading

yangmillstheory commented Oct 11, 2016

JaKXz commented Oct 11, 2016

JaKXz commented Oct 27, 2016 •

edited

Loading

JaKXz commented Oct 27, 2016

yangmillstheory Oct 28, 2016

JaKXz Oct 28, 2016

yangmillstheory commented Oct 28, 2016

yangmillstheory commented Oct 28, 2016

JaKXz commented Oct 28, 2016

yangmillstheory commented Oct 28, 2016

@@ @@ -3,3 +3,4 @@ coverage @@
               *.log
               lib
               dist
+              .idea

fix(handleAction): Add descriptive error for missing or invalid actions #141

fix(handleAction): Add descriptive error for missing or invalid actions #141

Conversation

JaKXz commented Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaKXz Oct 6, 2016 • 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

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

JaKXz commented Oct 6, 2016

yangmillstheory commented Oct 6, 2016

JaKXz commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangmillstheory Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

JaKXz Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

yangmillstheory commented Oct 7, 2016 • edited Loading

JaKXz commented Oct 7, 2016 • edited Loading

yangmillstheory commented Oct 11, 2016

JaKXz commented Oct 11, 2016 • edited Loading

yangmillstheory commented Oct 11, 2016

JaKXz commented Oct 11, 2016

JaKXz commented Oct 27, 2016 • edited Loading

JaKXz commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangmillstheory commented Oct 28, 2016

yangmillstheory commented Oct 28, 2016

JaKXz commented Oct 28, 2016

yangmillstheory commented Oct 28, 2016

JaKXz commented Oct 6, 2016 •

edited

Loading

JaKXz Oct 6, 2016 •

edited

Loading

yangmillstheory Oct 7, 2016 •

edited

Loading

JaKXz Oct 7, 2016 •

edited

Loading

yangmillstheory commented Oct 7, 2016 •

edited

Loading

JaKXz commented Oct 7, 2016 •

edited

Loading

JaKXz commented Oct 11, 2016 •

edited

Loading

JaKXz commented Oct 27, 2016 •

edited

Loading