Skip to content
This repository has been archived by the owner on Sep 16, 2022. It is now read-only.

DI: In debug mode, trace a provider failure #434

Closed
5 tasks done
matanlurey opened this issue Jun 9, 2017 · 6 comments
Closed
5 tasks done

DI: In debug mode, trace a provider failure #434

matanlurey opened this issue Jun 9, 2017 · 6 comments

Comments

@matanlurey
Copy link
Contributor

matanlurey commented Jun 9, 2017

This used to happen in pre-Angular Dart 2.0, but was never added for code generation.

EDIT as of 2018-08-14 - We still need to cover the view-compiler cases to close this:

  • Child directives with dynamic dependencies:

    _CompChild2_0_5 = CompChild2(parentView.injectorGet(import1.A, viewData.parentIndex));

    ... should become ...

    // class _ViewFooComponent
    debugInjectorGet(CompChild2);
    _CompChild2_0_5 = CompChild2(parentView.injectorGet(import1.A, viewData.parentIndex));
    debugInjectorLeave(CompChild2);
  • Child directives in embedded templates (or anything that triggers parentView.injectorGet):

    (i.e. figure out what code-paths invoke injectFromViewParentInjector)

  • Queries for services that live on child directives that have dynamic dependencies

  • Provided services with dynamic dependencies:

    _InjectsMissingService_0_5 = InjectsMissingService(injectorGet(MissingService, viewData.parentIndex));

    ... should become ...

    debugInjectorGet(InjectsMissingService);
    _InjectsMissingService_0_5 = InjectsMissingService(injectorGet(MissingService, viewData.parentIndex));
    debugInjectorLeave(InjectsMissingService);
  • Provided services via a FactoryProvider

@matanlurey matanlurey added this to the V5 milestone Jan 24, 2018
@matanlurey matanlurey self-assigned this Feb 5, 2018
matanlurey added a commit that referenced this issue Feb 5, 2018
…essages.

Added a simple test (passes), and a complex test (skipped, does not pass). In
dev-mode we will start tracking the chain of failed lookups and give a better
error message, i.e. "No provider found for B: A -> B".

Work towards #434.

PiperOrigin-RevId: 184594151
matanlurey added a commit that referenced this issue Feb 6, 2018
…essages.

Added a simple test (passes), and a complex test (skipped, does not pass). In
dev-mode we will start tracking the chain of failed lookups and give a better
error message, i.e. "No provider found for B: A -> B".

Work towards #434.

PiperOrigin-RevId: 184594151
matanlurey added a commit that referenced this issue Feb 6, 2018
*** Reason for rollback ***

Teams are relying on this throwsArgumentError, so I'll have to do this behind a flag...

*** Original change description ***

feat(Core): Start work on a MissingProviderError, with better error messages.

Added a simple test (passes), and a complex test (skipped, does not pass). In
dev-mode we will start tracking the chain of failed lookups and give a better
error message, i.e. "No provider found for B: A -> B".

Work towards #434.

***

PiperOrigin-RevId: 184617305
@matanlurey matanlurey mentioned this issue Feb 6, 2018
matanlurey added a commit that referenced this issue Feb 6, 2018
*** Reason for rollback ***

Teams are relying on this throwsArgumentError, so I'll have to do this behind a flag...

*** Original change description ***

feat(Core): Start work on a MissingProviderError, with better error messages.

Added a simple test (passes), and a complex test (skipped, does not pass). In
dev-mode we will start tracking the chain of failed lookups and give a better
error message, i.e. "No provider found for B: A -> B".

Work towards #434.

***

PiperOrigin-RevId: 184617305
@matanlurey
Copy link
Contributor Author

This is a WIP. In early code, it's looking ~OK, but with the following mistakes:

When the ViewCompiler generates code like so:

build() {
  _Foo = new Foo(injectorGet(FooDependency));
}

... it would need to insert trace statements ...

build() {
  debugInjectorEnter(Foo);
  _Foo = new Foo(injectorGet(FooDependency));
  debugInjectorExit(Foo);
}

... if we do that, we should get ~100% feature coverage.

@matanlurey
Copy link
Contributor Author

We talked offline. This is probably OK, but we'll validate first that:

  1. The general feature doesn't disrupt testing and development time noticeably.
  2. Adding enter/exit doesn't create too much additional code in a typical large DDC application.

matanlurey added a commit that referenced this issue Feb 8, 2018
This flag is *enabled* by default. When enabled, you can expect *some* better
error messages, but the better messages are *not* lazy, early testing by key customers don't show any noticeable performance regressions in DDC.

See a potentially blocking issue:
#434 (comment)

... where we've decided to evaluate code cost before going forward on that one.

PiperOrigin-RevId: 184883497
matanlurey added a commit that referenced this issue Feb 8, 2018
This flag is *enabled* by default. When enabled, you can expect *some* better
error messages, but the better messages are *not* lazy, early testing by key customers don't show any noticeable performance regressions in DDC.

See a potentially blocking issue:
#434 (comment)

... where we've decided to evaluate code cost before going forward on that one.

PiperOrigin-RevId: 184883497
@matanlurey
Copy link
Contributor Author

The rest of this is very non-trivial. We should review at the weekly about the priority of this.

@matanlurey
Copy link
Contributor Author

I... think either @hterkelsen or @ferhatb was going to help here.

@matanlurey
Copy link
Contributor Author

Finishing this feature has been postponed until after V5/Dart 2.

@matanlurey matanlurey modified the milestones: =5.0.0, >5.0.0 <6.0.0 Apr 24, 2018
@matanlurey matanlurey removed their assignment Jul 7, 2018
@matanlurey
Copy link
Contributor Author

matanlurey commented Aug 14, 2018

Related; this would be much easier with dart-lang/sdk#34141.

Otherwise, we'd need to convert current expressions into 3 statements, which is really hard in some places (for example we don't have a great way of creating unique local variables without just using a global counter of some sort).

matanlurey added a commit that referenced this issue Aug 14, 2018
Partial work towards #434.

It looks like unfortunately completing this fix might be [virtually] infeasible without dart-lang/sdk#34141, though we could potentially use a ternary expression (dart-lang/sdk#34141 (comment)).

Closes #1568

PiperOrigin-RevId: 208663946
matanlurey added a commit that referenced this issue Aug 14, 2018
Partial work towards #434.

It looks like unfortunately completing this fix might be [virtually] infeasible without dart-lang/sdk#34141, though we could potentially use a ternary expression (dart-lang/sdk#34141 (comment)).

Closes #1568

PiperOrigin-RevId: 208663946
@matanlurey matanlurey modified the milestones: <v6.0.0, =v5.1.0 Aug 16, 2018
matanlurey added a commit that referenced this issue Aug 16, 2018
Partially closes #434 (still need factories).

Downsides of this implementation is mostly the code it emits now for traceable `new X(...)` is plain ugly, though we wouldn't need the worst part, the ternary expression, if dart-lang/sdk#34141 was addressed.

Ideally we would not use this wrapper expression at all, but it is currently NP-hard to refactor what is an expression into a statement due to how the compiler pipeline works (the expression is created before the assignment), so this seems like the least-bad option _if_ we want this fix.

Also deleted some unused code.

Closes #1573

PiperOrigin-RevId: 209010949
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant