Omit payload on undefined only

[timche]

Continuing #115 and part of #122 (v1.0.0).

[timche]

@yangmillstheory This PR is slightly different than #115. Please have a look at the changes. You'll see that I've only changed a single line to fix the tests. It now looks a bit weird to me, because at some parts in createAction we are still using null (for example here).

So in the end, only the identity function is affected by this change. Do you think this is overall still correct?

I actually thought that we are using undefined everywhere to explicitly omit payload. In general the createAction behaviour is a bit weird imo.

This is how I think createAction should behave (compare it against the tests):

const TYPE = 'TYPE';
const errorObj = new TypeError('foo')

// 1. Throw an error when payloadCreator is not `undefined` or a function
// 
// Returns:
// new TypeError('payloadCreator must be undefined or a function')
const actionCreatorB = createAction(TYPE, 'foo')

// 2. Use identity function when payloadCreator is `undefined`
// 
// Returns:
// {
//   type: 'TYPE'
//   payload: 'foo'
// }
const actionCreator = createAction(TYPE)
actionCreator('foo')

// 3. Use payloadCreator when it is a function
// 
// Returns:
// {
//   type: 'TYPE',
//   payload: {
//     a: 'foo',
//     b: 'bar'
//   }
// }
const actionCreator = createAction(TYPE, (a, b) => ({ a, b }))
actionCreator('foo', 'bar')

// 4. Accept a second parameter as function for adding meta to the object
// 
// Returns:
// {
//   type: 'TYPE',
//   payload: 'foo',
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, (a) => a, (b) => b)
actionCreator('foo', 'bar')

// 5. Omit payload when payloadCreator is `undefined`, but a second parameter has been set for adding meta to the object
// 
// Returns:
// {
//   type: 'TYPE',
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, undefined, (a) => a)
actionCreator('bar')

// 6. Set `error` to `true` when payload is an Error object and bypass any payloadCreator
// 
// Returns:
// {
//   type: 'TYPE',
//   error: true,
//   payload: new TypeError('foo')
// }
const actionCreatorA = createAction(TYPE)
const actionCreatorB = createAction(TYPE, (a, b) => ({ a, b }))
actionCreatorA(errorObj)
actionCreatorB(errorObj)

// 7. Set error to true when payload is an Error object and bypass any payloadCreator, but not metaCreator
// 
// Returns:
// {
//   type: 'TYPE',
//   error: true,
//   payload: new TypeError('foo'),
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, (a) => a, (b) => b)
actionCreatorA(errorObj, 'bar')

Maybe I am missing sth, but clarification for this would be very helpful.

This is not only a question for @yangmillstheory, everyone can give his opinion to this.

[timche]

Ping @jrasky, @TylerBrock, @noahsilas, @vbud, @arb.

[jrasky]

Hmm, your list confuses me a little bit. This PR is such that payload is omitted if the output of payloadCreator is undefined, but your list I think says something about omitting payload when payloadCreator itself is undefined.

Regardless, if the convention is to use undefined to omit an argument—and I think it should be—then it's probably a good idea to change the tests to follow that everywhere, even in cases where using null would have the same effect.

[timche]

@jrasky the changes in this PR are already sufficient in your opinion to fullfill your needs??

[yangmillstheory]

Regardless, if the convention is to use undefined to omit an argument—and I think it should be—then it's probably a good idea to change the tests to follow that everywhere, even in cases where using null would have the same effect.

I was thinking about saying this, but looks like it's already been said. We should disambiguate between null and undefined everywhere, especially in tests, since they're an alternate form of documentation.

[yangmillstheory]

@timche

// 4. Accept a second parameter as function for adding meta to the object
// 
// Returns:
// {
//   type: 'TYPE',
//   payload: 'foo',
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, (a) => a, (b) => b)
actionCreator('foo', 'bar')

I think this behavior is incorrect; payloadCreator and metaCreator have the same parameter signature, so metaCreatorshould be (a, b) => b in this case to produce the commented return value.

Otherwise, the examples look 👌 .

To add to the first example, I don't like that we're coercing a non-function here. Like elsewhere in the API, we should not silently coerce, but ask for a function or undefined for payloadCreator, and throw a TypeError if given otherwise. If given undefined, we should use identity.

See also #97 (comment).

However, this hardening doesn't belong in this PR; I can open up a separate one for it if you're on board. Please let me know your thoughts on this. It looks like you already agree based on the first example.

---

@jrasky

Hmm, your list confuses me a little bit. This PR is such that payload is omitted if the output of payloadCreator is undefined, but your list I think says something about omitting payload when payloadCreator itself is undefined.

I don't see anything in the list about omitting payload when payloadCreator is undefined. The closest example in that list would be the second one, but it doesn't apply.

[yangmillstheory]

Can we get rid of this test? It doesn't have anything to do with the test name and is already being tested here.

[yangmillstheory]

Regarding #128 (comment), I don't see any tests where we're calling action creators with null and undefined where we're not intending to; in fact, with this change, I'm sure tests would fail in that case, as one test already had to be changed.

I do see calls to createAction with null payloadCreator, but I don't think this should change now. It should be changed (and will have to be changed, since tests will throw errors) in a separate PR, when only allow functions and undefined for payloadCreator.

See #128 (comment).

[yangmillstheory]

@timche Let's update the README to specify that the payload key will exist if and only if the payloadCreator returns a defined value.

After that, LGTM.

[jrasky]

@yangmillstheory what about number five? I'm probably confused about how this relates to the other examples. Otherwise, something in the README would be perfect.

[yangmillstheory]

@jrasky

Thanks for clarifying, I completely missed that; the fifth example is what you're referring to.

I think that example is specifying the wrong behavior; we shouldn't omit payload if given an undefined payloadCreator, but use the identity as the payloadCreator (and the metaCreator, since it's provided in that example as well).

This would require no new code changes, since we've always delegated to identity. So the corrected fifth example would be

// Returns:
// {
//   type: 'TYPE',
//   payload: 'bar',
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, undefined, (a) => a)
actionCreator('bar')

but is redundant with the second example, which is correct.

We should probably all be aligned on this point...

[timche]

Thanks for the discussion guys.

I've fixed example 4 according to your comment @yangmillstheory:

const TYPE = 'TYPE';
const errorObj = new TypeError('foo')

// 1. Throw an error when payloadCreator is not `undefined` or a function
// 
// Returns:
// new TypeError('payloadCreator must be undefined or a function')
const actionCreatorB = createAction(TYPE, 'foo')

// 2. Use identity function when payloadCreator is `undefined`
// 
// Returns:
// {
//   type: 'TYPE'
//   payload: 'foo'
// }
const actionCreator = createAction(TYPE)
actionCreator('foo')

// 3. Use payloadCreator when it is a function
// 
// Returns:
// {
//   type: 'TYPE',
//   payload: {
//     a: 'foo',
//     b: 'bar'
//   }
// }
const actionCreator = createAction(TYPE, (a, b) => ({ a, b }))
actionCreator('foo', 'bar')

// 4. Accept a second parameter as function for adding meta to the object
// 
// Returns:
// {
//   type: 'TYPE',
//   payload: 'foo',
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, (a) => a, (a, b) => b)
actionCreator('foo', 'bar')

// 5. Omit payload when payloadCreator is `undefined`, but a second parameter has been set for adding meta to the object
// 
// Returns:
// {
//   type: 'TYPE',
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, undefined, (a) => a)
actionCreator('bar')

// 6. Set `error` to `true` when payload is an Error object and bypass any payloadCreator
// 
// Returns:
// {
//   type: 'TYPE',
//   error: true,
//   payload: new TypeError('foo')
// }
const actionCreatorA = createAction(TYPE)
const actionCreatorB = createAction(TYPE, (a, b) => ({ a, b }))
actionCreatorA(errorObj)
actionCreatorB(errorObj)

// 7. Set error to true when payload is an Error object and bypass any payloadCreator, but not metaCreator
// 
// Returns:
// {
//   type: 'TYPE',
//   error: true,
//   payload: new TypeError('foo'),
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, (a) => a, (b) => b)
actionCreatorA(errorObj, 'bar')

---

Regarding example 5: It's not very clear to me then when to use undefined in general, when we are saying, undefined should omit sth. How about we use null instead to make it more predictable that we don't want to define a payloadCreator function and just use the default one a => a?

Also a more understandable example for 5 with meta would be to use the second parameter, otherwise I don't understand why payload and meta should have the same value :P

