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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

New TestBed.get shows deprecation warning for abstract types #29905

Open
cexbrayat opened this Issue Apr 15, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@cexbrayat
Copy link
Contributor

cexbrayat commented Apr 15, 2019

馃悶 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):

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.

馃敩 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?

@Goodwine

This comment has been minimized.

Copy link
Contributor

Goodwine commented Apr 15, 2019

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

@Goodwine

This comment has been minimized.

Copy link
Contributor

Goodwine commented Apr 15, 2019

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

@cexbrayat

This comment has been minimized.

Copy link
Contributor Author

cexbrayat commented Apr 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

cexbrayat commented Apr 16, 2019

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.

@Goodwine

This comment has been minimized.

Copy link
Contributor

Goodwine commented Apr 16, 2019

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

@Goodwine

This comment has been minimized.

Copy link
Contributor

Goodwine commented Apr 16, 2019

@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

@ngbot ngbot bot added this to the needsTriage milestone Apr 16, 2019

@cexbrayat

This comment has been minimized.

Copy link
Contributor Author

cexbrayat commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Contributor

Goodwine commented Apr 18, 2019

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.

@Goodwine

This comment has been minimized.

Copy link
Contributor

Goodwine commented Apr 19, 2019

This is - Microsoft/TypeScript#24428

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

Goodwine added a commit to Goodwine/angular that referenced this issue Apr 19, 2019

fix(core): Fix Type<T> for strictFunctionTypes
See - Microsoft/TypeScript#24428
tl;dr - When using `--strict` mode, `any` isn't a subtype of `string`
and thus the function doesn't match types.

Fixes angular#29905

@Goodwine Goodwine referenced a pull request that will close this issue Apr 19, 2019

Open

fix(core): Fix Type<T> for strictFunctionTypes #30001

5 of 14 tasks complete

Goodwine added a commit to Goodwine/angular that referenced this issue Apr 19, 2019

fix(core): Fix Type<T> for strictFunctionTypes
See - Microsoft/TypeScript#24428
tl;dr - When using `--strict` mode, `any` isn't a subtype of `string`
and thus the function doesn't match types.

Fixes angular#29905
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.