Skip to content

Tree-Shakeable Tokens #22005

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

Closed
wants to merge 4 commits into from
Closed

Tree-Shakeable Tokens #22005

wants to merge 4 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Feb 2, 2018

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

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

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

...injectables.map(
ref => ({
summary: this._metadataResolver.getInjectableSummary(ref.symbol) !,
metadata: this._metadataResolver.getInjectableSummary(ref.symbol) !.type
Copy link
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
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.

if (_currentInjector === null) {
throw new Error(`inject() must be called from an injection context`);
}
return _currentInjector !.get(token, notFoundValue, flags);
Copy link
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.

data._providers[index] = UNDEFINED_VALUE;
data._providers[index] =
_createProviderInstance(data, data._def.providersByKey[depDef.tokenKey]);
return data._providers[index];
Copy link
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
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

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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

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

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
@@ -3,15 +3,15 @@
"master": {
"uncompressed": {
"inline": 1447,
"main": 154185,
"main": 159944,
Copy link
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?

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
This is needed so the rules produce metadata.json files, which is essential
for building compiler/integration tests with Bazel.

PR Close angular#22005
jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
…at runtime (angular#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 angular#22005
jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
…22005)

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 angular#22005
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
This is needed so the rules produce metadata.json files, which is essential
for building compiler/integration tests with Bazel.

PR Close angular#22005
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
…at runtime (angular#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 angular#22005
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
…22005)

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 angular#22005
@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.

7 participants