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

feat(compiler-cli): lower some exported expressions #30038

Closed
wants to merge 1 commit into from

Conversation

@alxhub
Copy link
Contributor

commented Apr 22, 2019

The compiler uses metadata to represent what it statically knows about
various expressions in a program. Occasionally, expressions in the program
for which metadata is extracted may contain sub-expressions which are not
representable in metadata. One such construct is an arrow function.

The compiler does not always need to understand such expressions completely.
For example, for a provider defined with useValue, the compiler does not
need to understand the value at all, only the outer provider definition. In
this case, the compiler employs a technique known as "expression lowering",
where it rewrites the provider expression into one that can be represented
in metadata. Chiefly, this involves extracting out the dynamic part (the
useValue expression) into an exported constant.

Lowering is applied through a heuristic, which considers the containing
statement as well as the field name of the expression.

Previously, this heuristic was not completely accurate in the case of
route definitions and the loadChildren field, which is lowered. If the
route definition using loadChildren existed inside a decorator invocation,
lowering was performed correctly. However, if it existed inside a standalone
variable declaration with an export keyword, the heuristic would conclude
that lowering was unnecessary. For ordinary providers this is true; however
the compiler attempts to fully understand the ROUTES token and thus even if
an array of routes is declared in an exported variable, any loadChildren
expressions within still need to be lowered.

This commit enables lowering of already exported variables under a limited
set of conditions (where the initializer expression is of a specific form).
This should enable the use of loadChildren in route definitions.

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

@alxhub alxhub requested a review from angular/fw-compiler as a code owner Apr 22, 2019

@googlebot googlebot added the cla: yes label Apr 22, 2019

feat(compiler-cli): lower some exported expressions
The compiler uses metadata to represent what it statically knows about
various expressions in a program. Occasionally, expressions in the program
for which metadata is extracted may contain sub-expressions which are not
representable in metadata. One such construct is an arrow function.

The compiler does not always need to understand such expressions completely.
For example, for a provider defined with `useValue`, the compiler does not
need to understand the value at all, only the outer provider definition. In
this case, the compiler employs a technique known as "expression lowering",
where it rewrites the provider expression into one that can be represented
in metadata. Chiefly, this involves extracting out the dynamic part (the
`useValue` expression) into an exported constant.

Lowering is applied through a heuristic, which considers the containing
statement as well as the field name of the expression.

Previously, this heuristic was not completely accurate in the case of
route definitions and the `loadChildren` field, which is lowered. If the
route definition using `loadChildren` existed inside a decorator invocation,
lowering was performed correctly. However, if it existed inside a standalone
variable declaration with an export keyword, the heuristic would conclude
that lowering was unnecessary. For ordinary providers this is true; however
the compiler attempts to fully understand the ROUTES token and thus even if
an array of routes is declared in an exported variable, any `loadChildren`
expressions within still need to be lowered.

This commit enables lowering of already exported variables under a limited
set of conditions (where the initializer expression is of a specific form).
This should enable the use of `loadChildren` in route definitions.

@alxhub alxhub force-pushed the alxhub:ngc/metadata/lowering branch from 978026b to b283720 Apr 22, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

// properly exported.
const varNode = node as ts.VariableDeclaration;
return isExported || (varNode.initializer !== undefined &&
(ts.isObjectLiteralExpression(varNode.initializer) ||

This comment has been minimized.

Copy link
@alexeagle

alexeagle Apr 22, 2019

Contributor

if we really want to reduce risk of this change, we could special-case only the import() callExpression, right? but if you're comfortable with handling more cases, that's great

This comment has been minimized.

Copy link
@alxhub

alxhub Apr 23, 2019

Author Contributor

It's tricky, because isEligibleForLowering recursively considers the whole statement. Another section of logic determines whether the expression needs lowering (for example, loadChildren: string expressions won't be lowered). This function only sets bounds on which statements are considered in the first place.

@kara

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

merge-assistance: alex E has global approval and alex R is the code owner for this anyway

@benlesh benlesh closed this in 8e73f9b Apr 23, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

feat(compiler-cli): lower some exported expressions (angular#30038)
The compiler uses metadata to represent what it statically knows about
various expressions in a program. Occasionally, expressions in the program
for which metadata is extracted may contain sub-expressions which are not
representable in metadata. One such construct is an arrow function.

The compiler does not always need to understand such expressions completely.
For example, for a provider defined with `useValue`, the compiler does not
need to understand the value at all, only the outer provider definition. In
this case, the compiler employs a technique known as "expression lowering",
where it rewrites the provider expression into one that can be represented
in metadata. Chiefly, this involves extracting out the dynamic part (the
`useValue` expression) into an exported constant.

Lowering is applied through a heuristic, which considers the containing
statement as well as the field name of the expression.

Previously, this heuristic was not completely accurate in the case of
route definitions and the `loadChildren` field, which is lowered. If the
route definition using `loadChildren` existed inside a decorator invocation,
lowering was performed correctly. However, if it existed inside a standalone
variable declaration with an export keyword, the heuristic would conclude
that lowering was unnecessary. For ordinary providers this is true; however
the compiler attempts to fully understand the ROUTES token and thus even if
an array of routes is declared in an exported variable, any `loadChildren`
expressions within still need to be lowered.

This commit enables lowering of already exported variables under a limited
set of conditions (where the initializer expression is of a specific form).
This should enable the use of `loadChildren` in route definitions.

PR Close angular#30038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.