Skip to content
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.

Sign up for GitHub

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

Jump to bottom

createAction does more pre-processing #210

Merged
merged 8 commits into from
May 10, 2017
Merged

createAction does more pre-processing #210

merged 8 commits into from
May 10, 2017

Conversation

Lucretiel
Copy link
Contributor

Slightly simplified createAction, and made it do more processing ahead of time that doesn't have to be done every time an action is emitted

@Lucretiel
createAction does more pre-processing
04a9415 
Slightly simplified createAction, and made it do more processing ahead of time that doesn't have to be done every time an action is emitted
@yangmillstheory yangmillstheory self-requested a review May 10, 2017 01:40
yangmillstheory
yangmillstheory reviewed May 10, 2017
View reviewed changes
src/createAction.js Outdated
@@ -10,35 +9,30 @@ export default function createAction(type, payloadCreator = identity, metaCreato
'Expected payloadCreator to be a function, undefined or null'
);

const finalPayloadCreator = isNull(payloadCreator)
const finalPayloadCreator = payloadCreator === null || payloadCreator === identity
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we're not using isNull anymore? Also why is the identity check needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. No reason. I just figured that the purpose of isNull is to be a predicate to functions like filter, so there's no reason to have the function call overhead here.
  2. The identity check means that if the payloadCreator is identity, we can skip the instanceof Error check every time it's called, and don't have to use the latter version of the finalPayloadCreator.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. re: overhead, this is called just once during when defining the action creator, so do we need to worry about it? If not, I would advise removing the isFunction call above and below as well for consistency, but would prefer to just keep as is.

src/createAction.js Outdated
action.meta = metaCreator(...args);
}

return action;
};

actionCreator.toString = () => type.toString();
const typeStr = type.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the local declaration needed, given that it's used in one place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means that we don't have to call type.toString every time actionCreator.toString is called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this declaration up a scope?

src/createAction.js Outdated
const payload = hasError ? args[0] : finalPayloadCreator(...args);
if (!isUndefined(payload)) {
const payload = finalPayloadCreator(...args);
const action = { type, error: payload instanceof Error };
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the key error always be present. Do we really want this new behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong preference either way. It doesn't change anything for FSA-compliant applications, and I liked the simplicity of this version. I would have put meta and payload inline as well, but there was no good way to do that given the limitations of javascript syntax.

src/createAction.js Outdated
const payload = finalPayloadCreator(...args);
const action = { type, error: payload instanceof Error };

if (payload !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait what's the objection to using isUndefined? Does it cost that much bytes that the readability isn't worth it? It's not much less tokens to parse, but still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a readability thing; just avoiding the function call overhead. I figured that the purpose of isUndefined is to be used as a predicate, like in filter, but there's no reason to use it here, when the comparison is so simple. Compare to isFunction, which has a much more complicated definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense here, sense this is being called dynamically at runtime.

src/createAction.js Outdated

if (isFunction(metaCreator)) {

if (hasMeta) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is where the savings is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, especially given that isFunction is surprisingly involved.

@Lucretiel
Copy link
Contributor Author

Obviously, all of this stuff is based on my personal preference. Originally I was only planning on doing the hasMeta change, but then I started editing other stuff while I was here. I've given my own justification for all these things, but feel free to give me a firm yes or no on any of them.

@yangmillstheory
Copy link
Contributor

So, these are actually quite good reasons. The build is failing because of lint; can you get that to pass and I'll get this merged Thanks.

@yangmillstheory
Copy link
Contributor

yangmillstheory commented May 10, 2017 •
edited
Loading

@Lucretiel I fixed some lint issues in the PR. I would like to get this in, but some tests are failing. Could you fix? These should be reproducible locally with npm test I believe.

@Lucretiel
Copy link
Contributor Author

Yeah, mostly it has to do with the having error always be present. I'll just remove that aspect, since I didn't feel that strongly about it anyway.

@Lucretiel
Copy link
Contributor Author

Do you mind if I do a force push? Or did you make changes you feel strongly about?

@yangmillstheory
Copy link
Contributor

Looks good now. Can you take a look and tell me if this is OK with you? I'll merge and publish a patch version.

@Lucretiel
Copy link
Contributor Author

Made one final change. Everything should be good now.

@yangmillstheory
Copy link
Contributor

Thanks 👍 .

@yangmillstheory yangmillstheory merged commit c87508b into redux-utilities:master May 10, 2017
@yangmillstheory
Copy link
Contributor

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

~/c/p/redux-actions (45c3a78)|master✓
$ npm search redux-actions
NAME                      | DESCRIPTION          | AUTHOR          | DATE       | VERSION  | KEYWORDS
redux-actions             | Flux Standard…       | =acdlite…       | 2017-05-10 | 2.0.3    |
redux-actions-assertions- | Javascript…          | =dmitry-zaets   | 2016-09-12 | 1.1.0    |
js                        |                      |                 |            |          |
redux-actions-sequences   | Make sequences of…   | =andrevinsky    | 2017-01-17 | 1.1.19   |
npmdoc-redux-actions      | #### api…            | =npmdoc         | 2017-04-24 | 2017.4.… |
redux-actions-helpers     | Small helpers…       | =olegman        | 2016-10-27 | 1.0.2    |
redux-actions-api-addon   | Redux Actions…       | =jkusachi       | 2016-07-14 | 1.1.0    |
redux-actions-utils       | Utility fo Redux…    | =dahel          | 2017-03-27 | 0.0.2    |
xm-redux-actions          | Flux Standard…       | =yeanzhi        | 2017-04-24 | 2.0.2    |
redux-actions-assertions- | Javascript…          | =dmitry-zaets   | 2016-09-12 | 1.1.0    |
js                        |                      |                 |            |          |
redux-actions-sequences   | Make sequences of…   | =andrevinsky    | 2017-01-17 | 1.1.19   |
npmdoc-redux-actions      | #### api…            | =npmdoc         | 2017-04-24 | 2017.4.… |
redux-actions-helpers     | Small helpers…       | =olegman        | 2016-10-27 | 1.0.2    |
redux-actions-api-addon   | Redux Actions…       | =jkusachi       | 2016-07-14 | 1.1.0    |
redux-actions-utils       | Utility fo Redux…    | =dahel          | 2017-03-27 | 0.0.2    |

@Lucretiel Lucretiel deleted the patch-3 branch May 10, 2017 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yangmillstheory yangmillstheory yangmillstheory left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Lucretiel @yangmillstheory