docs(router): Clearly document how to retrieve all params in the router tree #40306

atscott · 2021-01-04T21:49:06Z

This commit documents how to add a helper function which combines all the params
in the router state tree into a single object. It provides a starting point for
developers to reference if they require a more fine-tuned approach.

Fixes #11023

petebacondarwin

As discussed recently in a meeting, we should try to avoid adding methods to classes, since they cannot be tree shaken. I would propose that this is made a free-standing helper function if possible. I think there is precedence here with things like convertToParamMap() for instance.

Further, I wonder if it would be enough to provide something like

forEachChild<T>(root: ActivatedRouteSnapshot, cb: (snapshot: ActivatedRouteSnapshot) => T): T[];

which could then be used to walk the url tree, avoiding us having to implement lots of different similar helpers to this one?

packages/router/src/router_state.ts

atscott · 2021-01-04T22:28:06Z

As discussed recently in a meeting, we should try to avoid adding methods to classes, since they cannot be tree shaken. I would propose that this is made a free-standing helper function if possible. I think there is precedence here with things like convertToParamMap() for instance.

I somehow missed that conversation about tree shaking class methods. I wanted to do this as a helper function in the first place, but was concerned with discoverability. I added documentation to the params property of ActivatedRouteSnapshot to hopefully help with this.

Further, I wonder if it would be enough to provide something like
forEachChild<T>(root: ActivatedRouteSnapshot, cb: (snapshot: ActivatedRouteSnapshot) => T): T[];
which could then be used to walk the url tree, avoiding us having to implement lots of different similar helpers to this one?

I actually feel like that would be too generic and could be implemented by developers easily enough if needed. I added this one to specifically address #11023, which is one of the top upvoted issues for the router package and I haven't seen anything similar for other tree items (data would pretty much be the only similar request for something like this - see flattenInherited). This helper function is also arguably something that should be handled in user-code as well, but again, this is a common enough request that it might warrant an implementation in the router itself. The other possible resolution would be a complete example of doing this in the documentation.

mary-poppins · 2021-01-04T22:45:54Z

You can preview 6b47563 at https://pr40306-6b47563.ngbuilds.io/.
You can preview f363823 at https://pr40306-f363823.ngbuilds.io/.

mary-poppins · 2021-01-04T22:58:34Z

You can preview e9df5d9 at https://pr40306-e9df5d9.ngbuilds.io/.

petebacondarwin

Reviewed-for: public-api

LGTM! Nice work @atscott .

packages/router/src/router_state.ts

AndrewKushnir

LGTM 👍

Reviewed-for: public-api

atscott · 2021-01-05T18:01:20Z

Note to public API reviewers (@AndrewKushnir @IgorMinar @jelbourn @alxhub) after discussion with @petebacondarwin:
Please review this with special consideration for if this is something we want to provide out of the box or if it should be resolved with more complete documentation. Here are a few of the key points:

The helper itself is a simple tree traversal that could be implemented by end-users
This is a highly requested feature so maybe that is an indication to its worth. Because it's a standalone helper, it doesn't incur any cost in code size if its unused
Would providing this type of helper open the door for requests to start providing similar helper functions and take us down a path where we cannot keep up with requests? Or is this a path we do want to take -- providing helper functions when it becomes clear that the need is there?
The complete helper could instead be moved to the documentation of the params property in ActivatedRouteSnapshot as an example implementation

mary-poppins · 2021-01-05T18:08:23Z

You can preview 80e20e6 at https://pr40306-80e20e6.ngbuilds.io/.

IgorMinar

I've read through your excellent comment at #11023 (comment) and other comments on that issue it seems that we can't agree on a best way to reconcile duplicates in the params tree. Several of the commenters used a simple depth-first approach ignoring siblings, but your implementation does take into account the siblings.

Additionally several of the users expect the collected params to be updated when child params are updated, so even with your helper you still need to write code that respond to the param changes.

It's a bit surprising that many developers write parent routes that need info about params coming from their child routes. It would be good to understand why this is the case before we jump into a solution in a form of a helper.

So my recommendation is not to add a helper until we clearly understand the requirements. In the meantime we should document in the API docs how to traverse the route tree and collect params or data.

(I've noticed the whole discussion about how this should work, after I reviewed the public api and left a few comments - I'll post those just as an fyi for future reference, but you can ignore them if we ultimately decide to go with a docs update instead.)

IgorMinar · 2021-01-05T19:42:09Z

packages/router/src/router_state.ts

+ *
+ * @publicApi
+ */
+export function getAllParamsInTree(activatedRouteSnapshot: ActivatedRouteSnapshot): Params {


can you please document the param and return types using jsdoc tags?

IgorMinar · 2021-01-05T19:51:03Z

packages/router/src/router_state.ts

+ *
+ * @publicApi
+ */
+export function getAllParamsInTree(activatedRouteSnapshot: ActivatedRouteSnapshot): Params {


Rather than using Params type, which is defined as [key: string]: any, could you consider a more type-safe alternative? Could for example ParamsMap be a suitable return type? If not, then [key: string]: string|undefined or [key: string]: unknown would be a type safe alternative.

I've also noticed that the strategy to resolve duplicates is "last dupe wins", which seems at odds with how ParamMap#get works. Is this intentional? If so, can you please clarify that in the docs where you talk about overriding.

Updated the type to [key: string]: string|string[] which I believe is the most accurate. I'm a bit torn on how I feel about it - I feel a bit like it should stay in-line with the type of snapshot.params even if it isn't very type safe.

As for the duplicates, the "last dupe wins" is actually not truly at odds with the ParamMap#get. The array there is in the context of actual array type query params or matrix params rather than duplicate parameter keys that may have appeared in the tree. The "last dupe wins" strategy is also used when inheriting parameters using paramsInheritanceStrategy: 'always' (https://github.com/angular/angular/blob/master/packages/router/src/router_state.ts#L243).

As for documentation and testing the override behavior, I actually wanted to specifically leave out what the expectation should be. I meant it to be more of "this behavior is more or less undefined (even though it's predictable) - duplicate keys will overwrite each other." I could still document it though if you'd prefer. I'm fine going that direction as well and stating that users should provide their own implementation if they want different behavior, following our helper as an example.

packages/router/src/router_state.ts

IgorMinar · 2021-01-05T20:27:56Z

packages/router/test/router_state.spec.ts

+           imports: [RouterTestingModule.withRoutes([{
+             path: '',
+             component: OutletCmp,
+             children: [{


this test case doesn't exercise the merging behavior. Could you add that please?

See thread above about testing/documenting duplicate keys. I could still add a test for it if we want to concretely define the behavior.

At the end of the day if this api has a sufficient usage, the implementation will become the spec, so if we do publish this as an api now or in the future, we should add a test at that point.

This seems to be moot at the moment because it seems that we are leaning towards just extending docs with a snippet of code for collecting params.

But in general, we can't rely on a behavior not being a public api just because we intentionally didn't document it, so for this reason it's usually good to have unit tests just to give us an early warning of a potential breaking change should we decide to change the behavior in the future.

IgorMinar

See comment in #40306 (review)

Reviewed-for: public-api

atscott

@IgorMinar Thanks for the detailed review.

we can't agree on a best way to reconcile duplicates in the params tree.

Yea, I don't think this is really something that happens frequently or it's at least not something most users are concerned with. Note that the duplicates are handled in approximately the same way as paramsInheritanceStrategy (the "approximate" part is that I've included named outlets, which might also have duplicate param keys).

Several of the commenters used a simple depth-first approach ignoring siblings, but your implementation does take into account the siblings.

Indeed, most of the comments there are using firstChild only to traverse the tree. This would get only the params from the primary outlet and exclude named outlets. I don't know if this is really something that could possibly be agreed on and really depends on the end-user needs. Including the params from named outlets would pretty rarely affect the result. I don't necessarily think that they've been excluded in the comments deliberately - it's just that they almost never matter the params on named outlets are probably even less likely to matter for the use-cases that want all params from the root. But I do think it would be more correct to include them.

Additionally several of the users expect the collected params to be updated when child params are updated, so even with your helper you still need to write code that respond to the param changes.

I don't think this should be addressed in that way. The issue is really that people need something to subscribe to in order to get updated params. I don't think it matters if it's the router navigation events or the ActivatedRoute.params, there just needs to be an easy, defined/documented way to do it.

It's a bit surprising that many developers write parent routes that need info about params coming from their child routes. It would be good to understand why this is the case before we jump into a solution in a form of a helper.

I don't think it's actually the "parent routes" that need child params but something more along the lines of this:
I have a url /webroot/account/:accountId/post/:postId. I want to show the account name and/or post name in the floating header. This floating header would live on the root component, and there's currently no documented way to get params deep in the tree from a service/component higher up. It's sometimes difficult to get the data you want from a component that can be placed anywhere when the data is only available if the component is placed in a specific place.

So my recommendation is not to add a helper until we clearly understand the requirements. In the meantime we should document in the API docs how to traverse the route tree and collect params or data.

I'm starting to lean towards the documentation route rather than providing an importable implementation. Maybe a copy-paste-able implementation with a good description of expected behavior would be better, given that there isn't necessarily a 100% "correct" way to do this; the implementation really depends on what the end-user is looking for.

WDYT?

packages/router/src/router_state.ts

atscott · 2021-01-05T22:29:14Z

packages/router/src/router_state.ts

+ *
+ * @publicApi
+ */
+export function getAllParamsInTree(activatedRouteSnapshot: ActivatedRouteSnapshot): Params {


Updated the type to [key: string]: string|string[] which I believe is the most accurate. I'm a bit torn on how I feel about it - I feel a bit like it should stay in-line with the type of snapshot.params even if it isn't very type safe.

As for the duplicates, the "last dupe wins" is actually not truly at odds with the ParamMap#get. The array there is in the context of actual array type query params or matrix params rather than duplicate parameter keys that may have appeared in the tree. The "last dupe wins" strategy is also used when inheriting parameters using paramsInheritanceStrategy: 'always' (https://github.com/angular/angular/blob/master/packages/router/src/router_state.ts#L243).

As for documentation and testing the override behavior, I actually wanted to specifically leave out what the expectation should be. I meant it to be more of "this behavior is more or less undefined (even though it's predictable) - duplicate keys will overwrite each other." I could still document it though if you'd prefer. I'm fine going that direction as well and stating that users should provide their own implementation if they want different behavior, following our helper as an example.

atscott · 2021-01-05T22:30:30Z

packages/router/test/router_state.spec.ts

+           imports: [RouterTestingModule.withRoutes([{
+             path: '',
+             component: OutletCmp,
+             children: [{


See thread above about testing/documenting duplicate keys. I could still add a test for it if we want to concretely define the behavior.

atscott · 2021-01-05T22:36:31Z

packages/router/src/router_state.ts

+ *
+ * @publicApi
+ */
+export function getAllParamsInTree(activatedRouteSnapshot: ActivatedRouteSnapshot): Params {


mary-poppins · 2021-01-05T23:15:49Z

You can preview ca0f417 at https://pr40306-ca0f417.ngbuilds.io/.

IgorMinar

It's a bit surprising that many developers write parent routes that need info about params coming from their child routes. It would be good to understand why this is the case before we jump into a solution in a form of a helper.

I don't think it's actually the "parent routes" that need child params but something more along the lines of this:
I have a url /webroot/account/:accountId/post/:postId. I want to show the account name and/or post name in the floating header. This floating header would live on the root component, and there's currently no documented way to get params deep in the tree from a service/component higher up. It's sometimes difficult to get the data you want from a component that can be placed anywhere when the data is only available if the component is placed in a specific place.

I see, if you are trying to extract this from outside of the outlet tree then I can see how it could be puzzlingly difficult. If however the service depends on some occasionally present param value, then I would write the code in a way where the component that extracts the param, informs the global state of the new value.

In a hierarchical route config, the ownership of params is delegated to nested routes on purpose to enable decoupling. In the cases discussed it seems that this "local" state is being accessed from top-level context which while possible is not ideal.

So my recommendation is not to add a helper until we clearly understand the requirements. In the meantime we should document in the API docs how to traverse the route tree and collect params or data.

I'm starting to lean towards the documentation route rather than providing an importable implementation. Maybe a copy-paste-able implementation with a good description of expected behavior would be better, given that there isn't necessarily a 100% "correct" way to do this; the implementation really depends on what the end-user is looking for.

WDYT?

works for me.

IgorMinar · 2021-01-05T23:11:13Z

packages/router/test/router_state.spec.ts

+           imports: [RouterTestingModule.withRoutes([{
+             path: '',
+             component: OutletCmp,
+             children: [{


At the end of the day if this api has a sufficient usage, the implementation will become the spec, so if we do publish this as an api now or in the future, we should add a test at that point.

This seems to be moot at the moment because it seems that we are leaning towards just extending docs with a snippet of code for collecting params.

But in general, we can't rely on a behavior not being a public api just because we intentionally didn't document it, so for this reason it's usually good to have unit tests just to give us an early warning of a potential breaking change should we decide to change the behavior in the future.

jelbourn · 2021-01-06T00:21:46Z

IMO saying we'd just add this to the docs is a bit hand-wavy. I think either approach here has a problem with respect to discoverability. Where would you see either approach (helper function or example) to fit into the bigger router docs?

atscott · 2021-01-06T01:56:12Z

@jelbourn I was planning to put it directly on the ActivatedRouteSnapshot.params documentation like this:

atscott · 2021-01-06T21:09:33Z

@jelbourn @IgorMinar - I've made this a docs-only change as a fixup commit so it can be dropped (and the code change can be restored) if this is not a satisfactory solution. I feel like this is a happy medium and is easily discoverable - it'll show up on aio and in the hover info for the type. PTAL. I'll update the commit message if this version of the change is approved.

mary-poppins · 2021-01-06T21:18:58Z

You can preview 2cbfc90 at https://pr40306-2cbfc90.ngbuilds.io/.

jelbourn

LGTM, but it would be good to change the commit message/PR title to reflect the new nature of the change (I'd probably just squash the commits)

…er tree This commit documents how to add a helper function which combines all the params in the router state tree into a single object. It provides a starting point for developers to reference if they require a more fine-tuned approach. Fixes angular#11023

Feedback was addressed.

mary-poppins · 2021-01-08T18:30:33Z

You can preview 2119dfe at https://pr40306-2119dfe.ngbuilds.io/.

…er tree (#40306) This commit documents how to add a helper function which combines all the params in the router state tree into a single object. It provides a starting point for developers to reference if they require a more fine-tuned approach. Fixes #11023 PR Close #40306

angular-automatic-lock-bot · 2021-02-08T16:44:54Z

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

atscott added action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release labels Jan 4, 2021

google-cla bot added the cla: yes label Jan 4, 2021

pullapprove bot requested review from AndrewKushnir, IgorMinar, jelbourn and petebacondarwin January 4, 2021 21:49

pullapprove bot added comp: docs/api area: router labels Jan 4, 2021

ngbot bot modified the milestone: Backlog Jan 4, 2021

petebacondarwin reviewed Jan 4, 2021

View reviewed changes

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

pullapprove bot requested a review from alxhub January 4, 2021 22:07

atscott force-pushed the allTreeParams branch 2 times, most recently from f363823 to e9df5d9 Compare January 4, 2021 22:45

atscott added the aio: preview label Jan 4, 2021

atscott mentioned this pull request Jan 4, 2021

Simplify current ActivatedRoute access from services or non active components #15004

Closed

petebacondarwin approved these changes Jan 5, 2021

View reviewed changes

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

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

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

atscott force-pushed the allTreeParams branch from e9df5d9 to 6dbe9e3 Compare January 5, 2021 17:48

atscott changed the title ~~feat(router): Add helper to ActivatedRouteSnapshot to retrieve all …~~ feat(router): Add helper function to retrieve all tree params Jan 5, 2021

atscott force-pushed the allTreeParams branch from 6dbe9e3 to 80e20e6 Compare January 5, 2021 17:58

AndrewKushnir approved these changes Jan 5, 2021

View reviewed changes

IgorMinar suggested changes Jan 5, 2021

View reviewed changes

pullapprove bot requested a review from IgorMinar January 5, 2021 21:14

IgorMinar previously requested changes Jan 5, 2021

View reviewed changes

atscott commented Jan 5, 2021

View reviewed changes

atscott force-pushed the allTreeParams branch from 80e20e6 to ca0f417 Compare January 5, 2021 23:02

IgorMinar reviewed Jan 5, 2021

View reviewed changes

pullapprove bot requested a review from jessicajaniuk January 6, 2021 15:04

atscott force-pushed the allTreeParams branch 2 times, most recently from 8729f10 to 2cbfc90 Compare January 6, 2021 21:08

atscott requested a review from IgorMinar January 6, 2021 23:25

jessicajaniuk approved these changes Jan 7, 2021

View reviewed changes

jelbourn approved these changes Jan 7, 2021

View reviewed changes

atscott force-pushed the allTreeParams branch from 2cbfc90 to 2119dfe Compare January 8, 2021 18:17

atscott changed the title ~~feat(router): Add helper function to retrieve all tree params~~ docs(router): Clearly document how to retrieve all params in the router tree Jan 8, 2021

atscott removed request for IgorMinar and alxhub January 8, 2021 19:47

atscott closed this in b36bece Jan 8, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(router): Clearly document how to retrieve all params in the router tree #40306

docs(router): Clearly document how to retrieve all params in the router tree #40306

atscott commented Jan 4, 2021 •

edited

petebacondarwin left a comment •

edited

atscott commented Jan 4, 2021

mary-poppins commented Jan 4, 2021

mary-poppins commented Jan 4, 2021

petebacondarwin left a comment

AndrewKushnir left a comment

atscott commented Jan 5, 2021

mary-poppins commented Jan 5, 2021

IgorMinar left a comment

IgorMinar Jan 5, 2021

atscott Jan 5, 2021

IgorMinar Jan 5, 2021

atscott Jan 5, 2021

IgorMinar Jan 5, 2021

atscott Jan 5, 2021

IgorMinar Jan 5, 2021

IgorMinar left a comment

atscott left a comment •

edited

atscott Jan 5, 2021

atscott Jan 5, 2021

atscott Jan 5, 2021

mary-poppins commented Jan 5, 2021

IgorMinar left a comment

IgorMinar Jan 5, 2021

jelbourn commented Jan 6, 2021

atscott commented Jan 6, 2021

atscott commented Jan 6, 2021 •

edited

mary-poppins commented Jan 6, 2021

jelbourn left a comment

mary-poppins commented Jan 8, 2021

angular-automatic-lock-bot bot commented Feb 8, 2021

docs(router): Clearly document how to retrieve all params in the router tree #40306

docs(router): Clearly document how to retrieve all params in the router tree #40306

Conversation

atscott commented Jan 4, 2021 • edited

petebacondarwin left a comment • edited

Choose a reason for hiding this comment

atscott commented Jan 4, 2021

mary-poppins commented Jan 4, 2021

mary-poppins commented Jan 4, 2021

petebacondarwin left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

atscott commented Jan 5, 2021

mary-poppins commented Jan 5, 2021

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

atscott left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Jan 5, 2021

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented Jan 6, 2021

atscott commented Jan 6, 2021

atscott commented Jan 6, 2021 • edited

mary-poppins commented Jan 6, 2021

jelbourn left a comment

Choose a reason for hiding this comment

mary-poppins commented Jan 8, 2021

angular-automatic-lock-bot bot commented Feb 8, 2021

atscott commented Jan 4, 2021 •

edited

petebacondarwin left a comment •

edited

atscott left a comment •

edited

atscott commented Jan 6, 2021 •

edited