feat(core): initial API work for tree shakable injectors by tinayuangao · Pull Request #20850 · angular/angular

tinayuangao · 2017-12-07T00:40:49Z

PR Checklist

Please check if your PR fulfills the following requirements:

[x ] The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

Current Injectable don't take any parameters.

What is the new behavior?

Injectable can be used with NgModule and provider

Does this PR introduce a breaking change?

[ ] Yes
[x ] No

Other information

googlebot · 2017-12-07T00:40:53Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

mary-poppins · 2017-12-07T00:48:18Z

You can preview 408eaba at https://pr20850-408eaba.ngbuilds.io/.

mary-poppins · 2017-12-07T01:10:11Z

You can preview 25c7788 at https://pr20850-25c7788.ngbuilds.io/.

mary-poppins · 2017-12-07T01:23:23Z

You can preview e772936 at https://pr20850-e772936.ngbuilds.io/.

mary-poppins · 2017-12-07T01:33:26Z

You can preview b4fd66b at https://pr20850-b4fd66b.ngbuilds.io/.

mary-poppins · 2017-12-07T17:04:37Z

You can preview d6dad71 at https://pr20850-d6dad71.ngbuilds.io/.

mary-poppins · 2017-12-07T17:35:06Z

You can preview a59b047 at https://pr20850-a59b047.ngbuilds.io/.

mary-poppins · 2017-12-07T19:28:29Z

You can preview 6fd9547 at https://pr20850-6fd9547.ngbuilds.io/.

mhevery

Excellent work! We are a bit light on tests, but otherwise this is a very sold start.

Please address the comments, and I will have another look when ready

mhevery · 2017-12-08T05:52:00Z

this will end up as public API, and I don't think we want to point to the design doc from there. Maybe remove the design doc link?

mhevery · 2017-12-08T05:52:57Z

move this TS issue into a comment, so that it is not part of the Public API.

mhevery · 2017-12-08T06:15:55Z

I think we need to process this in different order.
Given

@NgModule({ provides: [{provide: Foo, useValue: 'fooChild'}] }) class ModuleChild {} @NgModule({ provides: [{provide: Foo, useValue: 'fooParent'}] imports: [ModuleChild] }) class ModuleParent {}

In the above case the ModuleParent's providers should override that of ModuleChild, which means that Foo should be fooParent not fooChild. The way it is being processed now the imports are being overwritten.

So I think we need to process providers first and than process providers.

Also please add a tests which demonstrates it.

mhevery · 2017-12-08T06:18:37Z

can you add a tests which shows that providerOrInjectorDefType.ngInjectorDef.type gets instantiated eagerly? Which means that we will probably have to collect all of them into some array and than when the injector is done constructing we need to iterate over the array and retrieve all of the types so that they get treated.

mhevery · 2017-12-08T06:22:13Z

using find is not as performant as doing a for loop over the array. Since retrieval is time sensitive could you use a for loop.

Also we should be looking up the token only if we don't have record since otherwise we are wasting time. Please move closer to the place of use.

mhevery · 2017-12-08T06:26:18Z

Also add record here so that we can cache the parent value. This will also increase performance.

mhevery · 2017-12-08T06:33:55Z

I don't think ngModule will ever be not defined. So this check is not needed.

mhevery · 2017-12-08T06:34:25Z

What is the TODO comment for?

mhevery · 2017-12-08T06:37:47Z

use Injector.create since that is the public API.

Until that TS bug gets fixed we should change the public API to take Type so that we can avoid these casts.

mhevery · 2017-12-08T06:39:26Z

We need to export all of the SenProviders as well

mary-poppins · 2017-12-11T20:27:35Z

You can preview a024bb2 at https://pr20850-a024bb2.ngbuilds.io/.

mary-poppins · 2017-12-11T20:31:44Z

You can preview 38258e1 at https://pr20850-38258e1.ngbuilds.io/.

mary-poppins · 2017-12-11T21:53:02Z

You can preview 7bd76e1 at https://pr20850-7bd76e1.ngbuilds.io/.

mary-poppins · 2018-01-03T18:35:28Z

You can preview cf724a4 at https://pr20850-cf724a4.ngbuilds.io/.

mary-poppins · 2018-01-03T19:57:00Z

You can preview 8e1cd1c at https://pr20850-8e1cd1c.ngbuilds.io/.

LinboLen · 2018-01-05T10:11:40Z

I can't understand title. I have a little knowledge about tree shakable. can anyone explain tree shakable injectors

trotyl · 2018-01-05T11:38:28Z

@LinboLen It make dropping unused injectables(services) possible for size saving purpose. (In combining with proper JavaScript bundler)

mary-poppins · 2018-01-08T17:49:01Z

You can preview 4f5c60c at https://pr20850-4f5c60c.ngbuilds.io/.

mary-poppins · 2018-01-08T18:08:59Z

You can preview 5b0759a at https://pr20850-5b0759a.ngbuilds.io/.

mary-poppins · 2018-01-08T23:43:59Z

You can preview 7b02a73 at https://pr20850-7b02a73.ngbuilds.io/.

mary-poppins · 2018-01-09T00:07:36Z

You can preview d5e098e at https://pr20850-d5e098e.ngbuilds.io/.

IgorMinar · 2018-01-09T07:48:45Z

We should merge this only after 5.2.0 is cut on Wednesday.

mary-poppins · 2018-01-10T07:52:33Z

You can preview dbf6255 at https://pr20850-dbf6255.ngbuilds.io/.

ngbot · 2018-01-24T00:55:59Z

Hi @tinayuangao! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

trotyl · 2018-02-16T14:17:46Z

Superseded by #22005.

mhevery · 2018-02-17T01:45:54Z

Closing this PR since @alxhub has taken over.

angular-automatic-lock-bot · 2019-09-13T08:21:14Z

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.}

googlebot added the cla: no label Dec 7, 2017

tinayuangao requested a review from mhevery December 7, 2017 00:41

tinayuangao force-pushed the module branch from 408eaba to 25c7788 Compare December 7, 2017 01:01

tinayuangao force-pushed the module branch 2 times, most recently from e772936 to b4fd66b Compare December 7, 2017 01:22

tinayuangao force-pushed the module branch from b4fd66b to d6dad71 Compare December 7, 2017 16:54

tinayuangao force-pushed the module branch from d6dad71 to a59b047 Compare December 7, 2017 17:24

tinayuangao force-pushed the module branch from a59b047 to 6fd9547 Compare December 7, 2017 19:20

mhevery suggested changes Dec 8, 2017

View reviewed changes

IgorMinar self-requested a review December 8, 2017 23:09

tinayuangao force-pushed the module branch 2 times, most recently from a024bb2 to 38258e1 Compare December 11, 2017 20:23

tinayuangao force-pushed the module branch from 38258e1 to 7bd76e1 Compare December 11, 2017 21:45

tinayuangao force-pushed the module branch from cf724a4 to 8e1cd1c Compare January 3, 2018 19:48

mhevery approved these changes Jan 4, 2018

View reviewed changes

tinayuangao force-pushed the module branch from 8e1cd1c to 4f5c60c Compare January 8, 2018 17:38

tinayuangao added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jan 8, 2018

tinayuangao force-pushed the module branch from 4f5c60c to 5b0759a Compare January 8, 2018 18:02

tinayuangao force-pushed the module branch from 5b0759a to 7b02a73 Compare January 8, 2018 23:37

feat(core): initial API work for tree shakable injectors

d5e098e

tinayuangao force-pushed the module branch from 7b02a73 to d5e098e Compare January 8, 2018 23:59

alexeagle added the cla: yes label Jan 9, 2018

trotyl mentioned this pull request Jan 9, 2018

Why can it work without specifying an operator (@) in services? #21382

Closed

IgorMinar added state: blocked and removed action: merge The PR is ready for merge by the caretaker labels Jan 9, 2018

WIP: updates to new API format

dbf6255

mhevery closed this Feb 17, 2018

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019

Conversation

tinayuangao commented Dec 7, 2017

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

googlebot commented Dec 7, 2017

Uh oh!

mary-poppins commented Dec 7, 2017

Uh oh!

mary-poppins commented Dec 7, 2017

Uh oh!

mary-poppins commented Dec 7, 2017

Uh oh!

mary-poppins commented Dec 7, 2017

Uh oh!

mary-poppins commented Dec 7, 2017

Uh oh!

mary-poppins commented Dec 7, 2017

Uh oh!

mary-poppins commented Dec 7, 2017

Uh oh!

mhevery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Dec 11, 2017

Uh oh!

mary-poppins commented Dec 11, 2017

Uh oh!

mary-poppins commented Dec 11, 2017

Uh oh!

mary-poppins commented Jan 3, 2018

Uh oh!

mary-poppins commented Jan 3, 2018

Uh oh!

LinboLen commented Jan 5, 2018

Uh oh!

trotyl commented Jan 5, 2018

Uh oh!

mary-poppins commented Jan 8, 2018

Uh oh!

mary-poppins commented Jan 8, 2018

Uh oh!

mary-poppins commented Jan 8, 2018

Uh oh!

mary-poppins commented Jan 9, 2018

Uh oh!

IgorMinar commented Jan 9, 2018

Uh oh!

mary-poppins commented Jan 10, 2018

Uh oh!

ngbot Bot commented Jan 24, 2018

Uh oh!

trotyl commented Feb 16, 2018

Uh oh!

mhevery commented Feb 17, 2018

Uh oh!