feat(core): Introduce TestBed.inject to replace TestBed.get #32200

Goodwine · 2019-08-19T23:35:41Z

TestBed.get is not type safe, fixing it would be a massive breaking
change. The Angular team has proposed replacing it with TestBed.inject
and deprecate TestBed.get.

Fixes #26491
Fixes #29905

BREAKING CHANGE: Injector.get now accepts abstract classes to return
type-safe values. Previous implementation returned any through the
deprecated implementation.

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?

TestBed.get(ChangeDetectorRef) // returns any

Issue Number: #26491, #29905

What is the new behavior?

// Now deprecated
TestBed.get(ChangeDetectorRef) // returns any

TestBed.inject(ChangeDetectorRef) // returns ChangeDetectorRef

Does this PR introduce a breaking change?

Yes
No

The breaking change affects a feature that has been deprecated since Angular 4.x
Injector#get() has 2 signatures:

Typed signature, accept an InjectionToken<T> and a class (Type<T>)
Anything else is also accepted but typed as any

The updated Typed signature now also accepts abstract classes (AbstractType<T>) which may cause compilation issues to the deprecated use of any as it would need to be manually downcasted to the concrete type if needed.

Other information

Internal doc - go/testbed.inject

Note: I did not update all uses of TestBed.get in the Angular codebase to keep this diff small.
I will follow with another CL migrating all TestBed.get to TestBed.inject if this PR goes through.

mhevery · 2019-08-21T18:44:00Z

aio/src/app/app.component.spec.ts

@@ -677,7 +677,7 @@ describe('AppComponent', () => {
      });

      it('should not be loaded/registered until necessary', () => {
-        const loader: TestElementsLoader = fixture.debugElement.injector.get(ElementsLoader);
+        const loader = fixture.debugElement.injector.get(ElementsLoader) as unknown as TestElementsLoader;


I don't understand why this change is needed.

Per the "breaking changes" comment, I added AbstractType<T> to the "typed version" of Injector#get. Previously abstract classes would have returned any, the untyped version has been deprecated since Angular 4.x

This caused the returned value here to be type ElementsLoader which I thought I could cast directly to TestElementsLoader but they are apparently not fully compatible since the compiler complained about it

mhevery · 2019-08-21T18:49:32Z

presubmit

mhevery · 2019-08-21T19:52:13Z

This PR breaks a lot of things in google3 need to investigate more before it can be merged.

Goodwine · 2019-08-21T20:03:43Z

It can be caused by:

Change in Injector#get, abstract classes now return the abstract type and need to be downcasted, this could cause compilation errors, I looked at the breakages and it seems like it's unproper typing that caused it.
It will make all TestBed.get calls deprecated which would "break" all presubmits that block on TSLint

The former can be fixed by using proper typings, the latter can be solved by not marking TestBed.get in this commit and do it after migrating all of google3

Goodwine · 2019-08-21T20:18:28Z

Discussed with Misko, I'll remove the deprecation in this PR and fix breakages. Once it's merged, then I'll migrate google3 code, and then send another PR deprecating TestBed.get

TestBed.get is not type safe, fixing it would be a massive breaking change. The Angular team has proposed replacing it with TestBed.inject and deprecate TestBed.get. Deprecation from TestBed.get will come as a separate commit. Issue #26491 Fixes #29905 BREAKING CHANGE: Injector.get now accepts abstract classes to return type-safe values. Previous implementation returned `any` through the deprecated implementation.

IgorMinar

Can't we change this so that #get behavior is unmodified while #inject projects a type-safe implementation?

I don't think that we can change the behavior of the #get api without causing lots of breakages.

IgorMinar · 2019-08-23T17:36:25Z

packages/core/test/render3/view_container_ref_spec.ts

@@ -295,7 +295,7 @@ describe('ViewContainerRef', () => {
             const changeDetector = ref.injector.get(ChangeDetectorRef);
             changeDetector.detectChanges();
             expect(dynamicComp.doCheckCount).toEqual(1);
-             expect(changeDetector.context).toEqual(dynamicComp);
+             expect((changeDetector as any).context).toEqual(dynamicComp);


this change tells me that this is a painful breaking change that will likely affect lots of code.

Note that this is Injector.get, not TestBed, the change here is that we're now taking abstract classes in consideration to return the right type instead of returning any which is the deprecated behavior since Angular 4.x

I actually don't expect many breakages from Injector.get

For example, this only broke 5-6 targets on google3, affecting about 10-15 files which I already fixed.

Goodwine · 2019-08-23T18:00:08Z

Can't we change this so that #get behavior is unmodified while #inject projects a type-safe implementation?

This is what this commit is doing, I discussed with Vikram (see doc linked at the bottom of the PR) and he had the concern that TestBed.get is just too big to break, so we could add TestBed.inject and deprecate TestBed.get, the deprecation would happen on a separate commit.

mhevery · 2019-08-23T19:46:38Z

presubmit

mhevery · 2019-08-28T21:58:57Z

presubmit

mhevery · 2019-08-28T22:19:24Z

Can you do a global search and replace in the angular code base to switch all of the documentation example and tests to use the new inject API? Also add a note to it. Maybe do it in separate PR so that we can land this independently?

Goodwine · 2019-08-29T00:51:01Z

I have written the PR for that - #32382
One of the AIO tests was failing because the type wasn't found, I'm suspecting it needs this one submitted because I believe it might be pulling the types from HEAD, no?

…32200) TestBed.get is not type safe, fixing it would be a massive breaking change. The Angular team has proposed replacing it with TestBed.inject and deprecate TestBed.get. Deprecation from TestBed.get will come as a separate commit. Issue angular#26491 Fixes angular#29905 BREAKING CHANGE: Injector.get now accepts abstract classes to return type-safe values. Previous implementation returned `any` through the deprecated implementation. PR Close angular#32200

…tBed.inject` Relates to angular/angular#32200

This is cleanup/followup for PR #32200 PR Close #32382

From 9.0.0 use TestBed.inject See #32200 Fixes #26491 PR Close #32406

…32200) TestBed.get is not type safe, fixing it would be a massive breaking change. The Angular team has proposed replacing it with TestBed.inject and deprecate TestBed.get. Deprecation from TestBed.get will come as a separate commit. Issue angular#26491 Fixes angular#29905 BREAKING CHANGE: Injector.get now accepts abstract classes to return type-safe values. Previous implementation returned `any` through the deprecated implementation. PR Close angular#32200

This is cleanup/followup for PR angular#32200 PR Close angular#32382

From 9.0.0 use TestBed.inject See angular#32200 Fixes angular#26491 PR Close angular#32406

angular-automatic-lock-bot · 2019-09-29T16:04: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.}

Goodwine requested review from a team as code owners August 19, 2019 23:35

googlebot added the cla: yes label Aug 19, 2019

Goodwine requested a review from a team as a code owner August 20, 2019 01:20

This was referenced Aug 21, 2019

Breaking typed testbed get #30066

Closed

feat(core): Add TestBed.typedGet #30036

Closed

atscott added area: testing Issues related to Angular testing features, such as TestBed action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Aug 21, 2019

ngbot bot modified the milestone: needsTriage Aug 21, 2019

atscott requested a review from mhevery August 21, 2019 18:26

mhevery approved these changes Aug 21, 2019

View reviewed changes

mhevery self-assigned this Aug 21, 2019

mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 21, 2019

mhevery added the state: blocked label Aug 21, 2019

IgorMinar reviewed Aug 23, 2019

View reviewed changes

mhevery added the action: merge The PR is ready for merge by the caretaker label Aug 28, 2019

mhevery removed the state: blocked label Aug 28, 2019

Goodwine mentioned this pull request Aug 28, 2019

refactor(core): Migrate TestBed.get to TestBed.inject #32382

Closed

14 tasks

mhevery closed this in 3aba7eb Aug 29, 2019

Goodwine deleted the testbed.inject branch August 29, 2019 05:07

alan-agius4 mentioned this pull request Aug 29, 2019

feat(@schemetics/angular): replace deprecated TestBed.get with `Te… angular/angular-cli#15478

Merged

Goodwine mentioned this pull request Aug 29, 2019

docs(core): Mark TestBed.get as deprecated #32406

Closed

14 tasks

kyliau pushed a commit to angular/angular-cli that referenced this pull request Sep 9, 2019

feat(@schematics/angular): replace deprecated TestBed.get with `Tes…

b9c4b34

…tBed.inject` Relates to angular/angular#32200

matsko pushed a commit that referenced this pull request Sep 9, 2019

refactor(core): Migrate TestBed.get to TestBed.inject (#32382)

9166baf

This is cleanup/followup for PR #32200 PR Close #32382

matsko pushed a commit that referenced this pull request Sep 12, 2019

feat(core): Deprecate TestBed.get as deprecated (#32406)

a85eccd

From 9.0.0 use TestBed.inject See #32200 Fixes #26491 PR Close #32406

clydin mentioned this pull request Sep 12, 2019

feat(@schematics/angular): add generator for interceptor angular/angular-cli#15260

Merged

arnehoek pushed a commit to arnehoek/angular that referenced this pull request Sep 26, 2019

refactor(core): Migrate TestBed.get to TestBed.inject (angular#32382)

ba77ea6

This is cleanup/followup for PR angular#32200 PR Close angular#32382

arnehoek pushed a commit to arnehoek/angular that referenced this pull request Sep 26, 2019

feat(core): Deprecate TestBed.get as deprecated (angular#32406)

53f83c2

From 9.0.0 use TestBed.inject See angular#32200 Fixes angular#26491 PR Close angular#32406

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Introduce TestBed.inject to replace TestBed.get #32200

feat(core): Introduce TestBed.inject to replace TestBed.get #32200

Goodwine commented Aug 19, 2019

mhevery Aug 21, 2019

Goodwine Aug 21, 2019

mhevery commented Aug 21, 2019

mhevery commented Aug 21, 2019

Goodwine commented Aug 21, 2019

Goodwine commented Aug 21, 2019

IgorMinar left a comment

IgorMinar Aug 23, 2019

Goodwine Aug 23, 2019

Goodwine Aug 23, 2019 •

edited

Goodwine commented Aug 23, 2019

mhevery commented Aug 23, 2019

mhevery commented Aug 28, 2019

mhevery commented Aug 28, 2019

Goodwine commented Aug 29, 2019

angular-automatic-lock-bot bot commented Sep 29, 2019

feat(core): Introduce TestBed.inject to replace TestBed.get #32200

feat(core): Introduce TestBed.inject to replace TestBed.get #32200

Conversation

Goodwine commented Aug 19, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

mhevery Aug 21, 2019

Choose a reason for hiding this comment

Goodwine Aug 21, 2019

Choose a reason for hiding this comment

mhevery commented Aug 21, 2019

mhevery commented Aug 21, 2019

Goodwine commented Aug 21, 2019

Goodwine commented Aug 21, 2019

IgorMinar left a comment

Choose a reason for hiding this comment

IgorMinar Aug 23, 2019

Choose a reason for hiding this comment

Goodwine Aug 23, 2019

Choose a reason for hiding this comment

Goodwine Aug 23, 2019 • edited

Choose a reason for hiding this comment

Goodwine commented Aug 23, 2019

mhevery commented Aug 23, 2019

mhevery commented Aug 28, 2019

mhevery commented Aug 28, 2019

Goodwine commented Aug 29, 2019

angular-automatic-lock-bot bot commented Sep 29, 2019

Goodwine Aug 23, 2019 •

edited