refactor(router): refactor and simplify router RxJS chains by martinsik · Pull Request #40290 · angular/angular

martinsik · 2021-01-01T11:05:33Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have 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
angular.io application / infrastructure changes
Other... Please describe:

Does this PR introduce a breaking change?

Yes
No

Other information

I refactored and simplified some RxJS usage in the Router package in order to reduce its size and increase performance. Unfortunatelly, I'm not able to verify the package size because node ./scripts/build/build-packages-dist.js fails on my system for some reason.

The biggest changes are these:

Some custom operators were wrapped with another function even though they didn't need source Observable.
from([x, y, z]) is the same as of(x, y, z) and using of() should be faster (I'll need to test this).

There are some other usecases that seem suspicious like using from().pipe(mergeAll()) should be the same as using just merge() but I'll need to be able to run build-packages-dist.js to make sure it generates smaller package.

Bundle size main-es2015 in cli-hello-world-lazy-rollup integration test dropped from expected 218507 to 217931.
Bundle size main-es2015 in cli-hello-world-lazy integration test dropped from expected 241837 to 241260.
So both bundles are cca 580 B smaller.

I'm not sure about the removed records in bundle.golden_symbols.json. It has probably something to do with this change https://github.com/angular/angular/pull/40290/files#diff-1706bbbdd4dda445171e80023d67b54f1aa77a457d6b81b6815239113fd34decR62-R66 but I'm not sure why.

I didn't make any change to existing tests because this should not break anything.

google-cla · 2021-01-01T11:05:49Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

atscott

presubmit

atscott · 2021-01-04T17:22:41Z

Removing these wrapper functions was a great change. Thanks!

atscott · 2021-01-04T17:28:16Z

I really like the removal of the taps. The Router rxjs chain is hard enough to follow without extra operators thrown in there.

atscott

reviewed-for: size-tracking

atscott · 2021-01-04T17:43:51Z

Adding blocked label, as #40029 has been waiting to go in and should be submitted first. That PR will cause merge conflicts with a couple changes here.

martinsik · 2021-01-04T23:07:10Z

I reverted take(1) back to first(). I know it wouldn't have any impact on package size but it would be more efficient. However, I forgot that first() will emit an error if the source Observable completes without emitting any value while take(1) won't. I answered the exact same question a while ago on StackOverflow and then completely forgot about it :).
All tests passed but it didn't occur to me that this use-case might not be tested and it really looks like it isn't (unless I missed it when looking into router.spec.ts).

I reverted also takeLast(1) with last() because that's the same issue.

atscott · 2021-01-04T23:53:07Z

I reverted take(1) back to first(). I know it wouldn't have any impact on package size but it would be more efficient. However, I forgot that first() will emit an error if the source Observable completes without emitting any value while take(1) won't. I answered the exact same question a while ago on StackOverflow and then completely forgot about it :).
All tests passed but it didn't occur to me that this use-case might not be tested and it really looks like it isn't (unless I missed it when looking into router.spec.ts).

I reverted also takeLast(1) with last() because that's the same issue.

@martinsik - I do like the idea of the change and appreciate you bringing it up! I just want to be a little careful about it since we know that changes to the router often cause unexpected failures. How about creating a separate PR for those changes (first() => take(1) and last() => takeLast(1))?

petebacondarwin

Reviewed-for: size-tracking

martinsik · 2021-01-05T12:11:33Z

@atscott I'll try doing that in a separate PR. I think I'll need to write tests cases where the guards don't emit (eg. return EMPTY) because I'm very suspicious that using take(1) will have different behavior than first(). So maybe it won't be even possible to use take(1) instead. Of course, unless the error from first() is caught somewhere down the chain.

IgorMinar

nice cleanup! thank you

Reviewed-for: size-tracking

atscott · 2021-01-08T18:28:43Z

Hi @martinsik, could you rebase this PR. I'd like to merge it early next week (or today if you're around to do the rebase).

jelbourn

LGTM

Reviewed-for: size-tracking

Refactor and simplifiy RxJS usage in the router package in order to reduce its size and increase performance.

martinsik · 2021-01-11T09:20:45Z

@atscott Rebased. However, I wasn't able to check package sizes because I can't run node ./scripts/build/build-packages-dist.js locally and the ci/angular: size task is not failing with the values in master :).

Refactor and simplifiy RxJS usage in the router package in order to reduce its size and increase performance. PR Close #40290

angular-automatic-lock-bot · 2021-02-11T16:34:58Z

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

