Skip to content
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

add tree-shakable providers doc #23027

Closed

Conversation

sreevani-ship-it
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Other information

#22803 - Orginal PR

@mary-poppins
Copy link

You can preview 2c58b45 at https://pr23027-2c58b45.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 87643c1 at https://pr23027-87643c1.ngbuilds.io/.

@about-code
Copy link

Current drafts on treeshakable providers in https://pr22803-95a505f.ngbuilds.io/guide/dependency-injection#creating-tree-shakable-providers suggest that a replacement of an injectable type should/could be declared in the same module which defines the injectable type, e.g.

@Injectable({ 
  providedIn: 'root', 
  useClass: FooServiceImpl  // same with useFactory
}) 
export class FooService {}

What I've learnt about DI, though, is that DI is meant to decouple a service consumer (which depends on an interface type like FooService) from a concrete implementation like FooServiceImpl in order to be able to inject alternative implementations or mocks of the interface type into the consumer without having to alter service consumer nor service interface type (because I may neither be consumer nor interface type provider but a third-party providing an alternative implementation)

But if I do specify an implementation of an interface in the same module which declares the interface type then this does seem to me like a tight coupling of an interface type and a concrete implementation. What's the point of DI then, anymore? The drafts look like that by choosing and injecting the interface type one does also inevitably choose a concrete implementation. Then I could do new FooServiceImpl() in the service consumer, once again like I did in the "good" old days without DI.

Or should FooServiceImpl be considered more kind of a default implementation which could be overriden with an alternative implementation provided via some AppModule provider? But if this syntax is about default implementations is it required at all? I mean FooService could be that default implementation itself, then.

What am I missing? I do understand tree shaking and I actually do understand dependency injection but it seems to me like the concepts are fundamentally incompatible if something like the above is the only way to make it somehow work together. Would love to find out where I miss the point.

@mary-poppins
Copy link

You can preview e92c137 at https://pr23027-e92c137.ngbuilds.io/.

@jenniferfell jenniferfell added comp: docs target: major This PR is targeted for the next major release labels Mar 28, 2018
@jenniferfell jenniferfell added this to the v6-candidates milestone Mar 28, 2018
@jenniferfell
Copy link
Contributor

@alxhub Can you provide some insight on the question from @about-code ? Thanks!

@alxhub
Copy link
Member

alxhub commented Mar 28, 2018

@about-code you're not missing anything, you actually answered your own question.

Or should FooServiceImpl be considered more kind of a default implementation which could be overriden with an alternative implementation provided via some AppModule provider?

Yes, exactly.

But if this syntax is about default implementations is it required at all? I mean FooService could be that default implementation itself, then.

Yes, it could:

@Injectable({providedIn: 'root'})
export class FooService {
  ...
}

Every token must have a provider in order to be injected. @NgModule allowed you to define that provider in 4 ways: class, existing, factory, and value. @Injectable has the same level of flexibility, but moving the definition onto the type itself (as a sort of "default provider" as you called it) allows the type to be removed if it's not referenced.

@jenniferfell
Copy link
Contributor

@alxhub : @chembu has incorporated the code snippets from your example. Time for approval review, meaning:

  1. Make sure all changes you requested previously are incorporated
  2. Check that any reviewer comments on this thread have been incorporated into doc to your satisfaction
  3. Check for any show-stoppers.
  4. Indicate your approval so pull-approve passes.

I'm doing the same as co-approver from doc team.

Please review by EOD 3/30. We need to have any final critical changes incorporated by @chembu , your approval, and mark for merge by EOD 4/2. Thanks! (target is next RC)

@jenniferfell
Copy link
Contributor

@IgorMinar Can you also provide an approval. We need an approval from you or someone else in the guide-and-tutorial group. This PR doesn't touch tutorial files, just ngmodule and DI files, but pull-approve is asking for the guide-and-tutorial group. Thanks!

Copy link
Contributor

@jenniferfell jenniferfell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some code snippets directly embedded here. Will those be coming from a live example soon?

Lots of notes, but I don't think there are any show stoppers. Some suggestions for clarifying sections that were hard to parse. Some replacements of which with that for accuracy. A few typos. If they don't make it in for this merge, please capture externally somehow for the next rev.

@@ -4663,7 +4663,7 @@ Compare with the less preferred `host` metadata alternative.



**Do** provide services to the Angular injector at the top-most component where they will be shared.
**Do** provide service with the app root injector in the `@Injectable` decorator of the service.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Feels like a word is missing. "Do provide a service with the app root injector..."

@@ -4685,8 +4685,7 @@ Compare with the less preferred `host` metadata alternative.



