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

fix(router): Update Navigation#initialUrl to match documentation and reality #43863

Closed
wants to merge 1 commit into from
Closed

fix(router): Update Navigation#initialUrl to match documentation and reality #43863

wants to merge 1 commit into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Oct 15, 2021 •
edited
Loading

BREAKING CHANGE:

  • The type of initialUrl is set to string|UrlTree but in reality,
    the Router only sets it to a value that will always be UrlTree
  • initialUrl is documented as "The target URL passed into the
    Router#navigateByUrl() call before navigation" but the value
    actually gets set to something completely different. It's set to the
    current internal UrlTree of the Router at the time navigation
    occurs.

PR-only note:
TGP showed no failures as a result of this change. We could consider
exposing the old t.currentRawUrl value as a new property on
Navigation if this does end up breaking someone. That said,
I don't believe this value was intended to be exposed and it's likely
that anyone using it currently could or should use something else
to do the task it's being used for.

@atscott atscott added area: router target: major This PR is targeted for the next major release labels Oct 15, 2021
@atscott atscott added this to the v14-candidates milestone Oct 15, 2021
@google-cla google-cla bot added the cla: yes label Oct 15, 2021
@atscott atscott marked this pull request as ready for review January 27, 2022 17:53
@pullapprove pullapprove bot requested a review from alxhub January 27, 2022 21:14
AndrewKushnir
AndrewKushnir approved these changes Jan 27, 2022
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.

Reviewed-for: public-api

alxhub
alxhub approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

jessicajaniuk
jessicajaniuk approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk 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

@atscott
fix(router): Update Navigation#initialUrl to match documentation an…
cfbca2a 
…d reality

BREAKING CHANGE:

* The type of `initialUrl` is set to `string|UrlTree` but in reality,
  the `Router` only sets it to a value that will always be `UrlTree`
* `initialUrl` is documented as "The target URL passed into the
  `Router#navigateByUrl()` call before navigation" but the value
  actually gets set to something completely different. It's set to the
  current internal `UrlTree` of the Router at the time navigation
  occurs.

With this change, there is no exact replacement for the old value of
`initialUrl` because it was enver intended to be exposed.
`Router.url` is likely the best replacement for this.
In more specific use-cases, tracking the `finalUrl` between successful
navigations can also be used as a replacement.
@atscott
Copy link
Contributor Author

atscott commented Feb 1, 2022

TGP

@dylhunn
Copy link
Contributor

dylhunn commented Feb 3, 2022

@atscott Is this ready for merge? Just asking since it looks green, but is not marked for merge.

@atscott
Copy link
Contributor Author

atscott commented Feb 3, 2022 •
edited
Loading

@dylhunn It's pending one change in g3 that I think could break with this change (even though there's no test that fails)

@dylhunn
Copy link
Contributor

dylhunn commented Feb 3, 2022

@atscott Sounds good, looks like that g3 change was just 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 risk: medium labels Feb 3, 2022
@atscott
Copy link
Contributor Author

atscott commented Feb 3, 2022

caretaker note: please merge and sync on its own. While TGP was green and uses of initialUrl were investigated, there's still a risk of needing to roll back.

@dylhunn
Copy link
Contributor

dylhunn commented Feb 3, 2022

caretaker note: please merge and sync on its own. While TGP was green and uses of initialUrl were investigated, there's still a risk of needing to roll back.

Thanks Andrew, I'll merge this afternoon and sync separately tomorrow morning.

@dylhunn
Copy link
Contributor

dylhunn commented Feb 4, 2022

This PR was merged into the repository by commit 64f837d.

@dylhunn dylhunn closed this in 64f837d Feb 4, 2022
@dylhunn
Copy link
Contributor

dylhunn commented Feb 4, 2022 •
edited
Loading

@atscott I think this is causing my TAP Train smoke test to fail: link

There was another PR in that run, I'll know for sure in an hour or so.

EDIT: The other PR was green, so it's likely this change.

@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 Mar 7, 2022
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this pull request Apr 8, 2022
@atscott @josmar-crwdstffng
fix(router): Update Navigation#initialUrl to match documentation an…
02409f3 
…d reality (angular#43863)

BREAKING CHANGE:

* The type of `initialUrl` is set to `string|UrlTree` but in reality,
  the `Router` only sets it to a value that will always be `UrlTree`
* `initialUrl` is documented as "The target URL passed into the
  `Router#navigateByUrl()` call before navigation" but the value
  actually gets set to something completely different. It's set to the
  current internal `UrlTree` of the Router at the time navigation
  occurs.

With this change, there is no exact replacement for the old value of
`initialUrl` because it was enver intended to be exposed.
`Router.url` is likely the best replacement for this.
In more specific use-cases, tracking the `finalUrl` between successful
navigations can also be used as a replacement.

PR Close angular#43863
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: router breaking changes cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: medium target: major This PR is targeted for the next major release
Projects
None yet
Milestone
v14-candidates
Development

Successfully merging this pull request may close these issues.
5 participants
@atscott @dylhunn @alxhub @AndrewKushnir @jessicajaniuk