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
refactor(compiler-cli): make the output AST translator generic #38775
Conversation
a6e7c76
to
6b43548
Compare
7a1eadb
to
5a0fa33
Compare
227d5b2
to
8accc38
Compare
8accc38
to
1a03a00
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.
I did a very quick initial pass over this, I plan to go over it again.
packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/translator/src/api/ast_factory.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/translator/src/api/ast_factory.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/translator/src/api/ast_factory.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/translator/src/api/import_generator.ts
Outdated
Show resolved
Hide resolved
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,
Reviewed-for: dev-infra
The cast to `ts.Identifier` was a hack that "just happened to work". The new approach is more robust and doesn't have to undermine the type checker.
These imports are not used and so are just bloating the code unnecessarily
Using an interface makes the code cleaner and more readable. This change also adds the `range` property to the type to be used for source-mapping.
When the target of the compiler is ES2015 or newer then we should be generating `let` and `const` variable declarations rather than `var`.
This file contains a number of classes making it long and hard to work with. This commit splits the `ImportManager`, `Context` and `TypeTranslatorVisitor` classes, along with associated functions and types into their own files.
…ator Previously each identifier was being imported individually, which made for a very long import statement, but also obscurred, in the code, which identifiers came from the compiler.
This commit refactors the `ExpressionTranslatorVisitor` so that it is not tied directly to the TypeScript AST. Instead it uses generic `TExpression` and `TStatement` types that are then converted to concrete types by the `TypeScriptAstFactory`. This paves the way for a `BabelAstFactory` that can be used to generate Babel AST nodes instead of TypeScript, which will be part of the new linker tool.
6484e91
to
46cab61
Compare
These imports are not used and so are just bloating the code unnecessarily PR Close #38775
When the target of the compiler is ES2015 or newer then we should be generating `let` and `const` variable declarations rather than `var`. PR Close #38775
This file contains a number of classes making it long and hard to work with. This commit splits the `ImportManager`, `Context` and `TypeTranslatorVisitor` classes, along with associated functions and types into their own files. PR Close #38775
This commit refactors the `ExpressionTranslatorVisitor` so that it is not tied directly to the TypeScript AST. Instead it uses generic `TExpression` and `TStatement` types that are then converted to concrete types by the `TypeScriptAstFactory`. This paves the way for a `BabelAstFactory` that can be used to generate Babel AST nodes instead of TypeScript, which will be part of the new linker tool. PR Close #38775
OOC, do we need to care about ES5 at all? I thought that we started delegating the responsibility of downleveling to the building tool, like Angular CLI or whatever else, right? |
@alfaproject ngcc uses this machinery to emit code into ES5 bundles of libraries. |
In the long run we would not but for now there are plenty of libraries and project setups that rely upon ES5 |
Got it. Thank you for explaining. (: |
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. |
This commit refactors the
ExpressionTranslatorVisitor
so that itis not tied directly to the TypeScript AST. Instead it uses generic
TExpression
andTStatement
types that are then convertedto concrete types by the
TypeScriptAstFactory
.This paves the way for a
BabelAstFactory
that can be used togenerate Babel AST nodes instead of TypeScript, which will be
part of the new linker tool.