doc(Core): Start a guide on best DI practices.

[matanlurey]

WIP: Not yet ready to merge, but feedback welcome.

Items I'd still like to cover

Misc

• Rename as effective versus guide.

* Writing code that is reliant on initReflector() side-effects.

Providers

• Note that using types implicitly as a provider is not a best practice.

Tokens

• Note how to use OpaqueToken, typed providers.
• Use MulitToken over multi: true

Components

• What should dependency injection be used/not used for (inputs, outputs?).

Waiting on feedback from @jonahwilliams.

Testing

* Writing component tests without using provide(...)

Not sure this is worth it yet, since not all test beds support this.

[zoechi]

Very helpful 👍

providers: const [Foo],

is still ok, or should it be const ClassProvider(Foo)?

What's the right replacement for

const Provider<String>(APP_BASE_HREF, useValue: '/'),

[kwalrath]

Maybe instead of "guide" (which could be confused with https://webdev.dartlang.org/angular/guide/dependency-injection), we could use the Effective Dart precedent. Maybe "Effective Angular" (dir name effective or effective-angular)?

Or we could just say "Best Practices" (dir name best-practices).

[matanlurey]

@zoechi:

Very helpful 👍

providers: const [Foo],

is still ok, or should it be const ClassProvider(Foo)?

I'd recommend ClassProvider(Foo). I'll add this to the guide.

What's the right replacement for

const Provider<String>(APP_BASE_HREF, useValue: '/'),

Ideally APP_BASE_HREF would be:

const OpaqueToken<String>(...)

... if it is not already, and then you would write:

const ValueProvider.forToken(APP_BASE_HREF, '/')

... I can add this example to the guide, as well, when I get to tokens.

[matanlurey]

@kwalrath:

Maybe instead of "guide" (which could be confused with https://webdev.dartlang.org/angular/guide/dependency-injection), we could use the Effective Dart precedent. Maybe "Effective Angular" (dir name effective or effective-angular)?

I don't know if the folder name is that important; effective seems fine to me, though. I'll make that adjustment in a follow-up commit. Keep the questions coming :)

/cc @chalin who will likely want some input here too.

[LorenVS]

I think it would be helpful at some point to have an example of how we expect this to work in a component test, especially one utilizing mocks. The provide(Service, useValue: mockService) pattern is very common in these tests, and its not obvious how to switch these to component provider lists without a loss of ergonomics.

[matanlurey]

@LorenVS:

I think it would be helpful at some point to have an example of how we expect this to work in a component test, especially one utilizing mocks. The provide(Service, useValue: mockService) pattern is very common in these tests, and its not obvious how to switch these to component provider lists without a loss of ergonomics.

Great idea. The 30 second version is:

const FactoryProvider(Service, useFactory: createMockService)

... should work fine in some cases. In cases where you want to tweak (i.e. with mockito) I think we can offer other patterns. I'll make sure to make some notes, even if they are inspirational versus totally concrete.

[jonahwilliams]

"what" to provide is probably a topic worth discussing. Overuse of dependency injection in place of inputs/outputs is my biggest gripe internally. it makes code much more difficult to test since it requires mocking (and mocks can be, well, optimistic), and you can really feel that if you start to look at gt's tests.

For example, Does an injected service constitute part of the Component's public API? this is a question worth discussing in detail, especially versus the explicit interface of inputs/outputs.

[leonsenft]

Should be

const ClassProvider(Foo, useClass: CachedFoo);

as useClass is an named, optional parameter. I believe this is to support const ClassProvider(Foo) when you actually want to use Foo.

The question is should this syntax exist, as is it not just a verbose way of writing only Foo in a list of providers?

[matanlurey]

Oh, whoops, is because of the multi API, got it (thought it was positional optional and not named optional - I'll make an edit).

as is it not just a verbose way of writing only Foo in a list of providers?

We want to move away from "lists" of providers, so yeah, we'd want to make sure that all providers are concretely of the type Provider.

[leonsenft]

While I generally prefer named, optional parameters, this is a case where using a positional optional parameter would make the API more consistent with the other variants. Obviously this isn't possible at the moment, due to the named multi parameter. Will this eventually be removed now that we have MultiToken, but exists currently to ease migration to these new provider types?

[matanlurey]

Yup. I'll make sure note MultiToken is preferred in the guide,

[matanlurey]

@jonahwilliams:

"what" to provide is probably a topic worth discussing. Overuse of dependency injection in place of inputs/outputs is my biggest gripe internally. it makes code much more difficult to test since it requires mocking (and mocks can be, well, optimistic), and you can really feel that if you start to look at gt's tests.

Great, thats a good topic.

For example, Does an injected service constitute part of the Component's public API? this is a question worth discussing in detail, especially versus the explicit interface of inputs/outputs.

Also a good topic for the component section. Added both to the TODOs above.

[matanlurey]

@jonahwilliams:

Any recommendations for this part?

### AVOID using injection to configure individual components

[chalin]

@matanlurey - is all of this available for use under the current Angular 5-alpha, or is some of it part of planned 5.x features?

Once this stabilizes, I'll certainly have to adjust the current Angular DI guide to conform to the recommended practices.

[matanlurey]

@chalin:

@matanlurey - is all of this available for use under the current Angular 5-alpha, or is some of it part of planned 5.x features?

Unless otherwise mentioned (1 or 2 cases), this is all landed.

Once this stabilizes, I'll certainly have to adjust the current Angular DI guide to conform to the recommended practices.

Great, definitely one of the reasons to have published best practices.

[jonahwilliams]

@matanlurey

Some ideas, not that organized.

### AVOID using injection to configure individual components

Perhaps some examples comparing the resulting APIs from an input based vs di based API.

Version A

<my-component></my-component>

@Component(selector: 'my-component', ...)
class MyComponent {
  final MyData myData;

  MyComponent(this.myData);
}

Version B

<my-component [myData]="myData"></my-component>

@Component(selector: 'my-component', ...)
class MyComponent {
  @Input()
  MyData myData;

  MyComponent();
}

Example 1: testing

Testing Version A

In order to test the injected version, you have to either repeat your setup code or write your own custom setUp function.

void main() {
  group(MyComponent, () {
     test('case 1', () async {
       ... angular test config here, since value has to be provided.
       
       expect(testComponent.myData, ....);
     });

     test('case 2', () async {
       ... angular test config here, since value has to be provided.
       
       expect(testComponent.myData, ....);
     });
  });
}

Testing Version B

Setup can be shared across all cases, existing setup code can be used. Data is provided closer to the site of the assertion, testing framework works better with inputs, et cetera.

void main() {
  group(MyComponent, () {
    setUp(() async { ... });

    test('case 1', () async {
      ...
      testBed.apply(() => testComponent.myData = new MyData());

      expect(testComonent.myData, ...);
    });

    test('case 2', () async {
      ...
      testBed.apply(() => testComponent.myData = new MyData());

      expect(testComonent.myData, ...);
    });
  });
}

Example 2: public API/documentation

Given the same component, I can put a nice doc comment on the input - and get auto complete when using angular analyzer. In the DI version I need to carefully read the implementation. Since angular calls the component constructors for us, there is no good place for DI APi which can be provided as autocomplete information.

... I'll have some more thoughts later

[matanlurey]

@jonahwilliams PTAL at what I have so far.

[jonahwilliams]

LGTM

[har79]

Some advice about when to use Component's providers parameter vs top-level providers could be useful. We had a hard-to-debug issue from people mixing them and ending up with multiple instances in different parts of the DI tree.

Relatedly, when to make providers singletons so that people can use them in either location without worrying about which is correct.

[TedSander]

@jonahwilliams @matanlurey

"what" to provide is probably a topic worth discussing. Overuse of dependency injection in place of inputs/outputs is my biggest gripe internally. it makes code much more difficult to test since it requires mocking (and mocks can be, well, optimistic), and you can really feel that if you start to look at gt's tests.

I don't completely agree with this statement. There are times when I think this makes for a nicer API. Especially when it is overriding a default. The benefit is that it signals the value can't change, and the value is ready to be used immediately.

You can see an example here: https://github.com/dart-lang/angular_components/blob/6b28cce682d087e85bcb584b8590ee594d344220/lib/material_input/material_number_accessor.dart#L55

This also has the benefit of being able to change this value for the whole app, but also in specific instances. For example we do:
<material-input type="number" fourDecimals></material-input>

Where fourDecimals is a directive that provides the local number format.

Moving this to an input would make the code either a bunch more complex. Need to handle the input changing which isn't always possible. Or unintuitive because the input can only be set once.

I also don't think using injection forces mocking. You can use directives to provide the real value, or just provide the real value in the test providers. I would agree that over mocking is a smell, but I don't see why that is more or less prevalent with using injection vs inputs. I guess it is because they don't want to create separate test beds?

For example, Does an injected service constitute part of the Component's public API? this is a question worth discussing in detail, especially versus the explicit interface of inputs/outputs.

To me yes. I teach that the following are all a part of your API: selector, input/outputs, dependencies.

[denniskaselow]

Is there a best practice for async code?

For example, in v4 I could await async code (like opening a WebSocket) and then provide the WebSocket via bootstrap.

The best I can do with @GenerateInjector in v5 is to use a FactoryProvider but that means I have to inject a Future<WebSocket> which is kinda awkward, because now the WebSocket instance can no longer be final and I have to assume that it can be null as long as the Future isn't resolved; or to mitigate the null checks I'd have to sprinkle init methods everywhere and call them from the components.

runAppAsync doesn't seem like it could solve my problem and new Injector.map() with the rootInjector as parent isn't helpful either, because MapInjector.injectFromSelfOptional has no fallback to the parent injector if something isn't found.

Am I missing something? Or is it something you aren't supposed to do, because it slows down startup?

[cedx]

I'm completely lost with the changes introduced by version 5.0.0-alpha+10 for boostraping the Angular application!

I'm using a map to provide settings for configuring the root injector. It lets me customize the location strategy at runtime.

Pre-alpha+10 code:

// The `config.dart` file is generated during build phase.
// It provides the `config` map.
import 'package:angular/angular.dart';
import 'package:angular_router/angular_router.dart';
import 'config.dart';
import 'main.template.dart' as ng;

void main() { 
  bootstrapStatic(AppComponent, [ 
    config['locationStrategy'] == 'hash' ? routerProvidersHash : routerProviders,
    new ValueProvider.forToken(configToken, config),
    // ... other providers
  ], ng.initReflector); 
}

With alpha+10, I don't know how to reproduce this behavior:

import 'package:angular/angular.dart';
import 'package:angular_router/angular_router.dart';
import 'package:my_app/my_root_component.template.dart' as ng;
import 'config.dart';
import 'main.template.dart' as self;

LocationStrategy locationStrategyFactory(@Inject(configToken) Map config, PlatformLocation platformLocation)
  => config['locationStrategy'] == 'hash' ? new HashLocationStrategy(platformLocation) : new PathLocationStrategy(platformLocation);

@GenerateInjector(const [
  routerProviders,
  const FactoryProvider(LocationStrategy, locationStrategyFactory),
  // ... other providers
])
final InjectorFactory rootInjector = self.rootInjector$Injector;

void main() => runApp(ng.AppComponentNgFactory, createInjector: rootInjector);

When I run the application, the path location strategy is always used. How can I override the location strategy using the new way of providing the root injector?

[matanlurey]

https://github.com/dart-lang/angular/blob/master/doc/effective/di.md#do-use-factories-for-configuration

[chalin]

Edit: scratch that!

@cedx, how about trying this simpler factory function (it more closely matches what you had in your pre-alpha+10 code):

Provider locationStrategyFactory() => new ClassProvider(LocationStrategy,
useClass: config['locationStrategy'] == 'hash'
? HashLocationStrategy
: PathLocationStrategy);

If that doesn't work, print the value of config['locationStrategy'] and make sure that it is actually 'hash'.

(Also, you might want to move any further questions to Stackoverflow.)

[matanlurey]

@chalin: That pattern is an anti-pattern (creating providers dynamically) as it's not possible to declare statically.

[chalin]

Oops, right @matanlurey, a factory shouldn't be returning providers anyways. Scratch that! (Sorry, I should have tried that first.)

@cedx: you write that the "config.dart file is generated during build phase." Is there any reason why the config map can't be const? If the entire map can't be const then maybe you can define a const variable that identifies the location strategy. For examples:

// This would be inside the generated `config.dart` file:
const useHashLocationStrategy = /* set to true or false by the build script */;

final config = {
  'locationStrategy': useHashLocationStrategy ? 'hash' : '', // if you still need this as part of config
  // ...
};

Then in main.dart:

@GenerateInjector([
  useHashLocationStrategy ? routerProvidersHash : routerProviders,
  // ... other providers
])
final InjectorFactory injector = self.injector$Injector;

void main() { /* same as you've shown */ }

[cedx]

Thanks a lot @chalin and @matanlurey for your support. It's true that this kind of question could have been posted on Stack Overflow...

@matanlurey I don't really understand your remark about "DO use factories for configuration". This section says: "Use FactoryProvider instead." That's exactly what my current code does (I don't talk about the pre-alpha+10 sample : right now, I'm using a FactoryProvider to select the right LocationStrategy). I don't see the difference between the sample provided and my current code (but I had a hard day, so maybe I'm not focused enough).

@chalin The map is already const, everything seems fine... and in fact, it is! I didn't change any line, but the code is working now. The sole thing I've done : rm -rf .dart_tools/build before launching pub run build_runner serve (only once, I had a problem with the build cache: deleting it fixed the issue, so I tried at random... I should have done it earlier!). But I'm gonna use your suggestion finally: I find it more explicit, and it avoids a useless factory.

Sorry for making both of you waste time.

[chalin]

No worries, it wasn't a waste of time at all. I'm glad that it's working, and that you'll end up simplifying your code!

[denniskaselow]

Finding the solution to my problem (thanks to https://github.com/dart-lang/angular/blob/master/examples/hacker_news_pwa/web/main.dart) was a facepalm moment for me. It's pretty obvious:

@GenerateInjector(const [
  const FactoryProvider(WebSocket, getWebSocket),
  ...
])
final InjectorFactory rootInjector = ng.rootInjector$Injector;

WebSocket webSocket;
WebSocket getWebSocket() => webSocket;

void main() async {
  webSocket = await openWebSocket();
  runApp(ng.RootComponentNgFactory, createInjector: rootInjector);
}

Future<WebSocket> openWebSocket() async {
  ...
}