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

docs: update examples for tree-shakeable providers #22961

Closed
wants to merge 1 commit into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Mar 23, 2018

No description provided.

@alxhub alxhub added the target: major This PR is targeted for the next major release label Mar 23, 2018
@mary-poppins
Copy link

You can preview 5c10e42 at https://pr22961-5c10e42.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 854a794 at https://pr22961-854a794.ngbuilds.io/.

@alxhub alxhub added the action: merge The PR is ready for merge by the caretaker label Mar 27, 2018
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

I am not sure what the purpose of the PR is 😁
The examples look fine (afaict), but the main problems I see are that:

  1. The corresponding guides need to be updated to match the new code snippets.
  2. There are some files that don't seem to be used for anything.
  3. Some files that are no longer used should be removed (e.g. dependency-injector/src/app/heroes/hero.service.provider.ts).
  4. Several files are included in the live-example/downloadable zip, that may leave the users confused (e.g. what is HeroesTspComponent, what is it used for, what does Tsp stand for, etc).

But yay! for tree-shakeable injectors nonetheless \o/ ❤️

@@ -8,6 +8,5 @@ import { HeroService } from './heroes';
template: `
<toh-heroes></toh-heroes>
`,
providers: [HeroService]
Copy link
Member

Choose a reason for hiding this comment

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

The correspoding recommendation should be changed accordingly: https://pr22961-854a794.ngbuilds.io/guide/styleguide#style-07-03.

Copy link
Member Author

Choose a reason for hiding this comment

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

Guide updates are happening in a separate PR.

@@ -6,7 +6,6 @@ import { UserService } from './user.service';

@NgModule({
imports: [ BrowserModule ],
providers: [ UserService ],
Copy link
Member

Choose a reason for hiding this comment

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

The corresponding guide section should be updated accordingly: https://pr22961-854a794.ngbuilds.io/guide/providers#create-a-service

Copy link
Member Author

Choose a reason for hiding this comment

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

Guide updates are happening in a separate PR.

@@ -0,0 +1,9 @@
import { NgModule } from '@angular/core';
Copy link
Member

Choose a reason for hiding this comment

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

Where is this file used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I should've given some context. This is the predecessor PR to the docs updates for tree-shakeable providers.

@@ -0,0 +1,7 @@
import { Injectable } from '@angular/core';
Copy link
Member

Choose a reason for hiding this comment

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

Where is this file used?

@@ -0,0 +1,8 @@
import { Injectable } from '@angular/core';
Copy link
Member

Choose a reason for hiding this comment

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

Where is this file used?

@@ -0,0 +1,14 @@
// #docregion
Copy link
Member

Choose a reason for hiding this comment

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

What is hero.service.1.2.ts??? I m not even sure our infrastructure can handle such names 😁

providedIn: 'root',
useFactory: (logger: Logger, userService: UserService) =>
new HeroService(logger, userService.user.isAuthorized),
deps: [Logger, UserService],
Copy link
Member

Choose a reason for hiding this comment

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

The corresponding guide section should be updated accordingly: https://pr22961-854a794.ngbuilds.io/guide/dependency-injection#factory-providers

@mary-poppins
Copy link

You can preview d3c4128 at https://pr22961-d3c4128.ngbuilds.io/.

@alxhub
Copy link
Member Author

alxhub commented Mar 27, 2018

  1. Guides will be updated in a future PR.
  2. That future PR will use the new examples.
  3. This file is used (by HeroesComponent).
  4. I added a comment to HeroesTspComponent explaining what it is.

@mary-poppins
Copy link

You can preview ce7e9e0 at https://pr22961-ce7e9e0.ngbuilds.io/.

@mary-poppins
Copy link

You can preview fd1f243 at https://pr22961-fd1f243.ngbuilds.io/.

@alxhub alxhub closed this in e1ea7ed Mar 28, 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

4 participants