Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

@Goodwine
Copy link
Contributor

commented Aug 19, 2019

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:

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.

@Goodwine Goodwine requested review from angular/fw-core as code owners Aug 19, 2019

@googlebot googlebot added the cla: yes label Aug 19, 2019

@Goodwine Goodwine force-pushed the Goodwine:testbed.inject branch 2 times, most recently from 9eb9451 to 1c64f7e Aug 19, 2019

@Goodwine Goodwine requested a review from angular/docs-infra as a code owner Aug 20, 2019

@Goodwine Goodwine force-pushed the Goodwine:testbed.inject branch from 1c64f7e to f9f04ff Aug 20, 2019

@ngbot ngbot bot modified the milestone: needsTriage Aug 21, 2019

@atscott atscott requested a review from mhevery Aug 21, 2019

@@ -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;

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 21, 2019

Member

I don't understand why this change is needed.

This comment has been minimized.

Copy link
@Goodwine

Goodwine Aug 21, 2019

Author Contributor

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 mhevery self-assigned this Aug 21, 2019

@mhevery

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@mhevery

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

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

@Goodwine

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

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

feat(core): Introduce TestBed.inject to replace 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.

@Goodwine Goodwine force-pushed the Goodwine:testbed.inject branch from f9f04ff to 421f99f Aug 22, 2019

@IgorMinar
Copy link
Member

left a comment

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.

@@ -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 comment has been minimized.

Copy link
@IgorMinar

IgorMinar Aug 23, 2019

Member

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

This comment has been minimized.

Copy link
@Goodwine

Goodwine Aug 23, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@Goodwine

Goodwine Aug 23, 2019

Author Contributor

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

@Goodwine

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

@mhevery

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

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 added a commit to Goodwine/angular that referenced this pull request Aug 28, 2019
@Goodwine Goodwine referenced this pull request Aug 28, 2019
6 of 14 tasks complete
Goodwine added a commit to Goodwine/angular that referenced this pull request Aug 29, 2019
@Goodwine

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

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?

Goodwine added a commit to Goodwine/angular that referenced this pull request Aug 29, 2019
Goodwine added a commit to Goodwine/angular that referenced this pull request Aug 29, 2019

@mhevery mhevery closed this in 3aba7eb Aug 29, 2019

@Goodwine Goodwine deleted the Goodwine:testbed.inject branch Aug 29, 2019

Goodwine added a commit to Goodwine/angular that referenced this pull request Aug 29, 2019
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this pull request Aug 29, 2019
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this pull request Aug 29, 2019
Goodwine added a commit to Goodwine/angular that referenced this pull request Aug 29, 2019
Goodwine added a commit to Goodwine/angular that referenced this pull request Aug 29, 2019
docs(core): Mark TestBed.get as deprecated
We should now use TestBed.inject
See angular#32200

Fixes angular#26491
@Goodwine Goodwine referenced this pull request Aug 29, 2019
6 of 14 tasks complete
Goodwine added a commit to Goodwine/angular that referenced this pull request Sep 4, 2019
Goodwine added a commit to Goodwine/angular that referenced this pull request Sep 4, 2019
Goodwine added a commit to Goodwine/angular that referenced this pull request Sep 4, 2019
Goodwine added a commit to Goodwine/angular that referenced this pull request Sep 5, 2019
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this pull request Sep 5, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
feat(core): Introduce TestBed.inject to replace TestBed.get (angular#…
…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
Goodwine added a commit to Goodwine/angular that referenced this pull request Sep 6, 2019
Goodwine added a commit to Goodwine/angular that referenced this pull request Sep 9, 2019
kyliau added a commit to angular/angular-cli that referenced this pull request Sep 9, 2019
matsko added a commit that referenced this pull request Sep 9, 2019
matsko added a commit that referenced this pull request Sep 12, 2019
feat(core): Deprecate TestBed.get as deprecated (#32406)
From 9.0.0 use TestBed.inject
See #32200

Fixes #26491

PR Close #32406
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.