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

Linker/components #39707

Closed
wants to merge 12 commits into from
Closed

Linker/components #39707

wants to merge 12 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Nov 16, 2020

See individual commits.

@JoostK JoostK added feature Issue that requests a new feature area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release labels Nov 16, 2020
@ngbot ngbot bot added this to the needsTriage milestone Nov 16, 2020
@google-cla google-cla bot added the cla: yes label Nov 16, 2020
@JoostK JoostK force-pushed the linker/components branch 3 times, most recently from bbf6d70 to 1d0ff5a Compare November 16, 2020 20:05
* The following symbols are only referenced from partial declaration compilation outputs, which
* will never be emitted by the JIT compiler so are allowed to be omitted from the JIT environment.
*/
const PARTIAL_ONLY = new Set<string>([
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to exclude these symbols from this test, as having them in the JIT environment is entirely unnecessary. As an alternative, it may be a good idea to move these symbols away from the Identifiers object into a separate object, which would make it more apparent that those symbols are "special".

return value.getOpaque();
}
}) :
new Map<string, o.Expression>();
Copy link
Member Author

Choose a reason for hiding this comment

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

And the award for best code formatter of the month November goes to.... not clang-format I guess.

Copy link
Member

Choose a reason for hiding this comment

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

It's just CLANG trying to tell you to put that inline function into a named function :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I noticed this may benefit from some splitting up; I didn't put much thought into it until now.

@JoostK JoostK marked this pull request as ready for review November 16, 2020 21:52
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 16, 2020
@JoostK JoostK requested review from petebacondarwin and removed request for AndrewKushnir November 16, 2020 21:53
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Really nice PR @JoostK!
I love the early refactoring commits. Made it really straightforward to review.
I left a bunch of minor things that you can clean up etc.
The other refactoring that we noted can be done in later PRs.

One question I have is why do all pipes and directives need to be wrapped in a closure if any one of them is?

Comment on lines +59 to +60
preserveWhitespaces:
metaObj.has('preserveWhitespaces') ? metaObj.getBoolean('preserveWhitespaces') : false,
Copy link
Member

Choose a reason for hiding this comment

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

In normal builds this can be affected by a tsconfig option, I believe.
Should the linker be influenced similarly?

I think it might add too much complexity to the CLI integration. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have always found it incorrect, or strange at least, that application flags can influence library compilation like this. We can fairly simply add preserveWhitespaces as linker option which could then replace the default here. But it also requires that we actively track in R3ComponentMetadata whether the value is explicitly set or not.

I'll put this as a task on the tracker, for future consideration.

packages/compiler/src/render3/partial/component.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/partial/component.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/partial/component.ts Outdated Show resolved Hide resolved
@petebacondarwin petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 17, 2020
@JoostK JoostK added this to In progress in ng-linker via automation Nov 18, 2020
@JoostK JoostK removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 18, 2020
…ompliance tests

The JSON schema reference was off-by-one, preventing IDEs from finding
the file and offering suggestions and documentation. Additionally the
name of the golden file was slightly off.
The type checker had to do extensive work in resolving the
`NodePath.get` method call for the `NodePath` that had an intersection
type of `ts.VariableDeclarator&{init:t.Expression}`. The `NodePath.get`
method is typed using a conditional type which became expensive to
compute with this intersection type. As a workaround, the original
`init` property is explicitly omitted which avoids the performance
cliff. This brings down the compile time by 15s.
…tive linker

The compilation result of components may have inserted template
functions into the constant pool, which would be inserted into the Babel
AST upon program exit. Babel will then proceed with visiting this newly
inserted subtree, but we have already cleaned up the linker instance
when exiting the program. Any call expressions within the template
functions would then fail to be processed, as a file linker would no
longer be available.

Since the inserted AST subtree is known not to contain yet more partial
declarations, it is safe to skip visiting call expressions when no
file linker is available.
The metadata specification of queries allows for the boolean properties
`first`, `descendants` and `static` to be missing, but the linker did
not account for their omission.

This fix is tested in subsequent commits that implement compilation of
components, at which point this will be covered by the compliance tests.
…pilation of components

This commit is a precursor to supporting the partial compilation of
components, which leverages some of the compilation infrastructure that
is in place for directives.
…ponents

This commit adds the `i18nUseExternalIds` option to the linker options,
as the compliance tests exercise compilation results with and without
this flag enabled. We therefore need to configure the linker to take
this option into account, as otherwise the compliance test output would
not be identical.

Additionally, this commit switches away from spread syntax to set
the default options. This introduced a problem when the user-provided
options object did specify the keys, but with an undefined value. This
would have prevented the default options from being applied.
… compilation tests

The linker does not currently support outputting ES5 syntax, so any
compliance tests that request ES5 output cannot be run in partial
compilation mode. This commit marks these tests as pending.
…e in compliance tests

In production mode this flag defaults to `true`, but the compliance
tests override this to `false` unless it is provided. As such, the
linker should also adhere to this default as otherwise the compilation
output would not align with the output of the full tests.

There are still tests that exercise the value of this flag, together
with it being `undefined` to verify the behavior of the actual default
value.
This commit implements partial compilation of components, together with
linking the partial declaration into its full AOT output.

This commit does not yet enable accurate source maps into external
templates. This requires additional work to account for escape sequences
which is non-trivial. Inline templates that were represented using a
string or template literal are transplated into the partial declaration
output, so their source maps should be accurate. Note, however, that
the accuracy of source maps is not currently verified in tests; this is
also left as future work.

The golden files of partial compilation output have been updated to
reflect the generated code for components. Please note that the current
output should not yet be considered stable.
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

LGTM (for fw-core) 👍

@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir
Copy link
Contributor

FYI, presubmit is successful for the changes in this PR. Thank you.

@JoostK JoostK added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 24, 2020
AndrewKushnir pushed a commit that referenced this pull request Nov 24, 2020
…39707)

The type checker had to do extensive work in resolving the
`NodePath.get` method call for the `NodePath` that had an intersection
type of `ts.VariableDeclarator&{init:t.Expression}`. The `NodePath.get`
method is typed using a conditional type which became expensive to
compute with this intersection type. As a workaround, the original
`init` property is explicitly omitted which avoids the performance
cliff. This brings down the compile time by 15s.

PR Close #39707
ng-linker automation moved this from In progress to Done Nov 24, 2020
AndrewKushnir pushed a commit that referenced this pull request Nov 24, 2020
…tive linker (#39707)

The compilation result of components may have inserted template
functions into the constant pool, which would be inserted into the Babel
AST upon program exit. Babel will then proceed with visiting this newly
inserted subtree, but we have already cleaned up the linker instance
when exiting the program. Any call expressions within the template
functions would then fail to be processed, as a file linker would no
longer be available.

Since the inserted AST subtree is known not to contain yet more partial
declarations, it is safe to skip visiting call expressions when no
file linker is available.

PR Close #39707
AndrewKushnir pushed a commit that referenced this pull request Nov 24, 2020
The metadata specification of queries allows for the boolean properties
`first`, `descendants` and `static` to be missing, but the linker did
not account for their omission.

This fix is tested in subsequent commits that implement compilation of
components, at which point this will be covered by the compliance tests.

PR Close #39707
AndrewKushnir pushed a commit that referenced this pull request Nov 24, 2020
…pilation of components (#39707)

This commit is a precursor to supporting the partial compilation of
components, which leverages some of the compilation infrastructure that
is in place for directives.

PR Close #39707
AndrewKushnir pushed a commit that referenced this pull request Nov 24, 2020
…ponents (#39707)

This commit adds the `i18nUseExternalIds` option to the linker options,
as the compliance tests exercise compilation results with and without
this flag enabled. We therefore need to configure the linker to take
this option into account, as otherwise the compliance test output would
not be identical.

Additionally, this commit switches away from spread syntax to set
the default options. This introduced a problem when the user-provided
options object did specify the keys, but with an undefined value. This
would have prevented the default options from being applied.

PR Close #39707
AndrewKushnir pushed a commit that referenced this pull request Nov 24, 2020
… compilation tests (#39707)

The linker does not currently support outputting ES5 syntax, so any
compliance tests that request ES5 output cannot be run in partial
compilation mode. This commit marks these tests as pending.

PR Close #39707
AndrewKushnir pushed a commit that referenced this pull request Nov 24, 2020
…e in compliance tests (#39707)

In production mode this flag defaults to `true`, but the compliance
tests override this to `false` unless it is provided. As such, the
linker should also adhere to this default as otherwise the compilation
output would not align with the output of the full tests.

There are still tests that exercise the value of this flag, together
with it being `undefined` to verify the behavior of the actual default
value.

PR Close #39707
AndrewKushnir pushed a commit that referenced this pull request Nov 24, 2020
…9707)

This commit implements partial compilation of components, together with
linking the partial declaration into its full AOT output.

This commit does not yet enable accurate source maps into external
templates. This requires additional work to account for escape sequences
which is non-trivial. Inline templates that were represented using a
string or template literal are transplated into the partial declaration
output, so their source maps should be accurate. Note, however, that
the accuracy of source maps is not currently verified in tests; this is
also left as future work.

The golden files of partial compilation output have been updated to
reflect the generated code for components. Please note that the current
output should not yet be considered stable.

PR Close #39707
@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 Dec 25, 2020
@pullapprove pullapprove bot added the area: core Issues related to the framework runtime label Dec 25, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Dec 25, 2020
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 area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime cla: yes feature Issue that requests a new feature target: minor This PR is targeted for the next minor release
Projects
No open projects
ng-linker
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants