angular / angular

Deliver web apps with confidence 🚀
https://angular.dev
MIT License
96.11k stars 25.44k forks source link

Better error message when using a service with a generic parameter #12262

Closed graphicsxp closed 7 years ago

graphicsxp commented 8 years ago

I'm submitting a ... (check one with "x")

[ *] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

When compiling a Ionic2 application for AOT, the ngc compiler gives me this error.

ngc: Error: Error at C:/dev/ecdt-mobile/.tmp/app/app.module.ngfactory.ts:177:20: Generic type 'BaseService<T>' requires 1 type argument(s).

Expected behavior

It should compile. I have created an angular2 repro (without ionic) and it compiles fine. HOWEVER the generated code is underline in my IDE and I believe this is not right:

 __BaseService_76:import46.BaseService;

it should be something like: __BaseService_76:import46.BaseService<any>;

Minimal reproduction of the problem with instructions

The repro project is here: https://github.com/graphicsxp/angular2

What is the motivation / use case for changing the behavior?

Please tell us about your environment:

Windows, npm

alxhub commented 8 years ago

This sounds like an Ionic issue - please follow up with them. If you can provide a reproduction of the problem using only Angular, feel free to reopen the issue.

simosalmi commented 8 years ago

hi, I'm getting the same error as @graphicsxp... Actually, @alxhub, I think the repo he posted doesn't reference Ionic, and the error is reproducible. The type argument is absent for generics in the TS code of the ngfactory. That's what is causing the issue.

alxhub commented 8 years ago

@graphicsxp @simosalmi

Okay, I looked through the repo posted. The issue is that you are passing BaseService in your providers. BaseService is a generic class, so attempting to provide it is confusing - Angular doesn't know what type argument you meant to use.

@chuckjaz may have more insight, but I believe we do need to give a better error message in this case.

graphicsxp commented 8 years ago

@alxhub You are right. Removing BaseService from the list of providers was all I had to do to make it compiles. BUT now it crashes at runtime with the following error: No Provider for BaseService

error_handler.js:47 EXCEPTION: Uncaught (in promise): Error: Error in ./RequestListComponent class RequestListComponent_Host - inline template:0:0 caused by: **No provider for BaseService!**ErrorHandler.handleError @ error_handler.js:47next @ application_ref.js:272schedulerFn @ async.js:82SafeSubscriber.__tryOrUnsub @ Subscriber.js:223SafeSubscriber.next @ Subscriber.js:172Subscriber._next @ Subscriber.js:125Subscriber.next @ Subscriber.js:89Subject.next @ Subject.js:55EventEmitter.emit @ async.js:74NgZone.triggerError @ ng_zone.js:278onHandleError @ ng_zone.js:257t.handleError @ polyfills.js:3e.runGuarded @ polyfills.js:3r @ polyfills.js:3i @ polyfills.js:3invoke @ polyfills.js:3
error_handler.js:52 ORIGINAL STACKTRACE:

That 's why I added BaseService to the list of providers... Any idea what I should do ?

BrainCrumbz commented 8 years ago

Same issue with us. Angular 2, TypeScript 2, Webpack 2 web application (i.e. no Ionic involved). Development build goes fine. Production build with AOT and UglifyJs complains with:

ERROR in [default] D:/myproj/codegen/src/app/module.ngfactory.ts:191:21 Generic type 'RestApiService' requires 1 type argument(s).

(codegen is the output directory for ngc compiled factories).

Looking in there:

