fix/refactor(common): prevent duplicate URL change notifications from `SpyLocation` and use `TestBed.inject` in the `Location` test suite by LayZeeDK · Pull Request #37459 · angular/angular

LayZeeDK · 2020-06-05T11:49:21Z

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:

What is the current behavior?

SpyLocation emits duplicate notifications to URL change listeners when multiple are registered.
Location uses the private acces modifier for the private _urlChangeSubscription member.
location_spec.ts uses the weakly typed inject to resolve dependencies.

Issue Number: As discussed in #37404

What is the new behavior?

SpyLocation only emits one URL change notification to all listeners.
Location use @internal JSDoc annotations for all private members.
location_spec.ts uses the strongly typed TestBed.inject to resolve dependencies.

Does this PR introduce a breaking change?

Yes
No

Other information

Cc @atscott

LayZeeDK · 2020-06-05T12:58:57Z

Hey @atscott,

I've tried different things to get around this, but several other tests are broken by switching from @internal annotations to private access modifiers because of TypeScript weirdness when using MyClass implements MyOtherClass.

packages/router/test/integration.spec.ts:1225:27 - error TS2352: Conversion of type 'Location' to type 'SpyLocation' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
 

 
1225          const location = TestBed.inject(Location) as SpyLocation;
 
                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
packages/router/test/integration.spec.ts:1252:27 - error TS2352: Conversion of type 'Location' to type 'SpyLocation' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
 

 
1252          const location = TestBed.inject(Location) as SpyLocation;
 
                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
packages/router/test/integration.spec.ts:1318:25 - error TS2352: Conversion of type 'Location' to type 'SpyLocation' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
 
  Type 'Location' is missing the following properties from type 'SpyLocation': urlChanges, _history, _historyIndex, setInitialPath, and 3 more.
 

 
1318        const location = TestBed.inject(Location) as SpyLocation;
 
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This will break a lot of userland tests as well as SpyLocation is the most important stub for working with the RouterTestingModule.

What do we do? TypeScript suggests to use SpyLocation extends Location instead. We could do that, but it seems odd and might introduce additional work.

atscott · 2020-06-05T15:57:27Z

Hi @LayZeeDK Hmmm, I see. Thanks for investigating this. After taking a closer look, I think maybe @internal really is the correct access modifier. I don't think extends is quite right because we really don't want to use any of the real Location implementations in the SpyLocation and in addition, we still can't have separate implementations of the private members with extends and I don't think exposing them to things outside the package with protected is desired either.

Sorry for all this back and forth! I think @internal is correct and that you actually will need to change _urlChangeSubscription to @internal as well. In addition to learning a bit more about these interactions, another good thing that came out of this is that I noticed you need the same subscription fix using _urlChangeSubscription for SpyLocation that you added for Location in the previous PR. Could you do that here?

Use the strongly typed TestBed.inject rather than the weakly typed inject test utility function. Reuse injected dependency variables between sibling test cases.

Prevent duplicate notifications from being emitted when multiple URL change listeners are registered using SpyLocation#onUrlChange. Use `@internal` annotation for the `_urlChangeSubscription` properties instead of the `private` access modifier. Otherwise, we get in trouble because of `SpyLocation implements Location`.

LayZeeDK · 2020-06-05T21:47:58Z

🏎️ Here we go, @atscott. Let's do this!

atscott

Great, thanks so much for working on this!

AndrewKushnir

LGTM 👍

Prevent duplicate notifications from being emitted when multiple URL change listeners are registered using Location#onUrlChange. PR Close #37404

Prevent duplicate notifications from being emitted when multiple URL change listeners are registered using SpyLocation#onUrlChange. Use `@internal` annotation for the `_urlChangeSubscription` properties instead of the `private` access modifier. Otherwise, we get in trouble because of `SpyLocation implements Location`. PR Close #37459

Use the strongly typed TestBed.inject rather than the weakly typed inject test utility function. Reuse injected dependency variables between sibling test cases. PR Close #37459

Prevent duplicate notifications from being emitted when multiple URL change listeners are registered using SpyLocation#onUrlChange. Use `@internal` annotation for the `_urlChangeSubscription` properties instead of the `private` access modifier. Otherwise, we get in trouble because of `SpyLocation implements Location`. PR Close #37459

Use the strongly typed TestBed.inject rather than the weakly typed inject test utility function. Reuse injected dependency variables between sibling test cases. PR Close angular#37459

Prevent duplicate notifications from being emitted when multiple URL change listeners are registered using SpyLocation#onUrlChange. Use `@internal` annotation for the `_urlChangeSubscription` properties instead of the `private` access modifier. Otherwise, we get in trouble because of `SpyLocation implements Location`. PR Close angular#37459

angular-automatic-lock-bot · 2020-07-10T16:14:36Z

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

Use the strongly typed TestBed.inject rather than the weakly typed inject test utility function. Reuse injected dependency variables between sibling test cases. PR Close angular#37459

Prevent duplicate notifications from being emitted when multiple URL change listeners are registered using SpyLocation#onUrlChange. Use `@internal` annotation for the `_urlChangeSubscription` properties instead of the `private` access modifier. Otherwise, we get in trouble because of `SpyLocation implements Location`. PR Close angular#37459

googlebot added the cla: yes label Jun 5, 2020

LayZeeDK force-pushed the LayZeeDK/refactor/common-refactor-private-members-and-use-testbed branch 3 times, most recently from 68e51d2 to c8f6895 Compare June 5, 2020 12:43

LayZeeDK added 2 commits June 5, 2020 23:24

test(common): prefer TestBed.inject over inject

ac0ae21

Use the strongly typed TestBed.inject rather than the weakly typed inject test utility function. Reuse injected dependency variables between sibling test cases.

LayZeeDK force-pushed the LayZeeDK/refactor/common-refactor-private-members-and-use-testbed branch from c8f6895 to 10af4db Compare June 5, 2020 21:25

LayZeeDK changed the title ~~refactor(common): refactor private members in Location and use TestBed.inject in its test suite~~ fix/refactor(common): refactor private members in Location + SpyLocation and use TestBed.inject in its test suite Jun 5, 2020

LayZeeDK changed the title ~~fix/refactor(common): refactor private members in Location + SpyLocation and use TestBed.inject in its test suite~~ fix/refactor(common): prevent duplicate URL change notifications from SpyLocation and use TestBed.inject in the Location test suite Jun 5, 2020

LayZeeDK marked this pull request as ready for review June 5, 2020 21:47

pullapprove Bot requested a review from kara June 5, 2020 21:47

pullapprove Bot requested a review from AndrewKushnir June 6, 2020 02:25

atscott approved these changes Jun 8, 2020

View reviewed changes

atscott added area: common Issues related to APIs in the @angular/common package action: presubmit The PR is in need of a google3 presubmit target: patch This PR is targeted for the next patch release labels Jun 8, 2020

ngbot Bot modified the milestone: needsTriage Jun 8, 2020

AndrewKushnir approved these changes Jun 8, 2020

View reviewed changes

LayZeeDK referenced this pull request Jun 8, 2020

fix(common): prevent duplicate URL change notifications (#37404)

fff424a

Prevent duplicate notifications from being emitted when multiple URL change listeners are registered using Location#onUrlChange. PR Close #37404

atscott added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Jun 8, 2020

atscott removed the request for review from kara June 8, 2020 22:04

atscott closed this in 0b78459 Jun 9, 2020

LayZeeDK deleted the LayZeeDK/refactor/common-refactor-private-members-and-use-testbed branch June 9, 2020 19:37

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix/refactor(common): prevent duplicate URL change notifications from `SpyLocation` and use `TestBed.inject` in the `Location` test suite#37459

fix/refactor(common): prevent duplicate URL change notifications from `SpyLocation` and use `TestBed.inject` in the `Location` test suite#37459
LayZeeDK wants to merge 2 commits into
angular:masterfrom
LayZeeDK:LayZeeDK/refactor/common-refactor-private-members-and-use-testbed

LayZeeDK commented Jun 5, 2020 •

edited

Loading

Uh oh!

LayZeeDK commented Jun 5, 2020 •

edited

Loading

Uh oh!

atscott commented Jun 5, 2020

Uh oh!

LayZeeDK commented Jun 5, 2020 •

edited

Loading

Uh oh!

atscott left a comment

Uh oh!

AndrewKushnir left a comment

Uh oh!

angular-automatic-lock-bot Bot commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

LayZeeDK commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

LayZeeDK commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott commented Jun 5, 2020

Uh oh!

LayZeeDK commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott 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!

angular-automatic-lock-bot Bot commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LayZeeDK commented Jun 5, 2020 •

edited

Loading

LayZeeDK commented Jun 5, 2020 •

edited

Loading

LayZeeDK commented Jun 5, 2020 •

edited

Loading