New TestBed.get shows deprecation warning for abstract types and everywhere if using a strict tsconfig

@Goodwine

🐞 bug report

Affected Package

The issue is caused by package @angular/core/testing

Is this a regression?

Yes, the previous version in which this bug was not present was: 8.0.0-beta.11

Description

8.0.0-beta.12 introduced a new TestBed.get method and deprecated the existing one (see
609024f):

angular/packages/core/testing/src/test_bed.ts

Lines 246 to 256 in 609024f

    
             static get<T>(token: Type<T>|InjectionToken<T>, notFoundValue?: T, flags?: InjectFlags): any; 
        
             /** 
        
              * @deprecated from v8.0.0 use Type<T> or InjectionToken<T> 
        
              * @suppress {duplicate} 
        
              */ 
        
             static get(token: any, notFoundValue?: any): any; 
        
             static get( 
        
                 token: any, notFoundValue: any = Injector.THROW_IF_NOT_FOUND, 
        
                 flags: InjectFlags = InjectFlags.Default): any { 
        
               return _getTestBedViewEngine().get(token, notFoundValue, flags); 
        
             }

The new signature should be transparent for users as const service = TestBed.get(UserService); works.

But the new signature does not work with abstract types, like for example HttpTestingController (see https://github.com/angular/angular/blob/master/packages/common/http/testing/src/api.ts#L29) which leads to confusing deprecation warnings.

Update: this is even happening on every TestBed.get calls if you have a strict tsconfig, see #29905 (comment)

🔬 Minimal Reproduction

import { TestBed } from '@angular/core/testing';
import { HttpClientTestingModule, HttpTestingController } from '@angular/common/http/testing';

describe('UserService', () => {
  beforeEach(() => TestBed.configureTestingModule({
    imports: [HttpClientTestingModule]
  }));

  it('should be created', () => {
    const http: HttpTestingController = TestBed.get(HttpTestingController);
    expect(http).toBeTruthy();
  });
});

🔥 Exception or Error

ng lint shows:

/src/app/user.service.spec.ts:10:49 - get is deprecated: from v8.0.0 use Type<T> or InjectionToken<T>

🌍 Your Environment

Angular Version:


Angular CLI: 8.0.0-beta.13
Node: 11.11.0
OS: darwin x64
Angular: 8.0.0-beta.12
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.800.0-beta.13
@angular-devkit/build-angular     0.800.0-beta.13
@angular-devkit/build-optimizer   0.800.0-beta.13
@angular-devkit/build-webpack     0.800.0-beta.13
@angular-devkit/core              8.0.0-beta.13
@angular-devkit/schematics        8.0.0-beta.13
@angular/cli                      8.0.0-beta.13
@ngtools/webpack                  8.0.0-beta.13
@schematics/angular               8.0.0-beta.13
@schematics/update                0.800.0-beta.13
rxjs                              6.4.0
typescript                        3.4.3
webpack                           4.29.6

cc @Goodwine who wrote it and @mhevery who reviewed: I have not followed closely but couldn't get<T>(token: T | InjectionToken<T>, notFoundValue?: T, flags?: InjectFlags): any; be enough?

For abstract types you should cast to Type<MyAbstract>

couldn't get<T>(token: T | InjectionToken<T>, notFoundValue?: T, flags?: InjectFlags): any; be enough?

No because MyClass as an argument is type typeof MyClass

For example: if you were using Injector.get to get ChangeDetectorRef, you'd get the same warning because ChangeDetectorRef is an abstract class and you'd need to cast to Type<ChangeDetectorRef>.

The change in TestBed.get was to make the function signature slightly similar to Injector.get, but having it return any for now, My plan is to send a PR for review where TestBed.get<T> would return a type T instead of any.

More info on Issue #26491

@Goodwine

@Goodwine Thank you for your answer. Yeah I knew this was the goal and I very much look forward to it ;)

My point is:

can we have a better signature to not have to explicitely cast to Type<MyAbstract>? (looks like no?)
if no, maybe the deprecation message and the changelog message should be more explicit about this use-case. Injector.get is not used that often in an application, whereas TestBed.get is used in pretty much every service unit test. So the deprecation message is showing up a lot after upgrading and other developers are going to wonder if this is normal or not.

It would be awesome to not have to cast, because I think it's not a very good developer experience to have to write:

import { Type } from '@angular/core';

const http = TestBed.get(HttpTestingController as Type<HttpTestingController>);

I realize the new signature has another issue. I had many warnings with abstract classes but I also had others that looked weird and I figured it out.

If you have a strict TS config, more specifically if you have strictFunctionTypes turned on (which is the case if you activate the strict option), the deprecation warning shows up even for a simple case:

@Injectable({
  providedIn: 'root'
})
export class UserService {

  constructor(private http: HttpClient) {}

}

const service: UserService = TestBed.get(UserService); // tslint is unhappy

I guess this is because the Type signature in core.d.ts is:

export declare const Type: FunctionConstructor;

export declare interface Type<T> extends Function {
    new (...args: any[]): T;
}

(which is OK for bivariance check but not for contravariance check like strictFunctionCheck does).

This is going to be confusing for developers that activated the strict option in their TS config.

Re: better type for abstract class
Yes, there will be a better signature for get where you don't have to cast the abstract class

Re: strictFunctionCheck
Let me ask a coworker

@cexbrayat

@cexbrayat I tried reproducing the issue and couldn't, I guess that's because the error comes from a tslint check and not from the compiler strict option, what rule is the one failing?
I looked at https://palantir.github.io/tslint/rules/ and the only one that looked similar was strict-type-predicates, is that the one that failed for you? Or do you have your own strictFunctionCheck tslint rule?

repro attempt: (in options enable strictFunctionTypes) playground

@Goodwine

@Goodwine Sorry if I wasn't clear.

The TS rule failing is still the deprecated one.
Because when strict or strictFunctionCheck option is enabled, TestBed.get doesn't match the new signature but the deprecated one.

See repo for a reproduction: https://github.com/cexbrayat/test-bed-get

Or you can manually reproduce with:

ng new test-bed-get --defaults // using CLI 8.0.0-beta.15
cd test-bed-get
ng g s user

Replace user.service.ts by:

import { Injectable } from '@angular/core';
import { HttpClient } from '@angular/common/http';

@Injectable({
  providedIn: 'root'
})
export class UserService {

  constructor(private http: HttpClient) {}
}

Change the tsconfig.json file to have the strict option enabled:

{
  "compileOnSave": false,
  "compilerOptions": {
    "baseUrl": "./",
    "outDir": "./dist/out-tsc",
    "sourceMap": true,
    "declaration": false,
    "module": "esnext",
    "moduleResolution": "node",
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "strict": true,
    "importHelpers": true,
    "target": "es2015",
    "typeRoots": [
      "node_modules/@types"
    ],
    "lib": [
      "es2018",
      "dom"
    ]
  }
}

Then run ng lint, you should get:

WARNING: test-bed-get/src/app/user.service.spec.ts:9:42 - get is deprecated: from v8.0.0 use Type<T> or InjectionToken<T>

Thanks for the detailed repro steps, I'll take a look tomorrow and will try to come up with a PR to fix this issue.

This is - microsoft/TypeScript#24428

I'll see whether I can update Type<T> as suggested over there

After talking to the team it was decided to temporarily remove the @deprecated annotation to avoid too much noise in v8, until we properly fix it. See #30514

@deprecated

fix(core): temporarily remove @deprecated jsdoc tag for a TextBed.get…

@deprecated

fix(core): temporarily remove @deprecated jsdoc tag for a TextBed.get…

Closed by 561e01d

@kara

@kara I think we should keep this open until we properly fix it. The commit removing the @depecated tag was just a quick fix to avoid too much noise in v8 and only partially address the issue.

For example the new signature still forces us to write:

import { Type } from '@angular/core';

const http = TestBed.get(HttpTestingController as Type<HttpTestingController>);

which is not ideal.

How did this get overlooked on the upgrade guides and breaking changes? Unit tests are not second class code. I'm seeing this in the latest release version 8.0.0. Are we awaiting a quick bug-fix release that will allow this to work? Or for 8.0.0 do I just have to use the new get<T>?

@jkyoutsey

@jkyoutsey this wasn't meant to be a breaking change, but due to a TSLint bug it ended up being one. @angular/core@8.0.2 should resolve this temporarily.

angular/packages/core/testing/src/test_bed_common.ts

Lines 117 to 126 in a73b8a6

    
             get<T>(token: Type<T>|InjectionToken<T>, notFoundValue?: T, flags?: InjectFlags): any; 
        
             // TODO: switch back to official deprecation marker once TSLint issue is resolved 
        
             // https://github.com/palantir/tslint/issues/4522 
        
             /** 
        
              * deprecated from v8.0.0 use Type<T> or InjectionToken<T> 
        
              * This does not use the deprecated jsdoc tag on purpose 
        
              * because it renders all overloads as deprecated in TSLint 
        
              * due to https://github.com/palantir/tslint/issues/4522. 
        
              */ 
        
             get(token: any, notFoundValue?: any): any;

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

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

Navigation Menu

Repository navigation

New TestBed.get shows deprecation warning for abstract types and everywhere if using a strict tsconfig #29905

🐞 bug report

Affected Package

Is this a regression?

Description

🔬 Minimal Reproduction

🔥 Exception or Error

🌍 Your Environment

6 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	static get<T>(token: Type<T>\|InjectionToken<T>, notFoundValue?: T, flags?: InjectFlags): any;
	/**
	* @deprecated from v8.0.0 use Type<T> or InjectionToken<T>
	* @suppress {duplicate}
	*/
	static get(token: any, notFoundValue?: any): any;
	static get(
	token: any, notFoundValue: any = Injector.THROW_IF_NOT_FOUND,
	flags: InjectFlags = InjectFlags.Default): any {
	return _getTestBedViewEngine().get(token, notFoundValue, flags);
	}

	get<T>(token: Type<T>\|InjectionToken<T>, notFoundValue?: T, flags?: InjectFlags): any;
	// TODO: switch back to official deprecation marker once TSLint issue is resolved
	// https://github.com/palantir/tslint/issues/4522
	/**
	* deprecated from v8.0.0 use Type<T> or InjectionToken<T>
	* This does not use the deprecated jsdoc tag on purpose
	* because it renders all overloads as deprecated in TSLint
	* due to https://github.com/palantir/tslint/issues/4522.
	*/
	get(token: any, notFoundValue?: any): any;

New TestBed.get shows deprecation warning for abstract types and everywhere if using a strict tsconfig #29905

Description

🐞 bug report

Affected Package

Is this a regression?

Description

🔬 Minimal Reproduction

🔥 Exception or Error

🌍 Your Environment

Activity

Goodwine commented on Apr 15, 2019

Goodwine commented on Apr 15, 2019

cexbrayat commented on Apr 16, 2019

cexbrayat commented on Apr 16, 2019

Goodwine commented on Apr 16, 2019

Goodwine commented on Apr 16, 2019

cexbrayat commented on Apr 17, 2019

Goodwine commented on Apr 18, 2019

Goodwine commented on Apr 19, 2019

cexbrayat commented on May 16, 2019

6 remaining items

kara commented on May 24, 2019

cexbrayat commented on May 24, 2019

jkyoutsey commented on Jun 13, 2019

Splaktar commented on Jun 30, 2019

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

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions