Skip to content

refactor(router): Adjust type of parameter in navigateByUrl and creat… #38227

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
Closed
wants to merge 1 commit into from
Closed

refactor(router): Adjust type of parameter in navigateByUrl and creat… #38227

wants to merge 1 commit into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Jul 24, 2020 •
edited
Loading

…eUrlTree to be more accurate

router.navigateByUrl and router.createUrlTree only use a subset of the NavigationExtras. This commit
changes the parameter type to use new interfaces that only specify the properties used by
those function implementations. NavigationExtras extends both of those interfaces.

Fixes #18798

BREAKING CHANGE: While the new parameter types allow a variable of type
NavigationExtras to be passed in, they will not allow object literals,
as they may only specify known properties. They will also not accept
types that do not have properties in common with the ones in the Pick.
To fix this error, only specify properties from the NavigationExtras which are
actually used in the respective function calls.

@atscott atscott added action: review The PR is still awaiting reviews from at least one requested reviewer area: router target: patch This PR is targeted for the next patch release cross-cutting: types labels Jul 24, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jul 24, 2020
@atscott atscott force-pushed the routeromit branch 2 times, most recently from 0059cc5 to d7b67d5 Compare July 24, 2020 21:12
@atscott atscott added breaking changes state: blocked and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 24, 2020
@atscott atscott modified the milestones: needsTriage, v11-candidates Jul 24, 2020
@atscott
Copy link
Contributor Author

atscott commented Jul 24, 2020

This will need a migration to ensure call locations that violate the new type restriction do not break builds. The simplest way to do this would be to add as {} on the parameter if the object specifies properties that are no longer allowed.

@bradtaniguchi
Copy link

Any update/help/info on what could be done on this PR or what we are waiting for?

Been following this nagging issue for years :)

@atscott
Copy link
Contributor Author

atscott commented Aug 13, 2020

Hi @bradtaniguchi, the change needs to land in a major version release since it's breaking. Once we have a branch that's accepting changes for v11, we can start looking at some of these updates and determining how to move them forward. We have several of them lined up and they're generally labelled with cross-cutting: types.

@crisbeto crisbeto mentioned this pull request Sep 12, 2020
Closed
crisbeto added a commit to crisbeto/angular that referenced this pull request Sep 12, 2020
@crisbeto
feat(router): add migration to update calls to navigateByUrl and crea…
e65956c 
…teUrlTree with invalid parameters

In angular#38227 the signatures of `navigateByUrl` and `createUrlTree` were updated to exclude unsupported
properties from their `extras` parameter. This migration looks for the relevant method calls that
pass in an `extras` parameter and drops the unsupported properties.

**Before:**
```
this._router.navigateByUrl('/', {skipLocationChange: false, fragment: 'foo'});
```

**After:**
```
this._router.navigateByUrl('/', {
  /* Removed unsupported properties by Angular migration: fragment. */
  skipLocationChange: false
});
```

These changes also move the method call detection logic out of the `Renderer2` migration and into
a common place so that it can be reused in other migrations.
AndrewKushnir pushed a commit that referenced this pull request Sep 16, 2020
@crisbeto @AndrewKushnir
feat(router): add migration to update calls to navigateByUrl and crea…
7849fdd 
…teUrlTree with invalid parameters (#38825)

In #38227 the signatures of `navigateByUrl` and `createUrlTree` were updated to exclude unsupported
properties from their `extras` parameter. This migration looks for the relevant method calls that
pass in an `extras` parameter and drops the unsupported properties.

**Before:**
```
this._router.navigateByUrl('/', {skipLocationChange: false, fragment: 'foo'});
```

**After:**
```
this._router.navigateByUrl('/', {
  /* Removed unsupported properties by Angular migration: fragment. */
  skipLocationChange: false
});
```

These changes also move the method call detection logic out of the `Renderer2` migration and into
a common place so that it can be reused in other migrations.

PR Close #38825
@atscott atscott requested a review from crisbeto September 17, 2020 20:39
crisbeto
crisbeto approved these changes Sep 17, 2020
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@atscott atscott added the action: presubmit The PR is in need of a google3 presubmit label Sep 17, 2020
@pullapprove pullapprove bot requested a review from AndrewKushnir September 17, 2020 21:06
@atscott atscott force-pushed the routeromit branch 5 times, most recently from c3b2b92 to d3d5937 Compare September 21, 2020 23:55
petebacondarwin
petebacondarwin approved these changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

I like the new interfaces. CI doesn't :- Also the commit message needs tweaking.

@atscott atscott force-pushed the routeromit branch 2 times, most recently from f596f43 to 8f158ff Compare September 22, 2020 15:44
@atscott
refactor(router): Adjust type of parameter in navigateByUrl and creat…
36a94e5 
…eUrlTree to be more accurate

`router.navigateByUrl` and `router.createUrlTree` only use a subset of the `NavigationExtras`. This commit
changes the parameter type to use new interfaces that only specify the properties used by
those function implementations. `NavigationExtras` extends both of those interfaces.

Fixes angular#18798

BREAKING CHANGE: While the new parameter types allow a variable of type
`NavigationExtras` to be passed in, they will not allow object literals,
as they may only specify known properties. They will also not accept
types that do not have properties in common with the ones in the `Pick`.
To fix this error, only specify properties from the `NavigationExtras` which are
actually used in the respective function calls or use a type assertion
on the object or variable: `as NavigationExtras`.
jelbourn
jelbourn approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api

@mary-poppins
Copy link

You can preview 36a94e5 at https://pr38227-36a94e5.ngbuilds.io/.

jelbourn
jelbourn approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: docs-packaging-and-releasing

AndrewKushnir
AndrewKushnir approved these changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

New interfaces look great 👍

Reviewed-for: public-api

@atscott
Copy link
Contributor Author

atscott commented Sep 23, 2020

Internal migration completed. presubmit (trigger global later as well)

@atscott
Copy link
Contributor Author

atscott commented Sep 24, 2020

caretaker note: global presubmit showed a couple failures from this change. cl/333519736 will resolve those build failures. When syncing, either patch this CL in or wait for it to be submitted.

@atscott atscott added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Sep 24, 2020
@atscott atscott added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: presubmit The PR is in need of a google3 presubmit labels Sep 24, 2020
@alxhub alxhub closed this in e4f4d18 Sep 25, 2020
@crisbeto crisbeto mentioned this pull request Oct 20, 2020
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@crisbeto crisbeto crisbeto approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

+1 more reviewer

@petebacondarwin petebacondarwin petebacondarwin approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: router breaking changes cla: yes cross-cutting: types target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Router navigateByUrl does not consistently apply queryParams
8 participants
@atscott @bradtaniguchi @mary-poppins @petebacondarwin @crisbeto @jelbourn @AndrewKushnir @googlebot