// ...
class AppModuleInjector extends import0.NgModuleInjector<import1.AppModule> {
  // ...
  _RestApiService_65:import43.RestApiService;
  //                 ----------------------- this has the squiggly underline

Original code is:

// **** app/module.ts

@NgModule({
  imports: [
    SharedModule.forRoot(),
    // ...

// **** shared/module.ts

export class SharedModule {
  static forRoot(): ModuleWithProviders {
    return {
      ngModule: SharedModule,
      providers: [
        RestApiService,
        // ...

Of course, generic class is used throughout the app as a dependency with different types passed as T.

BrainCrumbz commented 8 years ago

Some related explanation might come from the final comments in this issue

graphicsxp commented 8 years ago

@BrainCrumbz I had a look at the other issue in your link. That sentence made me jump off my chair:

There are some technical challenges that must be overcome and some TypeScript features that need to be added for this to be implemented correctly that we currently don't believe adding this support provides enough value to justify the cost of its implementation.

If generics is not a TypeScript feature that provides value, I don't know what it is then... Or maybe I've missed something ?

BrainCrumbz commented 8 years ago

Tried also by registering typed classes as providers:

providers: [
  MyProvider, 
  MyOtherProvider,
  RestApiService<MyPayloadA>
  //             ---------- highlighted
]

but that is already highlighted by VS Code, and when building gives an error like:

Error: Invalid provider for the NgModule 'RelaxingModule' - only instances of Provider and Type are allowed, got: [MyProvider, MyOtherProvider, ?false?] at D:\myproj\node_modules\@angular\compiler\bundles\compiler.umd.js:14328:25

BrainCrumbz commented 8 years ago

Tried also declaring specific classes inheriting from generic one, and registering them as providers (here one of those as an example):

@Injectable()
export class AuthRestApiService extends RestApiService<MyAuthPayload>{
}

providers: [
  MyProvider, 
  MyOtherProvider,
  AuthRestApiService
]

No error highlighted in IDE, but when compiling and building:

ERROR in [default] D:/myproj/codegen/src/app/module.ngfactory.ts:xxx:yyy Supplied parameters do not match any signature of call target.

Looking at factory code:

this._AuthRestApiService_65 = new import43.AuthRestApiService();
                              --------------------------------- highlighted

Error hint in popup says:

[ts] Supplied parameters do not match any signature of call target. constructor AuthRestApiService<import46.AuthPayload>(http: import112.Http): import43.AuthRestApiService

So it looks like type parameter is ignored when _new_ing the service instance.

chuckjaz commented 8 years ago

@graphicsxp I believe you are taking my comment as a general indictment of generics which is not what I intended at all. There are some implementation challenges that make supporting generics in this context very difficult. It is this difficulty that I was trying to convey, not any slight to generics which I find very, very useful.

I believe the problem with the AuthRestApiService above is that it is inheriting the constructor of RestApiService but the metadata produced for AuthRestApiService declares its constructor doesn't take any parameters. Try repeating the constructor calling the super constructor as a work-around.

The reason

providers: [
  MyProvider, 
  MyOtherProvider,
  RestApiService<MyPayloadA>
]

doesn't work is because RestApiService<MyPayloadA> is not valid TypeScript because, in this context, RestApiService<MyPayload> is interpreted as RestApiService < MyPayload comparing the two types instead of using MyPayload as a type parameter (which results in false at runtime as you noticed) and the trailing > is reported a a syntax error and is elided in the output.

The instantiations of a generic types are not reified but are erased following a Java like erasure model. This means that there is no way, at runtime, to distinguish between RestApiService<MyPayloadA> and RestApiService<MyPayloadB> as both translate into a reference to the function RestApiSerivice; so even if this was valid TypeScript we couldn't support it. Creating a subclass that binds the type parameter creates a concrete reified class that can be used as a token for dependency injection.

DI only supports using concrete reified types as tokens as it must manufacture an instance of the type and the constructor function must be unique to the type. The token cannot be an instantiated generic type because it is ambigious as the type parameters are discarded at runtime and the same constructor function is used for all instantiations.

alxhub commented 8 years ago

@chuckjaz, is there a way for the compiler/metadata extractor to give a better error message when attempting to use a class with a generic type parameter as an injection token, instead of the current situation where the generated NgFactory code is incorrect and causes the TypeScript error given in the issue's title?

chuckjaz commented 8 years ago

@alxhub The static reflector host should be able to figure this out when it is asked to produce the reference to BaseService.

BrainCrumbz commented 8 years ago

@chuckjaz totally missed the constructor thing! (... from a C# point of view, the classic "base class does not contain a constructor that takes 0 arguments" error would have been nice in TypeScript too!).

Thanks for hint, that worked.

chuckjaz commented 8 years ago

Leaving this issue open; I assigned it to myself and will interpret this to be a request for a better error message as recommended by @alxhub.

simosalmi commented 8 years ago

Hi, thanks all for the contributions. This doesn’t solve the original issue IMHO, which is: how to provide generic injectables? Thanks

Simone

On 19 Oct 2016, at 18:34, Chuck Jazdzewski notifications@github.com wrote:

Leave this issue open; I assigned it to myself and will interpret this to be a request for a better error message as recommended by @alxhub https://github.com/alxhub.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/angular/angular/issues/12262#issuecomment-254884503, or mute the thread https://github.com/notifications/unsubscribe-auth/AAb03_wcDXExh7JBhk2Qj-Y4TKiXO9pVks5q1lSigaJpZM4KVkQj.

chuckjaz commented 8 years ago

@simosalmi They are not supported as a DI tokens. This bug is now tracking producing an error message that is more explicit about the limitation.

You need to create a concrete type that binds the type parameters.

graphicsxp commented 8 years ago

@chuckjaz I got your point about the difficulty of implementing this feature. Now, WILL this be implemented at all ? Is there a planning for this ?

I don't understand the workaround you suggested. In the case of my BaseService, what should I do ? Before I used generics, I had tried the inheritance approach. But since the parent constructor was injected dependencies (such as http service), the child constructor needed to inject these dependencies as well (passing them to the super() method). This I found silly because that means there was a dependency on http service in both the base class AND in the inherited classes. That is the reason why I went the generics way. Does it make sense ?

alxhub commented 8 years ago

No, it will not be implemented. As Chuck has said, Typescript does not have reified generics, so it is not possible within the language to use a generic type as a DI token.

graphicsxp commented 8 years ago

There is something that I don't understand and I would very much like an answer:

Why DI works with generics when I don't use AOT ?

In the meantime, here is the code I ended up writing. Since DI cannot be used with generics, I've followed above recommendations and used inheritance:

export interface IBase {
    id: number;
}

@Injectable()
export abstract class BaseService<T extends IBase> {
  private _loadingService: LoadingService;
  public serviceUrl
  constructor(private _http: Http, @Inject(LoadingService) loadingService: LoadingService) {    
    this._loadingService = loadingService;
  }

  getAll(): Observable<T[]> {
    this._loadingService.presentLoading();
    return this._http.get(this.serviceUrl)
      .map((response: Response) => <T[]>response.json())
      .finally(() => this._loadingService.hideLoading())
      .do(data => console.log("All: " + JSON.stringify(data)))
      .catch(this.handleError);
  }
}

Then in the inherited class:

@Injectable()
export class RequestService extends BaseService<IRequest> {

  constructor(_http: Http, @Inject(LoadingService) loadingService: LoadingService) {
    super(_http, loadingService);
    this.serviceUrl = './build/requests.json';
  }

  getAll(): Observable<IRequest[]> {
    return super.getAll();
  }
}

and in app.module providers: [RequestService]

What I don't like about this solution is that I have to inject dependencies in both the base class and in the inherited classes. But at least it works with AOT.

chuckjaz commented 8 years ago

Why DI works with generics when I don't use AOT ?

It works as long as you have only one instantiation of the generic used as a provider token. If you have two, they will both use the same token and one will obscure the other. In other words, it only appears to work. As soon as you try to introduce another instance of BaseService<T>, your application will subtly break. The code you moved to above works regardless of the number of derived classes from BaseService<T>.

The only way we could support this would be to have a way of reifying a generic type. TypeScript currently does not provide a way to do this and I am not aware of any plans to change this.

BrainCrumbz commented 8 years ago

@chuckjaz thanks for clarification. Although from our usage, we do have a working application in development build with several "occurences" of the same generic class injected as dependency with different type parameters: RestApiService<PayloadA>, RestApiService<PayloadB>, ...

But, and I guess this marks the difference, all those PayloadX type parameters are interfaces, not classes

nikvarma commented 8 years ago

Hi Guys,

Please help me, in the below code i m not getting error nor data in correct format

Component

export class HomeComponent { public homeBigSilder = bigSlider; public TopVideosModels: TopvideosModels[]; constructor(private _dataservice: TopvideosServices){ } ngOnInit() { this.getVedios(); } private getVedios(): void { this._dataservice .GetAll() .subscribe((data:TopvideosModels[]) => this.TopVideosModels = data, Error => console.log(Error), () => this.TopVideosModels); } }

Model

export class TopvideosModels { mediadata: MediaData[]; }

export class MediaData { public category: string; public imageURL: string; public likes: string; public mediaId: string; public section: string; public synopsis: string; public title: string; public urlCategory: string; public urlTitle: string; public videoID: string; public videoUrl: string; public views: number; }

Services

export class TopvideosServices { private actionURL : string; private headers: Headers; constructor(private _http: Http, private _urlConfig: urlConfig) { this.actionURL = _urlConfig.CrossbaseURL + "/" + _urlConfig.DatabaseURL

public GetAll = (): Observable<TopvideosModels[]> => {
    return this._http.get(this.actionURL)
    .map((response: Response) => <TopvideosModels[]>response.json());
}

}

Currently i am getting data in below format

Object data : {MediaData : { array }}

Object error : { }

but i want when the i get in response then it should get to MediaData object as we have List in C Sharp, nested property

class main { public string id {get;set}; public List userdetails {get;set}; }

chuckjaz commented 8 years ago

Your comment on this issue doesn't fall into the bug report or feature request category. This issue tracker is not suitable for support requests, please repost your issue on StackOverflow using tag angular.

If you are wondering why we don't resolve support issues via the issue tracker, please check out this explanation.

liquidboy commented 7 years ago

I just hit this issue with our AOT compiled Angular 2 app ... was working perfectly (or what appeared to work perfectly) for the JITed version...

We are injecting ModalDialogInstance < T > in alot of areas, and T can be whatever "Component" we want to load in the ModalDialogInstance .... Sadly seems like this is causing AOT to fail because as mentioned above the compiler has no idea what to make of T..

sigh!!!

chuckjaz commented 7 years ago

@liquidboy I believe what you are running into is more along the lines of the general principal requested in #12398. What it proposes is to be able to infer generic parameters from the types of the corresponding inputs. The issue uses NgFor as an example but it should work in your case too. What you want, I believe, is for T to be inferred to be the type of the child component used as the dialog instance.

chuckjaz commented 7 years ago

Closing as the original issue was fixed by https://github.com/angular/angular/commit/69e14b500b351965b235bb029bd6360f80e0ca23

Injecting a token with an interface type will be fixed by #14894

Inferring the type parameter will be deferred to post 4.0. The 4.0 code generation makes this less urgent as the 4.0 generated code will compile without inferring the correct type parameter.

angular-automatic-lock-bot[bot] commented 5 years ago

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.