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

Improve error messages for the `StaticInjector` #19302

Closed
rkirov opened this Issue Sep 20, 2017 · 6 comments

Comments

Projects
None yet
6 participants
@rkirov
Contributor

rkirov commented Sep 20, 2017

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or 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 a there is a missing injectable, say constructor(@Inject('foo') private foo: {}) and no matching provide, the error thrown does not mention the module, nor the dependency path.

The current error on the static injector looks like this:

search_videos.ng.html:26 ERROR Error: StaticInjectorError[foo]: 
  StaticInjectorError[foo]: 
    NullInjectorError: No provider for foo!
...

Expected behavior

It would be nice to see

  • The name of the module where the error occurs.
  • The name of the component where the injectable is injected.
  • Any intermediate injectables, like 'A needs B, B needs C, C needs Foo'.
@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Sep 21, 2017

Member

There are three issues:

Issue 1: Add the StaticInjectorError should add NgModule name.

The error should include the module name as you see here:

search_videos.ng.html:26 ERROR Error: StaticInjectorError(RouterModule)[foo]: 
  StaticInjectorError(MyAppModule)[foo]: 
    NullInjectorError: No provider for foo!
...
  1. Have Injector take additional optional name parameter https://github.com/angular/angular/blob/master/packages/core/src/di/injector.ts#L74
  2. Have error message creation include the error https://github.com/angular/angular/blob/master/packages/core/src/di/injector.ts#L125
  3. Look for places where "Injector.create" gets invoked an add useful name

Issue 2: The NgModuleRef should handle errors in the same way as StaticInjector

  1. get method of NgModuleRef_
    get(token: any, notFoundValue: any = Injector.THROW_IF_NOT_FOUND): any {
    return resolveNgModuleDep(
    this, {token: token, tokenKey: tokenKey(token), flags: DepFlags.None}, notFoundValue);
    }
    should add token information in the same way StaticInjector Does https://github.com/angular/angular/blob/master/packages/core/src/di/injector.ts#L241-L251
  2. It will probably mean that this code needs to be extracted in some way for re-usability.

Issue 3: The NgComponentFactory should handle errors in the same way as StaticInjector

This requires changes to the Compiler and is on @tbosch roadmap.

Repro steps

@Component({ ... })
class MyComponent {
  construct(iDontExist: IDontExist) {}
}

@NgModule({
  declarations: [MyComponent]
})
class MyModule

will produce an stack trace which never mentions MyModule or MyComponent

Member

mhevery commented Sep 21, 2017

There are three issues:

Issue 1: Add the StaticInjectorError should add NgModule name.

The error should include the module name as you see here:

search_videos.ng.html:26 ERROR Error: StaticInjectorError(RouterModule)[foo]: 
  StaticInjectorError(MyAppModule)[foo]: 
    NullInjectorError: No provider for foo!
...
  1. Have Injector take additional optional name parameter https://github.com/angular/angular/blob/master/packages/core/src/di/injector.ts#L74
  2. Have error message creation include the error https://github.com/angular/angular/blob/master/packages/core/src/di/injector.ts#L125
  3. Look for places where "Injector.create" gets invoked an add useful name

Issue 2: The NgModuleRef should handle errors in the same way as StaticInjector

  1. get method of NgModuleRef_
    get(token: any, notFoundValue: any = Injector.THROW_IF_NOT_FOUND): any {
    return resolveNgModuleDep(
    this, {token: token, tokenKey: tokenKey(token), flags: DepFlags.None}, notFoundValue);
    }
    should add token information in the same way StaticInjector Does https://github.com/angular/angular/blob/master/packages/core/src/di/injector.ts#L241-L251
  2. It will probably mean that this code needs to be extracted in some way for re-usability.

Issue 3: The NgComponentFactory should handle errors in the same way as StaticInjector

This requires changes to the Compiler and is on @tbosch roadmap.

Repro steps

@Component({ ... })
class MyComponent {
  construct(iDontExist: IDontExist) {}
}

@NgModule({
  declarations: [MyComponent]
})
class MyModule

will produce an stack trace which never mentions MyModule or MyComponent

@ocombe ocombe assigned ocombe and unassigned mhevery Sep 25, 2017

@juliemr

This comment has been minimized.

Show comment
Hide comment
@juliemr

juliemr Sep 26, 2017

Member

For our use case, the 'nice to have' "Any intermediate injectables, like 'A needs B, B needs C, C needs Foo'" is pretty critical. Teams are using common components and don't know where ngUpgraded stuff is coming from - and this is exceedingly useful for any unit testing scenario, even ones without ngUpgrade.

Member

juliemr commented Sep 26, 2017

For our use case, the 'nice to have' "Any intermediate injectables, like 'A needs B, B needs C, C needs Foo'" is pretty critical. Teams are using common components and don't know where ngUpgraded stuff is coming from - and this is exceedingly useful for any unit testing scenario, even ones without ngUpgrade.

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Dec 1, 2017

Member

Seems like this was fixed by #19482 and than reverted. @ocombe can you have a look what is the status of it?

Member

mhevery commented Dec 1, 2017

Seems like this was fixed by #19482 and than reverted. @ocombe can you have a look what is the status of it?

@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Dec 1, 2017

Contributor

There was a problem with g3 for some applications but I have no idea what it was because I don't have access to that.
Who could give me for more info?

Contributor

ocombe commented Dec 1, 2017

There was a problem with g3 for some applications but I have no idea what it was because I don't have access to that.
Who could give me for more info?

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Dec 5, 2017

Member

@ocombe could you create a new PR and rebase on master.

Member

mhevery commented Dec 5, 2017

@ocombe could you create a new PR and rebase on master.

@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Dec 6, 2017

Contributor
Contributor

ocombe commented Dec 6, 2017

ocombe added a commit to ocombe/angular that referenced this issue Dec 11, 2017

@jasonaden jasonaden closed this in b7738e1 Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment