Skip to content

Improved implementation of handleAction - Take 2 #109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 17, 2016

Conversation

yangmillstheory
Copy link
Contributor

This is based off of the ideas in #97, which generated a lot of good discussion. Roughly:

  • don't mutate the passed in reducers
  • use small lodash libraries
  • properly scope reducer setup (i.e. do it once, and not every time an action is handled)

Full credit goes to @Lucretiel; just submitting it here for a cleaner pull request.

@Lucretiel, please review as well.

if (action.type !== typeValue) return state;
const typeValue = isSymbol(type)
? type
: type.toString();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works with string action types and action creators. I think calling out isSymbol explicitly clarifies the special case (where we cannot call .toString()).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@yangmillstheory
Copy link
Contributor Author

ping @timche for the new dependencies and that general approach
ping @Lucretiel

This is a small PR (from the long-outstanding #97) so, I'm planning to get merge sometime over the weekend. If there's no time to review, we can review retrospectively and I can address those comments after merge.

return isFunction(reducer)
? reducer(state, action)
: state;
return (action.error === true ? throwReducer : nextReducer)(state, action);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=== true could be omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on #97 (comment)?

Copy link
Member

@timche timche Jul 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, sorry, error could be any other value than true. === true adds more safety to respect FSA.

Copy link
Contributor Author

@yangmillstheory yangmillstheory Jul 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! I thought the same thing when reviewing #97; thanks to @Lucretiel for doing the research and pointing it out.

@timche
Copy link
Member

timche commented Jul 16, 2016

After clarified all the things, this PR is very neat 👌🏻. You get my check mark after you have resolved the linting issues 😁

@Lucretiel are you also fine with that?

@yangmillstheory
Copy link
Contributor Author

Thanks, and big thanks to @Lucretiel (this is really his PR).

There's a build break with b5546b5

/Users/VictorAlvarez/code/private/redux-actions/src/handleAction.js (3/0)
  ✖  1:24  Unable to resolve path to module 'lodash/isfunction'  import/no-unresolved
  ✖  3:19  Unable to resolve path to module 'lodash/isnil'       import/no-unresolved
  ✖  4:22  Unable to resolve path to module 'lodash/issymbol'    import/no-unresolved

I'll see what I can do to fix it; let me know if you have any ideas on it.

@yangmillstheory
Copy link
Contributor Author

yangmillstheory commented Jul 16, 2016

9a317f3 fixed the build. The interesting lesson was that the environments the tests were run in had a case-insensitive filesystem, including mine:

$ touch abc1
$ touch abC1
$ ls -l ab*
-rw-r--r--  1 VictorAlvarez  staff  0 Jul 16 02:05 abc1

but for some reason eslint-plugin-import wasn't picking this up.

@yangmillstheory
Copy link
Contributor Author

@timche what do you think of releasing this in v0.10.2?

@yangmillstheory yangmillstheory merged commit bd5b08a into master Jul 17, 2016
@timche
Copy link
Member

timche commented Jul 17, 2016

I would make this part of v0.11.0 with a bunch of other non-breaking PRs. Should try to follow semver.

yangmillstheory added a commit that referenced this pull request Jul 18, 2016
@yangmillstheory
Copy link
Contributor Author

Squashed merged in 5e78f25.

Would you consider this addition a bug fix or new functionality?

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards-compatible manner, and
PATCH version when you make backwards-compatible bug fixes.
Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

@timche timche deleted the hygiene/handle-actions-no-mutate branch April 18, 2018 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants