fix(store): prevent unexpected behavior of {} as a props type

[markostanimirovic]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

// Case 1
const foo = createAction('foo', props<{}>()); // no error
foo(1); // no error
foo('1'); // no error

// Case 2
const foo = createAction('foo', props<{ bar?: string }>()); // ok
foo({}); // ok

What is the new behavior?

// Case 1
const foo = createAction('foo', props<{}>()); // error
foo(1); // error
foo('1'); // error

// Case 2
const foo = createAction('foo', props<{ bar?: string }>()); // ok
foo({}); // ok

Does this PR introduce a breaking change?

[ ] Yes
[x] No

[ngrxbot]

Preview docs changes for 9a48cce at https://previews.ngrx.io/pr2728-9a48ccea/

[timdeschryver]

I think this is looking good!
@alex-okrushko can you also give this a look please?

@markostanimirovic it is missing a test at https://github.com/markostanimirovic/platform/blob/fix-payload-type/modules/store/spec/types/action_creator.spec.ts#L14, could you add one please?

[markostanimirovic]

I think this is looking good!
@alex-okrushko can you also give this a look please?

@markostanimirovic it is missing a test at https://github.com/markostanimirovic/platform/blob/fix-payload-type/modules/store/spec/types/action_creator.spec.ts#L14, could you add one please?

@timdeschryver sure

[alex-okrushko]

Hey @markostanimirovic
Didn't you want to switch it to the Record<string, unknown> or something similar?

[markostanimirovic]

I think this solution is better for minor/patch release, because with Record I should change signatures of multiple methods. But, I could refactor it in this PR. What do you think @alex-okrushko ?

[alex-okrushko]

I should change signatures of multiple methods.
@markostanimirovic
it shouldn't really change that much - it makes it a bit stricter, right?

[markostanimirovic]

@alex-okrushko Yes, Record<string, unknown> will make the props stricter. Array type will be denied by default and ArraysAreNotAllowed branch can be removed from NotAllowedCheck type guard, as we discussed.

I'll do that 👍

EDIT:
After testing various types with props<P extends Record<string, unknown>>(), I noticed the following behavior:

// Case 1
props<{ bar: string }>(); // ok ✔️

// Case 2
props<any[]>(); // error ✔️

// Case 3
interface Foo {
  bar: string;
}
props<Foo>(); // error ❌

So, it's not compatible with interfaces 👎

[timdeschryver]

LGTM, thanks!

[alex-okrushko]

So, it's not compatible with interfaces 👎

@markostanimirovic I'd argue that they shouldn't be compatible with the interfaces 🙂 Maybe that would be a good breaking (separate) change for the v11! (cc @brandonroberts and @timdeschryver ).

Why?

props<Foo>() does not make sense, because that Foo would be appended with type property, e.g.
const thingHappened = createAction('[source] a thing happened', props<Foo>()) would the action to reducer as Foo & {type: [source] a thing happened}.

How do you remove it in the reducer?

// One might even name it as foo 👇 while in fact it's still an action: foo obj + type prop
on(thingHappened, (state, foo) => ({ ... }))

if the foo is saved that way - you'll get more than you are expecting... and it would show in redux dev tools.
Ideally one would do this:

on(thingHappened, (state, {type, ...foo}) => ({ ... }))

That way type is detached and the actual foo is used further, but I doubt many people do that, and, on top that, it would be easier to wrap it with prop at that point in props: props<{foo: Foo}()> to avoid all of this:

on(thingHappened, (state, {foo}) => ({ ... }))

[markostanimirovic]

So, it's not compatible with interfaces 👎

@markostanimirovic I'd argue that they shouldn't be compatible with the interfaces 🙂 Maybe that would be a good breaking (separate) change for the v11! (cc @brandonroberts and @timdeschryver ).

Why?

props<Foo>() does not make sense, because that Foo would be appended with type property, e.g.
const thingHappened = createAction('[source] a thing happened', props<Foo>()) would the action to reducer as Foo & {type: [source] a thing happened}.

How do you remove it in the reducer?

// One might even name it as foo 👇 while in fact it's still an action: foo obj + type prop
on(thingHappened, (state, foo) => ({ ... }))

if the foo is saved that way - you'll get more than you are expecting... and it would show in redux dev tools.
Ideally one would do this:

on(thingHappened, (state, {type, ...foo}) => ({ ... }))

That way type is detached and the actual foo is used further, but I doubt many people do that, and, on top that, it would be easier to wrap it with prop at that point in props: props<{foo: Foo}()> to avoid all of this:

on(thingHappened, (state, {foo}) => ({ ... }))

@alex-okrushko Looks good. It will make NGRXers to use the library in the right way 🤘

[alex-okrushko]

Btw, this is a breaking change (making it stricter).
cc @brandonroberts