refactor(compiler-cli): make the output AST translator generic by petebacondarwin · Pull Request #38775 · angular/angular

petebacondarwin · 2020-09-09T21:06:13Z

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.

JoostK

I did a very quick initial pass over this, I plan to go over it again.

josephperrott

LGTM,

Reviewed-for: dev-infra

AndrewKushnir · 2020-09-21T17:32:32Z

Presubmit #2.

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.

These imports are not used and so are just bloating the code unnecessarily PR Close #38775

…38775) 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. 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

…ator (#38775) 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. 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

alfaproject · 2020-09-28T17:01:08Z

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?

JoostK · 2020-09-28T17:04:52Z

@alfaproject ngcc uses this machinery to emit code into ES5 bundles of libraries.

petebacondarwin · 2020-09-28T17:45:02Z

In the long run we would not but for now there are plenty of libraries and project setups that rely upon ES5

alfaproject · 2020-09-28T22:25:13Z

Got it. Thank you for explaining. (:

angular-automatic-lock-bot · 2020-10-29T16:34:41Z

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.}

petebacondarwin added state: WIP area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release labels Sep 9, 2020

ngbot Bot modified the milestone: needsTriage Sep 9, 2020

googlebot added the cla: yes label Sep 9, 2020

petebacondarwin force-pushed the linker-translator branch 8 times, most recently from 7a1eadb to 5a0fa33 Compare September 15, 2020 20:02

petebacondarwin mentioned this pull request Sep 15, 2020

refactor(compiler): replace Comment nodes with leadingComments property #38811

Closed

petebacondarwin force-pushed the linker-translator branch 2 times, most recently from 227d5b2 to 8accc38 Compare September 15, 2020 21:16

petebacondarwin added state: blocked and removed state: WIP labels Sep 16, 2020

petebacondarwin marked this pull request as ready for review September 16, 2020 09:44

pullapprove Bot requested review from AndrewKushnir and josephperrott September 16, 2020 09:44

petebacondarwin requested review from JoostK and gkalpak September 16, 2020 09:44

petebacondarwin force-pushed the linker-translator branch from 8accc38 to 1a03a00 Compare September 16, 2020 10:21

JoostK reviewed Sep 16, 2020

View reviewed changes

petebacondarwin requested a review from JoostK September 16, 2020 12:04

josephperrott approved these changes Sep 16, 2020

View reviewed changes

JoostK reviewed Sep 16, 2020

View reviewed changes

Comment thread packages/compiler-cli/src/ngtsc/translator/src/typescript_ast_factory.ts Outdated

Comment thread packages/compiler-cli/src/ngtsc/translator/src/api/ast_factory.ts Outdated

Comment thread packages/compiler-cli/src/ngtsc/translator/src/api/ast_factory.ts Outdated

petebacondarwin force-pushed the linker-translator branch from 368aec7 to e808b9f Compare September 21, 2020 13:36

petebacondarwin mentioned this pull request Sep 21, 2020

Linker babel translator #38866

Closed

AndrewKushnir removed their request for review September 21, 2020 18:57

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Sep 21, 2020

petebacondarwin 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 Sep 21, 2020

petebacondarwin added 8 commits September 21, 2020 20:27

refactor(compiler-cli): remove undesirable cast in the type translator

c9b7180

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.

refactor(compiler-cli): remove unused imports

9e8ed61

These imports are not used and so are just bloating the code unnecessarily

refactor(compiler): use a named type for cooked/raw string objects

c8d9bb0

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.

fix(compiler-cli): generate let statements in ES2015+ mode

5c9b842

When the target of the compiler is ES2015 or newer then we should be generating `let` and `const` variable declarations rather than `var`.

refactor(compiler-cli): split up translator file

a38218c

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.

refactor(compiler-cli): simplify imports from compiler to type transl…

0bc4d83

…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.

fixup! refactor(compiler-cli): make the output AST translator generic

46cab61

petebacondarwin force-pushed the linker-translator branch from 6484e91 to 46cab61 Compare September 21, 2020 19:27

mhevery closed this in 856e74a Sep 21, 2020

mhevery pushed a commit that referenced this pull request Sep 21, 2020

refactor(compiler-cli): remove unused imports (#38775)

b0a4387

These imports are not used and so are just bloating the code unnecessarily PR Close #38775

petebacondarwin deleted the linker-translator branch September 21, 2020 19:29

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Oct 29, 2020

Conversation

petebacondarwin commented Sep 9, 2020 • edited by gkalpak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented Sep 21, 2020

Uh oh!

alfaproject commented Sep 28, 2020

Uh oh!

JoostK commented Sep 28, 2020

Uh oh!

petebacondarwin commented Sep 28, 2020

Uh oh!

alfaproject commented Sep 28, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Oct 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

petebacondarwin commented Sep 9, 2020 •

edited by gkalpak

Loading