Skip to content

Tree-Shakeable Tokens - #22005

Closed
alxhub wants to merge 4 commits into
angular:masterfrom
alxhub:injectable-lite
Closed

alxhub wants to merge 4 commits into
angular:masterfrom
alxhub:injectable-lite

Conversation

@alxhub

@alxhub alxhub commented Feb 2, 2018

Copy link
Copy Markdown
Member

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
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

@mary-poppins

Copy link
Copy Markdown

You can preview a87b87b at https://pr22005-a87b87b.ngbuilds.io/.

@alxhub
alxhub force-pushed the injectable-lite branch 2 times, most recently from 1a952ae to c9bf452 Compare February 5, 2018 16:24
@mary-poppins

Copy link
Copy Markdown

You can preview 1a952ae at https://pr22005-1a952ae.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview c9bf452 at https://pr22005-c9bf452.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview 16fdc6d at https://pr22005-16fdc6d.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview 440b225 at https://pr22005-440b225.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview 49c7e69 at https://pr22005-49c7e69.ngbuilds.io/.

Comment thread packages/compiler/src/aot/compiler.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was here before, but do we really need to make this call twice?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It'd be tricky to unify these two, and for symmetry with the calls above I'm okay with the duplication. getInjectableSummary has a cache.

Comment thread packages/core/src/di/injector.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

_currentInjector should have been inferred non-null here, so the ! seems redundant.

Comment thread packages/core/src/view/ng_module.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might as well return the above assignment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

@mary-poppins

Copy link
Copy Markdown

You can preview 2e39934 at https://pr22005-2e39934.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview 41f5007 at https://pr22005-41f5007.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview 1fa4a6a at https://pr22005-1fa4a6a.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview eecb54b at https://pr22005-eecb54b.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview 101c73e at https://pr22005-101c73e.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview a3597d8 at https://pr22005-a3597d8.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview 04a920c at https://pr22005-04a920c.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview e0341d9 at https://pr22005-e0341d9.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview aa7cfbf at https://pr22005-aa7cfbf.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview 24a1d4c at https://pr22005-24a1d4c.ngbuilds.io/.

@alxhub alxhub added the target: major This PR is targeted for the next major release label Feb 12, 2018
@alxhub
alxhub dismissed chuckjaz’s stale review February 12, 2018 19:27

Comments addressed!

@mary-poppins

Copy link
Copy Markdown

You can preview dae8341 at https://pr22005-dae8341.ngbuilds.io/.

This commit bundles 3 important changes, with the goal of enabling tree-shaking
of services which are never injected. Ordinarily, this tree-shaking is prevented
by the existence of a hard dependency on the service by the module in which it
is declared.

Firstly, @Injectable() is modified to accept a 'scope' parameter, which points
to an @NgModule(). This reverses the dependency edge, permitting the module to
not depend on the service which it "provides".

Secondly, the runtime is modified to understand the new relationship created
above. When a module receives a request to inject a token, and cannot find that
token in its list of providers, it will then look at the token for a special
ngInjectableDef field which indicates which module the token is scoped to. If
that module happens to be in the injector, it will behave as if the token
itself was in the injector to begin with.

Thirdly, the compiler is modified to read the @Injectable() metadata and to
generate the special ngInjectableDef field as part of TS compilation, using the
PartialModules system.

Additionally, this commit adds several unit and integration tests of various
flavors to test this change.
@mary-poppins

Copy link
Copy Markdown

You can preview 7700d3c at https://pr22005-7700d3c.ngbuilds.io/.

@alxhub alxhub added the action: merge The PR is ready for merge by the caretaker label Feb 12, 2018
@mhevery mhevery closed this in 0a1a397 Feb 12, 2018
mhevery pushed a commit that referenced this pull request Feb 12, 2018
This is needed so the rules produce metadata.json files, which is essential
for building compiler/integration tests with Bazel.

PR Close #22005
mhevery pushed a commit that referenced this pull request Feb 12, 2018
…at runtime (#22005)

All of the providers in a module get compiled into a module definition in the
factory file. Some of these providers are for the actual module types, as those
are available for injection in Angular. For tree-shakeable tokens, the runtime
needs to be able to distinguish which modules are present in an injector.

This change adds a NodeFlag which tags those module providers for later
identification.

PR Close #22005
mhevery pushed a commit that referenced this pull request Feb 12, 2018
This commit bundles 3 important changes, with the goal of enabling tree-shaking
of services which are never injected. Ordinarily, this tree-shaking is prevented
by the existence of a hard dependency on the service by the module in which it
is declared.

Firstly, @Injectable() is modified to accept a 'scope' parameter, which points
to an @NgModule(). This reverses the dependency edge, permitting the module to
not depend on the service which it "provides".

Secondly, the runtime is modified to understand the new relationship created
above. When a module receives a request to inject a token, and cannot find that
token in its list of providers, it will then look at the token for a special
ngInjectableDef field which indicates which module the token is scoped to. If
that module happens to be in the injector, it will behave as if the token
itself was in the injector to begin with.

Thirdly, the compiler is modified to read the @Injectable() metadata and to
generate the special ngInjectableDef field as part of TS compilation, using the
PartialModules system.

Additionally, this commit adds several unit and integration tests of various
flavors to test this change.

PR Close #22005
"uncompressed": {
"inline": 1447,
"main": 154185,
"main": 159944,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this change (while awesome!) increased cli hello world by 5.8kb and closure hello world by 4kb. Do we understand why? Is this size increase justified? @alxhub can you please comment?

@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

7 participants