feat(router): Add `Route.title` with a configurable `TitleStrategy` by atscott · Pull Request #43307 · angular/angular

atscott · 2021-09-01T18:13:34Z

This commit provides a service, PageTitleStrategy for setting
the document page title after a successful router navigation.

Users can provide custom strategies by extending TitleStrategy and
adding a provider which overrides it.

The strategy takes advantage of the pre-existing data and resolve concepts
in the Router implementation:

We can copy the Route.title into data/resolve in a
non-breaking way by using a symbol as the key. This ensures that we
do not have any collisions with pre-existing property names. By using
data and resolve, we do not have to add anything more to
the router navigation pipeline to support this feature.

resolves #7630

reviewer notes: thoughts/design doc

AndrewKushnir

@atscott looks great, just left some comments 👍

packages/router/src/page_title_strategy.ts

packages/router/test/page_title_strategy_spec.ts

atscott · 2021-10-26T16:33:09Z

For public API reviewers, copying in the considerations for rolling this change out:

Because we’re using the existing Route.data property, there is a potential for developers to already have something named pageTitle there. If we add this new functionality by default, this would result in a change in behavior for those applications (the browser’s document title will be updated where it wasn’t previously).

On the other hand, by not adding this as default behavior in the Router, there could be an issue with feature discovery. Developers might read about the pageTitle in some blog or another application’s code, add it to their own Routes, only to find that this doesn’t work. Developers need to also register the provider which handles the setting of the page title.

These two issues are both important to consider, but are generally at odds with each other. There are a couple options that we can explore here:

Add the PageTitle service as a default provider in the Router, then
1. run TGP, and see what failures occur. This likely won’t work well because it wouldn’t be something that’s covered in test assertions
2. Update the service to throw an error if there is a pageTitle property in the data. This should give us a much better indication of how many pre-existing pageTitle properties there are.
3. Same as (b) above, but use a more verbose property name that’s less likely to be used, i.e. routerPageTitle
Add a devmode only warning that checks if pageTitle exists on Route data but there is no provider for the service. This would also be problematic because it would produce warnings for applications which have specifically disabled the service or provided one that reads a different property.
Roll out the feature in a minor version as opt-in, then add it as a default provider in the next major version and mark it as a breaking change. This is a nice in-between but also would result in applications having some unnecessary providers for the service in their applications. For this, we could
1. Ignore it, as the extra provider wouldn’t affect application behavior.
2. Create a migration to remove the provider.

jessicajaniuk

LGTM 🍪

reviewed-for: public-api

packages/router/test/page_title_strategy_spec.ts

mary-poppins · 2021-10-29T11:29:08Z

You can preview 9ee31bd at https://pr43307-9ee31bd.ngbuilds.io/.
You can preview fb7c264 at https://pr43307-fb7c264.ngbuilds.io/.
You can preview 79d990a at https://pr43307-79d990a.ngbuilds.io/.
You can preview a513187 at https://pr43307-a513187.ngbuilds.io/.
You can preview 4290baf at https://pr43307-4290baf.ngbuilds.io/.
You can preview 4718823 at https://pr43307-4718823.ngbuilds.io/.

petebacondarwin

Take a look at the AIO preview. I think the API docs need more content to explain how these classes are used (or at least a link to the guide). Also in the guide the example snippet given by the page-title region looks a bit strange since the constructor block has no context. I think it would be better to include the class in the snippet too.

The override modifier on the setTitle() method appears to have exposed a bug in our doc-gen: https://pr43307-4718823.ngbuilds.io/api/router/BrowserPageTitleStrategy#override.
For some reason the override is being treated as a property in its own right!!
I'll look into this.

packages/router/src/page_title_strategy.ts

mary-poppins · 2021-10-29T16:25:46Z

You can preview 2b263a2 at https://pr43307-2b263a2.ngbuilds.io/.

mary-poppins · 2021-10-29T16:49:53Z

You can preview 70f9756 at https://pr43307-70f9756.ngbuilds.io/.

AndrewKushnir

Looks great, thanks for your work on this feature 👍

AndrewKushnir · 2022-01-27T03:11:15Z

goldens/public-api/router/testing/testing.md

Since this is a public API, it might be confusing to have both titleStrategy and defaultTitleStrategy as arguments. Would it be possible to have an intermediate factory function in the RouterTestingModule providers that would select custom or default strategy and pass only one to this function?

I had similar question for the setupRouter function, but it's not public, so this is not a concern there.

dylhunn

reviewed-for: public-api

This commit provides a service, `PageTitleStrategy` for setting the document page title after a successful router navigation. Users can provide custom strategies by extending `TitleStrategy` and adding a provider which overrides it. The strategy takes advantage of the pre-existing `data` and `resolve` concepts in the Router implementation: We can copy the `Route.title` into `data`/`resolve` in a non-breaking way by using a `symbol` as the key. This ensures that we do not have any collisions with pre-existing property names. By using `data` and `resolve`, we do not have to add anything more to the router navigation pipeline to support this feature. resolves angular#7630

