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.

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

Fix disallow arrays in action creators #2155

Merged
merged 4 commits into from
Oct 14, 2019
Merged

Fix disallow arrays in action creators #2155

merged 4 commits into from
Oct 14, 2019

Conversation

chrisguttandin
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

I think it is debatable whether this is a bugfix or a feature.

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

I noticed that the new createAction() function has a clever type definition which prevents it from being used with a props object shaped like this { type: any }. Since an array is also an object in JavaScript it can still be used like this:

const myAction = createAction('MY_ACTION_TYPE', (x: string) => [ x ]);
const myOtherAction = createAction('MY_ACTION_TYPE', props<[ string ]>());

Of course this becomes a little odd when the resulting function is used to create an actual action:

myAction('hello');

TypeScript thinks it has the type string[] & TypedAction<"MY_ACTION_TYPE"> but in reality it is an object with the following shape: { 0: hello, type: 'MY_ACTION_TYPE' }.

What is the new behavior?

A similar technique to the one which prevents { type: any } is used to prevent arrays which means createAction('MY_ACTION_TYPE', props<[]>()) will not compile anymore.

Does this PR introduce a breaking change?

Technically this is a breaking change. But it will only break code which used createAction() in an unintended way.

[ -] Yes
[ -] No

Other information

Please let me know if there is anything I need to change before this can be merged. And in case you think this is not necessary at all feel free to close it.

I also cleaned up the test suite of the action creator a little bit by removing an unused line of code and by inserting a missing blank line and a missing quote. Please let me know if I should better exclude those changes from this pull request.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Oct 8, 2019

Preview docs changes for aef4dba at https://previews.ngrx.io/pr2155-aef4dba/

@brandonroberts brandonroberts self-assigned this Oct 8, 2019
@brandonroberts brandonroberts added Need Discussion Request For Comment needs input Project: Store labels Oct 8, 2019
Copy link
Member

@alex-okrushko alex-okrushko left a comment

Choose a reason for hiding this comment

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

Nice. Interesting use case. Thanks for adjusting it.

@brandonroberts brandonroberts merged commit 1e4c0be into ngrx:master Oct 14, 2019
@chrisguttandin chrisguttandin deleted the fix-disallow-arrays-in-action-creators branch October 18, 2019 09:39
jordanpowell88 pushed a commit to jordanpowell88/platform that referenced this pull request Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need Discussion Request For Comment needs input Project: Store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants