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

feat(router-store): add action creator for root router actions #2272

Merged
merged 4 commits into from
Dec 14, 2019

Conversation

Jefiozie
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

we added the action creator for root router actions

[ ] 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 #2206

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

@ngrxbot
Copy link
Collaborator

ngrxbot commented Nov 20, 2019

Preview docs changes for 213dace at https://previews.ngrx.io/pr2272-213dace/

@Jefiozie Jefiozie marked this pull request as ready for review November 27, 2019 19:53
@Jefiozie Jefiozie force-pushed the router-action-creators branch from a365589 to 1005064 Compare November 29, 2019 13:31
@Jefiozie
Copy link
Contributor Author

@timdeschryver, I made the changes to the doc. I also made a small diagram for the route-store flow. I think a picture tells more than some text. Looking forward on your reply.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Tbh, I have no idea how these actions are used in the wild.
I'm wondering if we also should provide a router action creator, to keep them generic.
This because a const can't have a generic AFAIK.

This would look as follows:

// allow the flexibility to create a custom router action with a generic
export const createRouterCancelAction = <
  T,
  V extends BaseRouterStoreState = SerializedRouterStateSnapshot
>() => createAction(ROUTER_CANCEL, props<{ payload: RouterCancelPayload<T, V> }>());

// the default implementation with the default router store state
export const routerCancelAction = createRouterCancelAction<SerializedRouterStateSnapshot>();

Does this make sense?

projects/ngrx.io/content/guide/router-store/actions.md Outdated Show resolved Hide resolved
modules/router-store/src/actions.ts Outdated Show resolved Hide resolved
@Jefiozie
Copy link
Contributor Author

Jefiozie commented Dec 3, 2019

Tbh, I have no idea how these actions are used in the wild.
I'm wondering if we also should provide a router action creator, to keep them generic.
This because a const can't have a generic AFAIK.
This would look as follows:
// allow the flexibility to create a custom router action with a generic
export const createRouterCancelAction = <
T,
V extends BaseRouterStoreState = SerializedRouterStateSnapshot
() => createAction(ROUTER_CANCEL, props<{ payload: RouterCancelPayload<T, V> }>());
// the default implementation with the default router store state
export const routerCancelAction = createRouterCancelAction();
Does this make sense?

So the main reason I think we should do this is to have a common Actions API for all of the packages of @ngrx. I believe this will help people to adopt it easier as they understand the way it is working.

Do you agree with this?

I don't think we should change the default router reducer in a similar way as in the @ngrx/store. Probably most projects use the default reducer and use the actions for specific needs. What are you thoughts around this?

@timdeschryver
Copy link
Member

I don't think we should change the default router reducer in a similar way as in the @ngrx/store.

I agree, the reason I mentioned it was for our end-users.

The only reason I see to create the router action creators, is for cases where a custom router serializer is used. In these serializers it's possible to add extra meta-data to the router state, which is added to the dispatched action.

In the current implementation (without creators), it's possible to define this extra meta-data with the generic on the Action. This can be helpful if the Action is used in, for example, an effect because it will provide a type-safe Action with the extra meta-data, for example you be able to do action.routerState.foo.

With the current implementation with the creator functions, this isn't possible anymore and the routerState on the action will always be of the type SerializedRouterStateSnapshot.

That being said, I think this PR is good. We can always add the router action creators later if needed, because I'm uncertain if this is use-case is being used.

@Jefiozie
Copy link
Contributor Author

Jefiozie commented Dec 3, 2019

Okay, just to be sure, this PR is good for now? 😊

@timdeschryver
Copy link
Member

For me it is! 👍

@Jefiozie
Copy link
Contributor Author

Jefiozie commented Dec 3, 2019

Great, thanks for your help will make a separate PR for the image that explains the action order.

@Jefiozie Jefiozie mentioned this pull request Dec 4, 2019
2 tasks
@brandonroberts brandonroberts merged commit f17589f into ngrx:master Dec 14, 2019
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.

feat(router): router actions as ActionCreators
4 participants