pullapprove Bot requested a review from atscott January 1, 2021 11:05

pullapprove Bot added the area: router label Jan 1, 2021

ngbot Bot added this to the Backlog milestone Jan 1, 2021

google-cla Bot added the cla: no label Jan 1, 2021

martinsik force-pushed the router-refactor-observables branch from 284127d to 7cdc344 Compare January 1, 2021 11:15

google-cla Bot added cla: yes and removed cla: no labels Jan 1, 2021

martinsik force-pushed the router-refactor-observables branch from 7cdc344 to be037b0 Compare January 1, 2021 11:48

pullapprove Bot added the area: core Issues related to the framework runtime label Jan 1, 2021

martinsik force-pushed the router-refactor-observables branch 2 times, most recently from 1bbb7af to 861a400 Compare January 1, 2021 12:40

gbumanzordev approved these changes Jan 2, 2021

View reviewed changes

atscott approved these changes Jan 4, 2021

View reviewed changes

pullapprove Bot requested review from alxhub, atscott, jelbourn and petebacondarwin January 4, 2021 17:41

pullapprove Bot added the area: build & ci Related the build and CI infrastructure of the project label Jan 4, 2021

atscott added action: presubmit The PR is in need of a google3 presubmit target: patch This PR is targeted for the next patch release and removed area: build & ci Related the build and CI infrastructure of the project labels Jan 4, 2021

pullapprove Bot added area: build & ci Related the build and CI infrastructure of the project and removed action: presubmit The PR is in need of a google3 presubmit target: patch This PR is targeted for the next patch release labels Jan 4, 2021

atscott added the target: patch This PR is targeted for the next patch release label Jan 4, 2021

atscott approved these changes Jan 4, 2021

View reviewed changes

pullapprove Bot requested a review from IgorMinar January 4, 2021 17:42

atscott added action: presubmit The PR is in need of a google3 presubmit state: blocked labels Jan 4, 2021

atscott added the risk: medium label Jan 4, 2021

martinsik force-pushed the router-refactor-observables branch from 861a400 to bea520a Compare January 4, 2021 22:47

martinsik force-pushed the router-refactor-observables branch from bea520a to 351c610 Compare January 4, 2021 23:17

martinsik force-pushed the router-refactor-observables branch from 351c610 to 4ce60f7 Compare January 5, 2021 09:25

pullapprove Bot removed the area: build & ci Related the build and CI infrastructure of the project label Jan 5, 2021

martinsik force-pushed the router-refactor-observables branch from 4ce60f7 to 4bbd6c9 Compare January 5, 2021 09:44

pullapprove Bot added the area: build & ci Related the build and CI infrastructure of the project label Jan 5, 2021

petebacondarwin approved these changes Jan 5, 2021

View reviewed changes

IgorMinar approved these changes Jan 5, 2021

View reviewed changes

atscott removed the state: blocked label Jan 8, 2021

atscott added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 8, 2021

jelbourn approved these changes Jan 8, 2021

View reviewed changes

martinsik force-pushed the router-refactor-observables branch from 4bbd6c9 to 115f06f Compare January 10, 2021 19:28

refactor(router): refactor and simplify router RxJS chains

c7dd878

Refactor and simplifiy RxJS usage in the router package in order to reduce its size and increase performance.

martinsik force-pushed the router-refactor-observables branch from 115f06f to c7dd878 Compare January 10, 2021 19:45

atscott removed the request for review from alxhub January 11, 2021 22:09

atscott pushed a commit that referenced this pull request Jan 11, 2021

refactor(router): refactor and simplify router RxJS chains (#40290)

95a7dca

Refactor and simplifiy RxJS usage in the router package in order to reduce its size and increase performance. PR Close #40290

atscott closed this in 9105005 Jan 11, 2021

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

Conversation

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

Uh oh!

PR Checklist

PR Type

Does this PR introduce a breaking change?

Other information

Uh oh!

google-cla Bot commented Jan 1, 2021

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

atscott Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atscott Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

atscott commented Jan 4, 2021

Uh oh!

martinsik commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott commented Jan 4, 2021

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

martinsik commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

atscott commented Jan 8, 2021

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

martinsik commented Jan 11, 2021

Uh oh!

angular-automatic-lock-bot Bot commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

martinsik commented Jan 1, 2021 •

edited

Loading

martinsik commented Jan 4, 2021 •

edited

Loading

martinsik commented Jan 5, 2021 •

edited

Loading