Skip to content

docs(router): Deprecate public members of Router that are meant to be configured elsewhere #48006

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 2 commits into from
Closed

docs(router): Deprecate public members of Router that are meant to be configured elsewhere #48006

wants to merge 2 commits into from

Conversation

atscott
Copy link
Contributor

@atscott atscott commented Nov 9, 2022

None of the public properties of the Router are meant to be writeable. They should all be configured using other methods, all of which have been documented.

The following strategies are meant to be configured by registering the application strategy in DI via the providers in the root NgModule or bootstrapApplication:

  • routeReuseStrategy
  • titleStrategy
  • urlHandlingStrategy

The following options are meant to be configured using the options available in RouterModule.forRoot or provideRouter.

  • onSameUrlNavigation
  • paramsInheritanceStrategy
  • urlUpdateStrategy
  • canceledNavigationResolution

The following options are available in RouterModule.forRoot but not available in provideRouter:

  • malformedUriErrorHandler - This was found to not be used anywhere internally.
  • errorHandler - Developers can instead subscribe to Router.events and filter for NavigationError.

@atscott atscott added the target: minor This PR is targeted for the next minor release label Nov 9, 2022
@ngbot ngbot bot added this to the Backlog milestone Nov 9, 2022
@atscott atscott force-pushed the deprecatePublicRouterFields branch from c671040 to fa45fe1 Compare November 9, 2022 20:31
@angular-robot angular-robot bot added the detected: deprecation PR contains a commit with a deprecation label Nov 9, 2022
AndrewKushnir
AndrewKushnir reviewed Nov 9, 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.

LGTM, left a few comments about additional references to replacement functions.

AndrewKushnir
AndrewKushnir approved these changes Nov 9, 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

@pullapprove pullapprove bot requested a review from dylhunn November 9, 2022 21:32
@atscott atscott added state: blocked action: global presubmit The PR is in need of a google3 global presubmit labels Nov 10, 2022
@atscott
Copy link
Contributor Author

atscott commented Nov 10, 2022

Blocked on review of global presubmit results.

@atscott atscott removed state: blocked action: global presubmit The PR is in need of a google3 global presubmit labels Nov 10, 2022
@atscott
Copy link
Contributor Author

atscott commented Nov 10, 2022

Removing the blocked label. The TGP tested removing these APIs from the public router members (by adding the @internal annotation) to see what failed.

By far the most common and concerning pattern that was identified was attempting to force a refresh with router.routeReuseStrategy.shouldReuseRoute = () => false. Often, this was not accompanied by a reset to the original value so the effect was that after loading a component with this line of code, the application would always destroy and recreate components during navigations.

The above was often accompanied with, router.onSameUrlNavigation = 'reload'.

It's clear that there is a missing feature in the router to better support hard refreshes. I don't think the current solutions (modifying the properties on the router before a navigation) are the correct ones. We should continue with the deprecation and follow up with creating new APIs to more easily accomplish these tasks.

pkozlowski-opensource
pkozlowski-opensource approved these changes Nov 11, 2022
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource 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 atscott removed request for alxhub and dylhunn November 11, 2022 17:38
@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Nov 15, 2022
@atscott
docs(router): Deprecate public members of Router that are meant to be…
71a524d 
… configured elsewhere

None of the public properties of the `Router` are meant to be writeable.
They should all be configured using other methods, all of which have been
documented.

DEPRECATED: router writable properties

The following strategies are meant to be configured by registering the
application strategy in DI via the `providers` in the root `NgModule` or
`bootstrapApplication`:
* `routeReuseStrategy`
* `titleStrategy`
* `urlHandlingStrategy`

The following options are meant to be configured using the options
available in `RouterModule.forRoot` or `provideRouter`.
* `onSameUrlNavigation`
* `paramsInheritanceStrategy`
* `urlUpdateStrategy`
* `canceledNavigationResolution`

The following options are available in `RouterModule.forRoot` but not
available in `provideRouter`:
* `malformedUriErrorHandler` - This was found to not be used anywhere
  internally.
* `errorHandler` - Developers can instead subscribe to `Router.events`
  and filter for `NavigationError`.
@atscott
fixup! docs(router): Deprecate public members of Router that are mean…
25d6694 
…t to be configured elsewhere
@atscott atscott force-pushed the deprecatePublicRouterFields branch from 4e8c442 to 25d6694 Compare November 17, 2022 15:32
@dylhunn
Copy link
Contributor

dylhunn commented Nov 17, 2022

This PR was merged into the repository by commit 0a8b8a6.

@dylhunn dylhunn closed this in 0a8b8a6 Nov 17, 2022
@kbrilla kbrilla mentioned this pull request Nov 30, 2022
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 Dec 18, 2022
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
@atscott @trekladyone
docs(router): Deprecate public members of Router that are meant to be…
404e010 
… configured elsewhere (angular#48006)

None of the public properties of the `Router` are meant to be writeable.
They should all be configured using other methods, all of which have been
documented.

DEPRECATED: router writable properties

The following strategies are meant to be configured by registering the
application strategy in DI via the `providers` in the root `NgModule` or
`bootstrapApplication`:
* `routeReuseStrategy`
* `titleStrategy`
* `urlHandlingStrategy`

The following options are meant to be configured using the options
available in `RouterModule.forRoot` or `provideRouter`.
* `onSameUrlNavigation`
* `paramsInheritanceStrategy`
* `urlUpdateStrategy`
* `canceledNavigationResolution`

The following options are available in `RouterModule.forRoot` but not
available in `provideRouter`:
* `malformedUriErrorHandler` - This was found to not be used anywhere
  internally.
* `errorHandler` - Developers can instead subscribe to `Router.events`
  and filter for `NavigationError`.

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

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker detected: deprecation PR contains a commit with a deprecation target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
4 participants
@atscott @dylhunn @pkozlowski-opensource @AndrewKushnir