docs(dependency-injection): revise Dart and TS code and prose #1573

chalin · 2016-06-01T15:18:06Z

Contributes to #1508 and #1522.

The main change on the TS side is to present best practices first, rather than presenting sample code and then saying "don't do that".
The "Optional dependencies" section has been moved towards the end of the provider variants section and its sample code has been simplified.
Reworked *_1.{ts,dart} sources so that main_1.{ts,dart} can actually be run, which helped find some bugs in those sources.
Despite OpaqueToken cannot be implemented in Dart angular#8928, the Dart and TS OpaqueToken sections have been left in the prose. They can be revisited later.

Enhancements to the Dart code include:

Added TestComponent.
Added a provider sample that makes use of an app config Map (to match TS); also added the more Dart-like use of a custom app configuration class, whose instance is initialized via the cascade operator.
Fixed Engine class hierarchy so that a single field named cylinders is defined (rather than two fields: one in the Engine supertype and one in the subclass), because having two fields is a bad design smell.
Similarly, fixed Logger hierarchy so that a single field named logs is defined. Also made logs a read-only field.
Reintroduced the SilentLogger().

chalin · 2016-06-01T15:23:00Z

@Foxandxss @wardbell @thso @kwalrath : this is ready for review. As usual, it is less painful to view the diff while ignoring changes in whitespace (such as indentation).

kwalrath · 2016-06-01T16:13:32Z

public/docs/_examples/dependency-injection/dart/lib/app_component.dart

@@ -11,6 +11,7 @@ import 'user_service.dart';
 //PENDING: check whether we intend to hide injector_component.dart & providers_component.dart; if so, change docregion name?


Should this comment really be visible? Also, it doesn't reflect test_component.dart.

That was an old comment that I missed (I had only searched for todo|fixme). I have removed it since it is not relevant (the entire source file is never shown).

Foxandxss · 2016-06-01T22:48:33Z

I have nothing to add, @kwalrath is doing an awesome job on the review. Sounds fine for me.

kwalrath · 2016-06-01T22:50:09Z

public/docs/ts/latest/guide/dependency-injection.jade

+
+  Our `HeroService` *requires* a `Logger`, but what if it could get by without
+  a logger?
+  As is illustrated next, we can indicate that to the dependency injection


This sounds very formal. Also, we should mention @Optional() in the text, not just in the code. Maybe:

We can tell Angular that the dependency is optional by annotating the constructor argument with @Optional():

This sounds very formal ...

Yes, what you propose is much better, thanks. Fixed.

kwalrath · 2016-06-02T16:17:47Z

public/docs/dart/latest/guide/dependency-injection.jade

-:marked
-  Then inject that class and its configuration information:
-+makeExample('dependency-injection/dart/lib/providers_component.dart','providers-7')(format=".")
+    The problem with using `Map` as a token, is not that it is an abstract


This is a really long sentence, and it continues the thought of the previous paragraph. Let's shorten this sentence and combine the two paragraphs. Maybe:

But what should we use as the token? We can't use Map because it's too general purpose. Our app might depend on several maps, each for a different purpose.

Dart difference: Interfaces are valid tokens
... are just class names. Map is a valid token even though it's the name of an abstract class; it's just unsuitable as a token because it's too general.

fixed per our discussion

kwalrath · 2016-06-02T16:45:28Z

Looks great! The only real problem I saw was the "key benefits of defining a configuration object class" paragraph, which needs to be clarified and probably broken up.

kwalrath · 2016-06-02T20:30:23Z

@Foxandxss & @wardbell: I don't know the TS side, so you still might want to take a look to make sure the new text is OK by you...

wardbell · 2016-06-02T21:57:50Z

Will look

chalin · 2016-06-03T00:48:08Z

Updates done.

Looks great! The only real problem I saw was the "key benefits..." paragraph

That paragraph has been reworked.

kwalrath · 2016-06-03T01:03:30Z

public/docs/dart/latest/guide/dependency-injection.jade

+    The Dart [cascade notation][cascade] (`..`) provides a convenient means of initializing
+    a configuration object. 
+
+    Use of the cascaded initialization idiom


On the right track, but this could still be simplified. Maybe:

If we use cascades, the configuration object can't be declared const and we can't use
a value provider. The solution is to use a factory provider:

Yes, what you propose is better. Done.

kwalrath · 2016-06-03T01:08:29Z

I had a couple more quibbles about the cascade wording, but otherwise LGTM.

chalin · 2016-06-03T01:28:16Z

Updates done.

Contributes to angular#1508 and angular#1522. - The main change on the TS side is to present best practices first, rather than presenting sample code and then saying "don't do that". - The "Optional dependencies" section has been moved towards the end of the provider variants section and its sample code has been simplified. - Reworked *_1.{ts,dart} sources so that main_1.{ts,dart} can actually be run, which helped find some bugs in those sources. - Despite angular/angular#8928, the Dart and TS OpaqueToken sections have been left in the prose. They can be revisited later. Enhancements to the Dart code include: - Added TestComponent. - Added a provider sample that makes use of an app config Map (to match TS); also added the more Dart-like use of a custom app configuration class, whose instance is initialized via the cascade operator. - Fixed Engine class hierarchy so that a single field named `cylinders` is defined (rather than two fields: one in the Engine supertype and one in the subclass), because having two fields is a bad design smell. - Similarly, fixed Logger hierarchy so that a single field named `logs` is defined. Also made `logs` a read-only field. - Reintroduced the SilentLogger().

kwalrath · 2016-06-03T18:09:14Z

LGTM. I'll merge this unless someone screams NO...

googlebot added the CLA: yes label Jun 1, 2016

kwalrath reviewed Jun 1, 2016
View reviewed changes

chalin force-pushed the chalin-dep-inj-0525 branch from 85bc650 to d627229 Compare June 1, 2016 23:44

kwalrath reviewed Jun 2, 2016
View reviewed changes

chalin force-pushed the chalin-dep-inj-0525 branch from d627229 to 40ba9a1 Compare June 2, 2016 17:24

chalin force-pushed the chalin-dep-inj-0525 branch from 40ba9a1 to dbcff2d Compare June 3, 2016 00:47

kwalrath reviewed Jun 3, 2016
View reviewed changes

chalin added 3 commits June 3, 2016 05:22

post-review updates

6e8f602

post-post-review updates

118d4a5

chalin force-pushed the chalin-dep-inj-0525 branch from 933f683 to 118d4a5 Compare June 3, 2016 12:22

kwalrath changed the title ~~docs(dependency-injection): revised Dart and TS code and prose~~ docs(dependency-injection): revise Dart and TS code and prose Jun 3, 2016

kwalrath merged commit 05864c2 into angular:master Jun 3, 2016

chalin deleted the chalin-dep-inj-0525 branch June 3, 2016 18:36

This was referenced Jun 3, 2016

Update Basics to new design scheme using Jade extends/blocks #1508

Closed

Add e2e tests for Basics Dart examples #1522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(dependency-injection): revise Dart and TS code and prose #1573

docs(dependency-injection): revise Dart and TS code and prose #1573

chalin commented Jun 1, 2016

chalin commented Jun 1, 2016

kwalrath Jun 1, 2016

chalin Jun 1, 2016

Foxandxss commented Jun 1, 2016

kwalrath Jun 1, 2016

chalin Jun 1, 2016

kwalrath Jun 2, 2016

chalin Jun 3, 2016

kwalrath commented Jun 2, 2016

kwalrath commented Jun 2, 2016

wardbell commented Jun 2, 2016

chalin commented Jun 3, 2016

kwalrath Jun 3, 2016

chalin Jun 3, 2016

kwalrath commented Jun 3, 2016

chalin commented Jun 3, 2016

kwalrath commented Jun 3, 2016

		@@ -11,6 +11,7 @@ import 'user_service.dart';
		//PENDING: check whether we intend to hide injector_component.dart & providers_component.dart; if so, change docregion name?

docs(dependency-injection): revise Dart and TS code and prose #1573

docs(dependency-injection): revise Dart and TS code and prose #1573

Conversation

chalin commented Jun 1, 2016

chalin commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Foxandxss commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwalrath commented Jun 2, 2016

kwalrath commented Jun 2, 2016

wardbell commented Jun 2, 2016

chalin commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwalrath commented Jun 3, 2016

chalin commented Jun 3, 2016

kwalrath commented Jun 3, 2016