perf(router-store): optimize selectQueryParams, selectQueryParam and selectFragment selectors

[markostanimirovic]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[x] Other... Please describe: Performance improvement

What is the current behavior?

selectQueryParams - returns query params from the last router state node
selectQueryParam - returns a query param from the last router state node
selectFragment - returns the fragment from the last router state node

What is the new behavior?

Optional chaining is introduced for all selectors.

selectQueryParams - returns query params from routerState.root
selectQueryParam - returns a query param from routerState.root
selectFragment - returns the fragment from routerState.root

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Actually, this can be a breaking change in test files (for selectQueryParams, selectQueryParam and selectFragment selectors) if mock router state is defined in the wrong way. But when it is used with Angular router, there are no breaking changes.

[ngrxbot]

Preview docs changes for 37e4a91 at https://previews.ngrx.io/pr2764-37e4a919/

[markostanimirovic]

@alex-okrushko @brandonroberts @timdeschryver
I observed the same issue with build on master branch. Steps:

1. rm -rf node_modules/
2. yarn
3. yarn ng build router-store

[brandonroberts]

@alex-okrushko @brandonroberts @timdeschryver
I observed the same issue with build on master branch. Steps:

1. rm -rf node_modules/
2. yarn
3. yarn ng build router-store

@markostanimirovic you have to use the --with-deps option to build router-store, as it depends on the store package being built first. There may be an issue with build-affected in CI when using --parallel

[timdeschryver]

Out of curiosity, I see this was first a perf change but how does this optimize the router selectors?
Doesn't the Optional Chaining operator compile to the same code?

Since this is a refactor, I would also revert the changes made to the spec to be sure we didn't break something. Or perhaps create a second test case for the root with the added fragment and params?

[markostanimirovic]

Out of curiosity, I see this was first a perf change but how does this optimize the router selectors?
Doesn't the Optional Chaining operator compile to the same code?

Since this is a refactor, I would also revert the changes made to the spec to be sure we didn't break something. Or perhaps create a second test case for the root with the added fragment and params?

@timdeschryver
Of course, optional chaining is not optimization, this is just an improvement to use new features :)

Optimized are three selectors: selectFragment, selectQueryParams and selectQueryParam to return the fragment/queryParam(s) from routerState.root instead of looping and returning fragment/queryParam(s) from the last router state tree node. I wrote this in PR description :)

About the breaking changes: this should not be a breaking change with Angular router, because fragment and query parameters are the same on root and any other child router tree node.

[markostanimirovic]

I can add tests with RouterTestingModule to be sure that everything is working fine.

[timdeschryver]

Optimized are three selectors: selectFragment, selectQueryParams and selectQueryParam to return the fragment/queryParam(s) from routerState.root instead of looping and returning fragment/queryParam(s) from the last router state tree node. I wrote this in PR description :)

My bad, I didn't read the description 😅.

But this change would also mean that the behavior is different (root vs current route), which you also pointed out.

So imho, we should either mark this as a breaking change or leave the implementation as is and create root selectors.
My opinion would be the second one, because these selectors are probably used in projects.
Thoughts?

I can add tests with RouterTestingModule to be sure that everything is working fine.

My comment about the spec was that originally it was verifying the child route, while it now validates the root route.
I was expecting the current tests to fail, but since I didn't read the PR's description I didn't know that this was intended.

[markostanimirovic]

So imho, we should either mark this as a breaking change or leave the implementation as is and create root selectors.
My opinion would be the second one, because these selectors are probably used in projects.
Thoughts?

Sure, let's wait for the opinion of others, and when the final decision is made, I'll fix this PR :)

[alex-okrushko]

So it is a breaking change, right?
For non-breaking changes the tests shouldn't change - just added.

Of course, optional chaining is not optimization, this is just an improvement to use new features :)

Optional chaining actually increases the generated code size. Unless we are reducing both sources code size AND generated code size, I wouldn't suggest to change it.

[markostanimirovic]

@alex-okrushko

So it is a breaking change, right?
For non-breaking changes the tests shouldn't change - just added.

mockData object from router_selectors.spec.ts should represent Angular Router state. However, it is wrongly defined, because queryParams and fragment properties should be the same in root as well as in firstChilds down to the router state tree. That's why I changed it, to be the real mock of Angular Router state. I can revert the tests. Old ones will work with new mockData object too.

Anyway, I can mark this PR as breaking change and change its title to feature or performance.
Or simply close it, if we don't want selectQueryParams and selectFragment optimization. What do you think? 🙂

Optional chaining actually increases the generated code size. Unless we are reducing both sources code size AND generated code size, I wouldn't suggest to change it.

Okay, I will revert optional chaining 👍

[brandonroberts]

@markostanimirovic What's left on this PR? Removal of the optional chaining?

[markostanimirovic]

@brandonroberts

Tim commented:

So imho, we should either mark this as a breaking change or leave the implementation as is and create root selectors.
My opinion would be the second one, because these selectors are probably used in projects.
Thoughts?

And I wanted to hear other thoughts on this and update this PR accordingly 🙂

My opinion is that we don't need new selectors for selecting fragment and query params from root, because with Angular router and default serializer root.fragment is equal to root.firstChild.fragment (the same is for query params).

Btw, optional chaining is removed now 🙂

cc @timdeschryver

[brandonroberts]

I don't think we need new selectors either, but this shouldn't be a breaking change functionally because they are equivalent as you stated. I'd still mark it as a breaking change just so people are aware of it.

[brandonroberts]

Changes LGTM. Just needs breaking changes to be added to commit message when merging

[timdeschryver]

We should also add a note to the migration docs

[markostanimirovic]

@brandonroberts @timdeschryver
I changed PR type from refactor to perf. Also, note is added to the migration guide 🙂

[timdeschryver]

Thanks for adding it to the migration guide