// Returns:
// {
//   type: 'TYPE',
//   payload: 'bar',
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, null, (a, b) => b)
actionCreator('bar')

---

Before I change the readme, code and tests, the overall outcome:

1. Only undefined will omit payload when passed to the identity function.
2. null as payloadCreator will use the identity function a => a (not decided yet).

Is that correct or am I missing sth?

[yangmillstheory]

1 sounds correct.

---

For 2, why do we need to allow null as well when undefined already gives us the identity payload creator? Regardless, I can live with the extra allowance of null, as long as undefined has the same behavior.

Also, will we throw errors in the case that we receive a payload creator that is not null, undefined, or a Function? See #129.

[jrasky]

I would agree that using undefined is better when what's meant is "use a sane default", especially since actually omitting the argument results in undefined, but I agree with @yangmillstheory that that doesn't have to be the only behavior.

[timche]

Also, will we throw errors in the case that we receive a payload creator that is not null, undefined, or a Function? See #129.

Yes, please.

---

Alright, so lets agree on the following for 2:

• undefined as payloadCreator will use the identity function a => a.

Example:

// 5. Omit payload when payloadCreator is `undefined`, but a second parameter has been set for adding meta to the object
// 
// Returns:
// {
//   type: 'TYPE',
//   payload: 'foo',
//   meta: 'bar'
// }
const actionCreator = createAction(TYPE, undefined, (a, b) => b)
actionCreator('foo', 'bar')

Overall result now is:

1. Only undefined will omit payload when passed to the identity function.
2. undefined as payloadCreator will use the identity function a => a.

---

If we agree on these changes for v1.0.0, we are good to release it asap.

[yangmillstheory]

I'm OK with the proposed. ☝

[BerkeleyTrue]

Seems weird to use undefined as a parameter.

Isn't it convention to use null when something is omitted with purpose? Meaning intentionally left blank.

Where undefined is meant to be provided by the environment when a value is not found?

Also, there is the fact that undefined can be overwritten, causing this rule to exist http://eslint.org/docs/rules/no-undefined.

I've used this on all my projects for years now.

[jrasky]

@BerkeleyTrue take a look at the previous discussion on #115. The short of it is that treating null like undefined means you can't pass a payload of null, and that other projects (including redux itself) and the javascript standard treat the two differently.

It's no longer the case that you can change the value of undefined. According the the spec, null means "explicitly no value," whereas undefined means "no value (yet)."

[BerkeleyTrue]

@jrasky I see. Thanks for the clarification.

[yangmillstheory]

LGTM

[yangmillstheory]

@timche is anything blocking this merge?

[timche]

@yangmillstheory Please review once more, I've removed an obsolete test.

[yangmillstheory]

Was this failing before?

Also, I'm wondering if there's a test somewhere that says the following:

  const explictNullAction = createAction(type)(null);
  expect(explictNullAction).to.deep.equal({
  type,
  payload: null,
});

[timche]

Yes, it was failing before, I fixed it before but I've removed it because of your comment 😅

[yangmillstheory]

@timche @jrasky @BerkeleyTrue

I've been thinking more about Example 2, which eventually resulted in this. I've been questioning it, wondering if it'll lead to more annoyance than usefulness and clarity.

However, I found some more material on the benefits of treating null and undefined differently (these are the first results that show up in a casual search)

• http://stackoverflow.com/a/2959845/2419669
• null props considered differently in getDefaultProps vs. isRequired facebook/react#2166 (comment)
• arg with null should turn to defaultValue or stay null? graphql/graphql-js#210
• https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters
• http://www.codereadability.com/javascript-default-parameters-with-or-operator/

So, I still think we're doing the right thing (there seems to be plenty of justification for it in the above material), even though I don't feel as strongly about it as before.

[timche]

Thanks for the research @yangmillstheory, feeling more confident now that we are doing this change 😁

[alexedev]

Also went in this problem a minute ago.
I passed null to action explicitly and was expecting it to be in payload. For now decided to set value to null in reducer if it receives undefined payload, but hope this will be automated in future updates. Thank you for working on this issue! :-)

[timche]

We are releasing v1.0.0 very soon that allows null to be passed, thanks for your patience :)

[timche]

v1.0.0 has been released! :) Thanks for your patience!