…tegy`

AndrewKushnir

👍

AndrewKushnir

Reviewed-for: public-api, size-tracking

alxhub

Reviewed-for: size-tracking

addressed

jessicajaniuk · 2022-01-27T22:02:35Z

This PR was merged into the repository by commit 910de8b.

angular-automatic-lock-bot · 2022-03-04T16:12:11Z

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.}

…ngular#43307) This commit provides a service, `PageTitleStrategy` for setting the document page title after a successful router navigation. Users can provide custom strategies by extending `TitleStrategy` and adding a provider which overrides it. The strategy takes advantage of the pre-existing `data` and `resolve` concepts in the Router implementation: We can copy the `Route.title` into `data`/`resolve` in a non-breaking way by using a `symbol` as the key. This ensures that we do not have any collisions with pre-existing property names. By using `data` and `resolve`, we do not have to add anything more to the router navigation pipeline to support this feature. resolves angular#7630 PR Close angular#43307

atscott added the target: minor This PR is targeted for the next minor release label Sep 1, 2021

google-cla bot added the cla: yes label Sep 1, 2021

jessicajaniuk added the area: router label Sep 1, 2021

ngbot bot added this to the Backlog milestone Sep 1, 2021

atscott force-pushed the routertitleservice branch 3 times, most recently from 40248ad to 79d990a Compare September 1, 2021 21:17

atscott force-pushed the routertitleservice branch 2 times, most recently from a513187 to 4290baf Compare October 15, 2021 23:40

atscott marked this pull request as ready for review October 16, 2021 00:03

pullapprove bot requested review from AndrewKushnir, alxhub, jelbourn and zarend October 16, 2021 00:03

AndrewKushnir reviewed Oct 25, 2021

View reviewed changes

pullapprove bot requested a review from jessicajaniuk October 25, 2021 18:57

atscott force-pushed the routertitleservice branch 2 times, most recently from 488c842 to 4718823 Compare October 25, 2021 20:04

jessicajaniuk approved these changes Oct 28, 2021

View reviewed changes

packages/router/test/page_title_strategy_spec.ts Outdated Show resolved Hide resolved

pullapprove bot requested a review from IgorMinar October 28, 2021 22:44

petebacondarwin added the aio: preview label Oct 29, 2021

petebacondarwin previously requested changes Oct 29, 2021

View reviewed changes

packages/router/src/page_title_strategy.ts Outdated Show resolved Hide resolved

packages/router/src/page_title_strategy.ts Outdated Show resolved Hide resolved

atscott force-pushed the routertitleservice branch from ccbaaf4 to 2b263a2 Compare October 29, 2021 16:16

atscott force-pushed the routertitleservice branch from 2b263a2 to 70f9756 Compare October 29, 2021 16:40

atscott force-pushed the routertitleservice branch 2 times, most recently from 9850189 to 0f4198b Compare October 29, 2021 19:15

atscott force-pushed the routertitleservice branch from d42de88 to 2c41125 Compare January 19, 2022 22:06

atscott changed the title ~~feat(router): Provide a service for setting the document page title~~ feat(router): Add Route.title with a configurable TitleStrategy Jan 19, 2022

AndrewKushnir reviewed Jan 27, 2022

View reviewed changes

pullapprove bot requested a review from AndrewKushnir January 27, 2022 03:13

dylhunn approved these changes Jan 27, 2022

View reviewed changes

atscott added 4 commits January 27, 2022 09:25

fixup! feat(router): Add Route.title with a configurable `TitleStra…

0af37b9

…tegy`

fixup! feat(router): Add Route.title with a configurable `TitleStra…

9c0d652

…tegy`

fixup! feat(router): Add Route.title with a configurable `TitleStra…

df64339

…tegy`

atscott force-pushed the routertitleservice branch from 2a869c1 to df64339 Compare January 27, 2022 17:25

AndrewKushnir approved these changes Jan 27, 2022

View reviewed changes

pullapprove bot requested a review from AndrewKushnir January 27, 2022 17:31

AndrewKushnir approved these changes Jan 27, 2022

View reviewed changes

pullapprove bot requested review from dylhunn and jessicajaniuk January 27, 2022 17:31

alxhub approved these changes Jan 27, 2022

View reviewed changes

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 Jan 27, 2022

atscott removed request for dylhunn, jelbourn, jessicajaniuk, petebacondarwin and zarend January 27, 2022 21:58

atscott removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jan 27, 2022

jessicajaniuk closed this in 910de8b Jan 27, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 4, 2022

Conversation

atscott commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atscott commented Oct 26, 2021

Uh oh!

jessicajaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mary-poppins commented Oct 29, 2021

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mary-poppins commented Oct 29, 2021

Uh oh!

mary-poppins commented Oct 29, 2021

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

dylhunn left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk commented Jan 27, 2022

Uh oh!

angular-automatic-lock-bot bot commented Mar 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

atscott commented Sep 1, 2021 •

edited

Loading