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(store): replace Creator with ActionCreator on createAction #2299

Merged
merged 2 commits into from
Jan 6, 2020

Conversation

meeroslav
Copy link
Contributor

createAction returns incorrect type as neither Creator not it's return type contains type.
The proper return type is ActionCreator with the inner typed creator

Closes meeroslav/ng-helpers#2

Could contribute to #2192 as well.

PR Checklist

Please check if your PR fulfills the following requirements:

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?

Using type ActionCreator as the input parameter is causing type errors due to the Creator type being insufficient. ActionCreator is a function that has type defined and returns typed Action. Creator does not return typed Action.

Closes meeroslav/ng-helpers#2

What is the new behavior?

ActionCreator now returns TypedCreator that extends CreatorFunctionality by returning TypedAction

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

createAction returns incorrect type as neither Creator not it's return type contains `type`.
Proper return type is ActionCreator with inner typed creator

Closes meeroslav/ng-helpers#2
@ngrxbot
Copy link
Collaborator

ngrxbot commented Jan 2, 2020

Preview docs changes for e846375 at https://previews.ngrx.io/pr2299-e846375/

modules/store/src/action_creator.ts Show resolved Hide resolved
modules/store/src/action_creator.ts Outdated Show resolved Hide resolved
modules/store/src/models.ts Outdated Show resolved Hide resolved
modules/store/src/models.ts Outdated Show resolved Hide resolved
@meeroslav
Copy link
Contributor Author

Thanks, @alex-okrushko, for quick feedback. I should have provided a bit more information to clarify the reasoning.

PR solves two complementary type issues.

createAction should return ActionCreator<T>

Since none of the overrides return Creator this is the correct return type. Equally, defineType should return ActionCreator<T> not Creator. This was just a side effect of the actual problem:

ActionCreator<T> is a function that returns TypedAction<T>

This is where the error actually happens. If I write a function like this:

function actionWrapper<P>(creator: ActionCreator<string, Creator>, payload: P): Observable<Action> {
  return of(creator(payload));
}

It will report error Type 'Observable<{}>' is not assignable to type 'Observable<Action> | ((...args: any[]) => Observable<Action>)'.

If you look closely at ActionCreator type, it extends Creator with type property. Since, as you mentioned, type property is explicitly forbidden on Creators return type R, ActionCreator could never return an object with type property - therefore, the result of ActionCreator type could never be Action type as type property would be missing.

The second generic type of ActionCreator cannot be Creator as it's in contradiction with createAction return types () => TypedAction<T>, (props: P) => P & TypedAction<T>> and FunctionWithParametersType<P, R & TypedAction<T>>.

I agree that TypedCreator might be an unfortunate naming choice. Perhaps it should be explicitly typed as FunctionWithParametersType<P, R & TypedAction<T>>.

@meeroslav
Copy link
Contributor Author

meeroslav commented Jan 3, 2020

@alex-okrushko, I've seen now that your PR #2301 solves the second issue so I will revert TypedCreator

@meeroslav meeroslav changed the title fix(store): replace insufficient type Creator with TypedCreator fix(store): replace Creator with ActionCreator on createAction Jan 3, 2020
@alex-okrushko
Copy link
Member

@meeroslav Yes, thanks. There was a separate issue that got me to rework that part.

I think "createAction should return ActionCreator" is the only part that should be adjusted. Thanks for creating this issue and PR.

@meeroslav
Copy link
Contributor Author

@alex-okrushko, are the latest changes (reverts) then ok, or should I do something additional in this PR?

@alex-okrushko
Copy link
Member

I think this is good!

@alex-okrushko alex-okrushko merged commit fe6bfa7 into ngrx:master Jan 6, 2020
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.

type casting necessary due to insufficient types on ngrx
3 participants