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
Linker/components #39707
Conversation
bbf6d70
to
1d0ff5a
Compare
* 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>([ |
There was a problem hiding this comment.
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>(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
packages/compiler-cli/linker/babel/test/ast/babel_ast_host_spec.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/linker/babel/test/ast/babel_ast_host_spec.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/linker/babel/test/ast/babel_ast_host_spec.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/linker/test/ast/typescript/typescript_ast_host_spec.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/linker/test/ast/typescript/typescript_ast_host_spec.ts
Outdated
Show resolved
Hide resolved
preserveWhitespaces: | ||
metaObj.has('preserveWhitespaces') ? metaObj.getBoolean('preserveWhitespaces') : false, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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-cli/linker/src/file_linker/partial_linkers/partial_component_linker_1.ts
Outdated
Show resolved
Hide resolved
1d0ff5a
to
bd4bdb3
Compare
…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.
…t an active linker
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.
bd4bdb3
to
53e0bf3
Compare
There was a problem hiding this 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
) 👍
FYI, presubmit is successful for the changes in this PR. Thank you. |
…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
…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
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
…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
…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
…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
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
See individual commits.