**Why?** When providing the service to a top level component,
that instance is shared and available to all child components of that top level component.
**Why?** When providing the service to a root injector, that instance of the service is shared and available in every class that needs the service. Additionally, when you register a service in the `@Injectable` decorator of the service, optimization tools such as those used by the CLI's production builds can perform tree shaking and remove services that aren't used by your app.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Who provides the service to a root injector? The way this is phrased "that instance of the service" is what provides the service to a root injector. I think the intent is "When you provide the service to a root injector..." ?

@@ -4685,8 +4685,7 @@ Compare with the less preferred `host` metadata alternative.



**Why?** When providing the service to a top level component,
that instance is shared and available to all child components of that top level component.
**Why?** When providing the service to a root injector, that instance of the service is shared and available in every class that needs the service. Additionally, when you register a service in the `@Injectable` decorator of the service, optimization tools such as those used by the CLI's production builds can perform tree shaking and remove services that aren't used by your app.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: I would break this into two "why" statement. The first is about scope. The second is about tree-shaking. I would also pull the 3rd why ("This is ideal when a service is sharing methods or state.") up to be part of the first half of this. "When you provide the service to a root injector, that instance of the service is .... This is ideal when.... "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -198,7 +198,7 @@ Its only purpose is to add http service providers to the application as a whole.

## What is the `forRoot()` method?

The `forRoot()` static method is a convention that makes it easy for developers to configure the module's providers.
The `forRoot()` static method is a convention that makes it easy for developers to configure services and providers for a module which are intended to be singletons. For services it is preferable to specify `providedIn: 'root'` on the service's `@Injectable()` decorator, which has the same effect.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: This is hard to parse. Does "which" refer to the services and providers? Or to modules? It also seems like the clause is restrictive, so "that" instead of "which".

"...configure services and providers that are intended to be singletons"?
"...configure services and providers for modules that are intended to be singletons"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -198,7 +198,7 @@ Its only purpose is to add http service providers to the application as a whole.

## What is the `forRoot()` method?

The `forRoot()` static method is a convention that makes it easy for developers to configure the module's providers.
The `forRoot()` static method is a convention that makes it easy for developers to configure services and providers for a module which are intended to be singletons. For services it is preferable to specify `providedIn: 'root'` on the service's `@Injectable()` decorator, which has the same effect.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Instead of "it is preferable", I recommend: "For a service, instead of using forRoot(), specify providedIn: 'root' on the service's @Injectable() decorator. This has the same effect, and is preferred." Because....can we say why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explained why to use @Injectable() decorator


## Component child injectors

For example, when Angular creates a new instance of a component that has `@Component.providers`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Rearrange so there isn't a heading followed by "For example." This structure breaks up the context of what is being demonstrated. It also is confusing for someone who uses the right nav to jump to this section.

Component injectors are independent of each other and
each of them creates its own instances of the component-provided services.

When Angular destroys one of these component instance, it also destroys the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Missing s in "one of these component instances"

When Angular destroys one of these component instance, it also destroys the
component's injector and that injector's service instances.

Thanks to [injector inheritance](guide/hierarchical-dependency-injection),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Because of

The token description is another developer aid.

Register the dependency provider using the `InjectionToken` object:
Like with `@Injectable`, you can directly configure a provider when creating an InjectionToken, that will determine which injector provides the token and how the value will be created. But, unlike with `@Injectable`, you cannot define standard providers (such as `useClass` or `useFactory`) with `InjectionToken`. Instead, you specify a factory function which returns the value to be provided directly:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: I'm not sure of the right answer technically, but the paragraph above doesn't parse well and has grammar issues. One attempt:

You can directly configure a provider when creating an InjectionToken. Directly configuring a provider determines which injector provides the token and how the value is created. Using an InjectionToken is similar to using @Injectable, expect that you cannot define standard providers such as useClass or useFactory. Instead, you specify a factory function that returns the value to be provided directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-phrased.


<code-example path="dependency-injection/src/app/providers.component.ts" region="providers-9" linenums="false">
<code-example>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Examples: Will these code snippets be coming from live examples?

@about-code
Copy link

@alxhub Thank you for your clarification.

Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, commented a couple of times for technical clarity.

@jenniferfell jenniferfell changed the title [WIP}: add tree-shakable providers doc add tree-shakable providers doc Mar 30, 2018
@mary-poppins
Copy link

You can preview 26fe94d at https://pr23027-26fe94d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 63e7dde at https://pr23027-63e7dde.ngbuilds.io/.

@jenniferfell
Copy link
Contributor

@IgorMinar Can you also provide an approval. We need an approval from you or someone else in the guide-and-tutorial group. This PR doesn't touch tutorial files, just ngmodule and DI files, but pull-approve is asking for the guide-and-tutorial group. AlexR has approved. Thanks!

@IgorMinar
Copy link
Contributor

lgtm

@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label Apr 3, 2018
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants