Skip to content

Only omit payload if undefined #115

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

Closed
wants to merge 3 commits into from
Closed

Conversation

jrasky
Copy link

@jrasky jrasky commented Aug 1, 2016

Include a null payload if defined

jrasky added 2 commits August 1, 2016 14:49
Include a `null` payload if defined
@yangmillstheory
Copy link
Contributor

yangmillstheory commented Aug 2, 2016

Thanks for the contribution. I like distinguishing between null and undefined, but have you seen #48, and in particular #48 (comment)?

I think @timche might have an opinion about this change.

@jrasky
Copy link
Author

jrasky commented Aug 2, 2016

The ECMAScript spec distinguishes between undefined as "no assigned value" and null as "intentionally absent value." I'm not sure it's more common to use null to omit an argument, as most of the code examples I've seen use undefined to omit arguments.

For instance, the MDN article on Promises uses undefined to show the relationship between catch and then. So does the ECMAScript specification for promises.

Further, a codebase that I work on uses undefined to mean "no data loaded yet," and null to mean "no data exists," which isn't a distinction supported by redux-actions in its current state.

@yangmillstheory
Copy link
Contributor

ping @timche

I'm happy to make an issue to not conflate undefined and null going forward.

@timche
Copy link
Member

timche commented Aug 2, 2016

This is a hard one imo. For example react-redux is also using null in their connect function to omit arguments.

I also prefer null over undefined, because undefined means to me "not defined yet" and null "no value".

@jrasky can you please describe the problem you see with it with an example?

Edit: Also, why are '', 0, [], {} or false not sufficient as state?

@jrasky
Copy link
Author

jrasky commented Aug 2, 2016

I don't doubt there's cases where treating null and undefined the same way is fine, and react-redux shows such an example. I do think, however, that the ECMAScript standard is a higher source of truth than react-redux.

The case I'm running into is where I'm using undefined to mean "no data yet," and null to mean "no data available," which is roughly in line with how the spec defines them.

Very specifically, I don't expect that passing in null as a payload will result in the reducer receiving no payload, since null in javascript means "explicitly no value," unlike undefined.

There are always "more code" approaches to solving nearly any problem, my concern here is with having more correct, spec-compliant code.

In other words, I want to pass null as a payload, and redux-actions doesn't provide for that. Yes, I can work around that issue, but I'd rather solve it instead.

@timche
Copy link
Member

timche commented Aug 2, 2016

For me personally, the open source/JS community is a higher source than the ECMAScript standard, because it represents how JS is actually used widely.

I'd be happy if you could provide some links and I'm willing to change it, but that would also mean, it is going to be a breaking change and we have to release a major version.

@jrasky
Copy link
Author

jrasky commented Aug 2, 2016

To be clear, I think your usage here is different from how redux-react uses null, because in the redux-actions case the value is "passed along."

There's a couple things I can link you to in the standard:

redux itself treats null and undefined differently, although in a somewhat implicit fashion.

It is a somewhat significant change, but also one that's not very difficult to work around. That being said, the codebase I'm working on and the standard both seem to indicate that undefined is the "omitted argument" value.

@yangmillstheory yangmillstheory mentioned this pull request Aug 19, 2016
@timche
Copy link
Member

timche commented Aug 20, 2016

I'd like to fix this with v1.0.0, also to follow semver. Everybody's fine with that?

@@ -13,7 +13,7 @@ export default function createAction(type, payloadCreator, metaCreator) {
};

const payload = hasError ? args[0] : finalPayloadCreator(...args);
if (!(payload === null || payload === undefined)) {
if (payload !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please use the lodash function lodash/isUndefined here?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, but just consider that it does exactly the same thing: https://github.com/lodash/lodash/blob/master/lodash.js#L11878

@timche timche mentioned this pull request Aug 31, 2016
3 tasks
@TylerBrock
Copy link

Lets get this merged! We just bumped into a situation where our payload is null (explicitly) and the current behavior was surprising.

@timche
Copy link
Member

timche commented Sep 1, 2016

@jrasky Can please have a look at this?

L38 looks a bit confusing to me. Are we now supposed to use null or undefined there?

@timche
Copy link
Member

timche commented Sep 6, 2016

Continuing in #128 to speed up the process.

@timche timche closed this Sep 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants