feat(store): provide better TS errors for action creator props #3060

david-shortman · 2021-07-01T01:37:06Z

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?

[ ] Bugfix
[x] 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?

Closes #2892

What is the new behavior?

Does this PR introduce a breaking change?

[x] Yes - usage of `props` outside of an action creator now breaks for invalid types
[ ] No

Other information

Thanks to the advice from Nadia Chibrikova on StackOverflow, props can have its generic argument asserted to pass NotAllowedCheck:

This provides a more precise error than the current one which is hidden within TS outputting multiple errors for a mismatch for each overload of createAction:

Additionally, I've updated the error messages to be hopefully more explicit about what is wrong.

UPDATE: With a suggestive playground link, I took @markostanimirovic's subtle suggestion to explicitly restrict primitive types.

I've also separated the error messages to be more contextually meaningful when using props vs when providing a creator function to createAction.

ngrxbot · 2021-07-01T01:45:34Z

Preview docs changes for 994b640 at https://previews.ngrx.io/pr3060-994b640f/

timdeschryver

Neat!

modules/store/src/action_creator.ts

…eator fn usage

timdeschryver · 2021-07-02T17:29:49Z

modules/store/src/models.ts

@@ -80,6 +95,8 @@ export type Creator<
  R extends object = object
 > = FunctionWithParametersType<P, R>;

+type Primitive = string | number | bigint | boolean | symbol | null | undefined;


Could you also replace this usage of Primitive please?
https://github.com/ngrx/platform/blob/master/modules/store/src/feature_creator_models.ts#L32

I wonder if there's a reason Typescript doesn't provide this type already...

Their site says

object is a type that represents the non-primitive type, i.e. anything that is not number, string, boolean, bigint, symbol, null, or undefined.

So it seems like checking if something extends object would be sufficient.

Perhaps I shouldn't have an explicit check for primitives then.

I need an explicit check because unknown does not extend object, and so it must be differentiated (but we can't check extends unknown because every type is assignable to unknown).

markostanimirovic

It looks much better now 🎉

modules/store/src/models.ts

markostanimirovic

Great work David! Thank you 🥇

There are no breaking changes when props is used within createAction, but props itself has a breaking change. So, I would wait until v13 to merge this improvement.

david-shortman · 2021-07-07T16:31:30Z

There are no breaking changes when props is used within createAction, but props itself has a breaking change. So, I would wait until v13 to merge this improvement.

Oh yeah, good point!

timdeschryver

LGTM 👍

david-shortman · 2021-08-26T02:57:05Z

modules/store/spec/types/action_creator.spec.ts

+      expectSnippet(`
+        const foo = createAction('FOO', props<null>());
+      `).toFail(
+        /Type 'ActionCreatorProps<null>' is not assignable to type '"action creator cannot return an array"'/


this should be

Suggested change

/Type 'ActionCreatorProps<null>' is not assignable to type '"action creator cannot return an array"'/

/Type 'ActionCreatorProps<null>' is not assignable to type '"action creator props cannot be a primitive value"'/

david-shortman · 2021-08-26T02:57:32Z

modules/store/spec/types/action_creator.spec.ts

+      expectSnippet(`
+        const foo = createAction('FOO', props<undefined>());
+      `).toFail(
+        /Type 'ActionCreatorProps<undefined>' is not assignable to type '"action creator cannot return an array"'/


this should also be

Suggested change

/Type 'ActionCreatorProps<undefined>' is not assignable to type '"action creator cannot return an array"'/

/Type 'ActionCreatorProps<undefined>' is not assignable to type '"action creator props cannot be a primitive value"'/

feat(store): provide better TS errors for action creator props

026cf6b

timdeschryver approved these changes Jul 1, 2021

View reviewed changes

timdeschryver reviewed Jul 1, 2021

View reviewed changes

modules/store/src/action_creator.ts Outdated Show resolved Hide resolved

markostanimirovic reviewed Jul 1, 2021

View reviewed changes

modules/store/src/action_creator.ts Outdated Show resolved Hide resolved

david-shortman added 3 commits July 1, 2021 19:24

added primitive restriction and made errors separate for props and cr…

b05a69d

…eator fn usage

fix tests and do not change NotAllowedCheck definition

c890bfd

remove unused message

6e11f45

david-shortman requested review from markostanimirovic and timdeschryver July 2, 2021 00:37

timdeschryver reviewed Jul 2, 2021

View reviewed changes

brandonroberts added the Needs Cleanup Review changes needed label Jul 5, 2021

david-shortman added 3 commits July 6, 2021 21:55

remove unneeded explicit check for primitive type

278d24e

re-include Primitive to account for NotAllowedInPropsCheck<unknown>

35c06f0

removed accidental test code

05e0c2f

markostanimirovic requested changes Jul 7, 2021

View reviewed changes

modules/store/src/models.ts Outdated Show resolved Hide resolved

removed accidental import

994b640

david-shortman requested a review from markostanimirovic July 7, 2021 15:31

markostanimirovic approved these changes Jul 7, 2021

View reviewed changes

timdeschryver added Breaking Change and removed Needs Cleanup Review changes needed labels Jul 7, 2021

timdeschryver approved these changes Jul 7, 2021

View reviewed changes

david-shortman commented Aug 26, 2021

View reviewed changes

brandonroberts merged commit 5ed3c3d into ngrx:master Nov 2, 2021

david-shortman deleted the props-type-assertion branch November 2, 2021 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(store): provide better TS errors for action creator props #3060

feat(store): provide better TS errors for action creator props #3060

david-shortman commented Jul 1, 2021 •

edited

Loading

ngrxbot commented Jul 1, 2021 •

edited

Loading

timdeschryver left a comment

timdeschryver Jul 2, 2021

david-shortman Jul 2, 2021

david-shortman Jul 2, 2021

david-shortman Jul 7, 2021

markostanimirovic left a comment

markostanimirovic left a comment

david-shortman commented Jul 7, 2021

timdeschryver left a comment

david-shortman Aug 26, 2021

david-shortman Aug 26, 2021

	/Type 'ActionCreatorProps<null>' is not assignable to type '"action creator cannot return an array"'/
	/Type 'ActionCreatorProps<null>' is not assignable to type '"action creator props cannot be a primitive value"'/

	/Type 'ActionCreatorProps<undefined>' is not assignable to type '"action creator cannot return an array"'/
	/Type 'ActionCreatorProps<undefined>' is not assignable to type '"action creator props cannot be a primitive value"'/

feat(store): provide better TS errors for action creator props #3060

feat(store): provide better TS errors for action creator props #3060

Conversation

david-shortman commented Jul 1, 2021 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

ngrxbot commented Jul 1, 2021 • edited Loading

timdeschryver left a comment

Choose a reason for hiding this comment

timdeschryver Jul 2, 2021

Choose a reason for hiding this comment

david-shortman Jul 2, 2021

Choose a reason for hiding this comment

david-shortman Jul 2, 2021

Choose a reason for hiding this comment

david-shortman Jul 7, 2021

Choose a reason for hiding this comment

markostanimirovic left a comment

Choose a reason for hiding this comment

markostanimirovic left a comment

Choose a reason for hiding this comment

david-shortman commented Jul 7, 2021

timdeschryver left a comment

Choose a reason for hiding this comment

david-shortman Aug 26, 2021

Choose a reason for hiding this comment

david-shortman Aug 26, 2021

Choose a reason for hiding this comment

david-shortman commented Jul 1, 2021 •

edited

Loading

ngrxbot commented Jul 1, 2021 •

edited

Loading