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.

Sign up for GitHub

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

Jump to bottom

feat(RouterStore): add routerState to action payload #1511

Merged
merged 4 commits into from
Jan 21, 2019
Merged

feat(RouterStore): add routerState to action payload #1511

merged 4 commits into from
Jan 21, 2019

Conversation

timdeschryver
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

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?

Only the RouterNavigationAction, RouterCancelAction and RouterErrorAction events had the routerState property in the payload.

Closes #1509

What is the new behavior?

All of the router events will have the routerState property in the payload.
The routerState property is added to RouterRequestAction and RouterNavigatedAction

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

These actions were introduced in #1267 and I couldn't find why the routerState property isn't added. The rest of the actions had this property from version 4.0.0.

brandonroberts
brandonroberts requested changes Jan 15, 2019
View reviewed changes
modules/router-store/src/actions.ts Outdated
@@ -19,16 +19,19 @@ export const ROUTER_REQUEST = '@ngrx/router-store/request';
/**
* Payload of ROUTER_REQUEST
*/
export type RouterRequestPayload = {
export type RouterRequestPayload<T extends BaseRouterStoreState> = {
Copy link
Member

Choose a reason for hiding this comment

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

Won't this be breaking if no default is provided?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, this is a breaking change - the type needs to be there.

modules/router-store/src/actions.ts Outdated
@@ -112,16 +115,19 @@ export const ROUTER_NAVIGATED = '@ngrx/router-store/navigated';
/**
* Payload of ROUTER_NAVIGATED.
*/
export type RouterNavigatedPayload = {
export type RouterNavigatedPayload<T extends BaseRouterStoreState> = {
Copy link
Member

Choose a reason for hiding this comment

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

Same

modules/router-store/src/router_store_module.ts Show resolved Hide resolved
modules/router-store/src/router_store_module.ts Outdated
@@ -302,7 +300,13 @@ export class StoreRouterConnectingModule {
private dispatchRouterAction(type: string, payload: any): void {
Copy link
Member

Choose a reason for hiding this comment

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

Will you check to see how we can improve the signature for this function? payload: any could be RouterEvent at a minimum I think.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jan 20, 2019

Preview docs changes for 7152ccf at https://previews.ngrx.io/pr1511-7152ccf/

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jan 20, 2019

Preview docs changes for 81c05e4 at https://previews.ngrx.io/pr1511-81c05e4/

@brandonroberts
Copy link
Member

@timdeschryver is this ready for another review? Some of the defaults are still missing for the types.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jan 21, 2019

Preview docs changes for 5c2476f at https://previews.ngrx.io/pr1511-5c2476f/

@timdeschryver
Copy link
Member Author

@brandonroberts I just added the defaults.
I think it's ready but I'll review this PR later today when I'm back home.

@brandonroberts brandonroberts merged commit 283424f into master Jan 21, 2019
@brandonroberts brandonroberts deleted the pr/router-actions-states branch January 21, 2019 14:33
@alex-okrushko
Copy link
Member

Hey @timdeschryver
This is a breaking change actually. routerState: T; is now added as a required field to the interface, and that broke a number of projects with my today's sync.

 error TS2322: Type '{ event: NavigationEnd; }' is not assignable to type 'RouterNavigatedPayload<SerializedRouterStateSnapshot>'.
  Property 'routerState' is missing in type '{ event: NavigationEnd; }'.

Should the routerState be optional, or do I need to fix all of the internal tests that don't have it?

@timdeschryver timdeschryver restored the pr/router-actions-states branch March 27, 2019 21:35
@timdeschryver timdeschryver deleted the pr/router-actions-states branch March 27, 2019 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonroberts brandonroberts brandonroberts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Router state to RouterNavigatedPayload
4 participants
@timdeschryver @ngrxbot @brandonroberts @alex-okrushko