fix(ivy): ngcc should not emit TypeScript syntax by JoostK · Pull Request #27051 · angular/angular

JoostK · 2018-11-11T14:27:35Z

Previously, ngcc used ngtsc's logic to transform the Angular AST
to a TypeScript AST and then leverages TypeScript's node printer to
obtain the source text to insert. This turns out to be a problem when
specific TypeScript syntax is represented in the AST; the printer cannot
be configured to output only JavaScript syntax.

A full-blown TypeScript emit of a complete ts.SourceFile would be
required to be able to emit JS and possibly downlevel into a lower
language target, which is not an option for ngcc as it currently
operates on partial ASTs, not full source files.

Instead, the Angular AST itself is used to emit source files, using the
emitter logic that is also used in JIT compilations. As the Angular AST
produced by ngtsc may contain literal TypeScript nodes inside of
WrappedNodeExpr, a TypeScript printer is still used to obtain source
text for such nodes (assuming no TypeScript specific syntax is present
within that node's subtree).

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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?

ngcc may output TypeScript specific syntax into JavaScript bundles, which fail to be parsed.

What is the new behavior?

ngcc uses existing emitter logic that operates on Angular AST to produce JavaScript code instead of TypeScript.

Does this PR introduce a breaking change?

Yes
No

Other information

@alxhub Can you comment on my assumption that we can safely print WrappedNodeExpr using TypeScript's printing logic, as doing so still has a risk of producing TS specific syntax.

petebacondarwin · 2018-11-11T18:53:06Z

@alxhub Can you comment on my assumption that we can safely print WrappedNodeExpr using TypeScript's printing logic, as doing so still has a risk of producing TS specific syntax.

I am pretty confident that for ngcc, all of these wrapped nodes will have come from JS files, so it seems likely that they are already JS. Right?

petebacondarwin · 2018-11-11T18:55:59Z

Generally looks like a good idea!

petebacondarwin · 2018-11-12T16:37:42Z

How do we know what name is defined? Should we not check name for the whole function body?

petebacondarwin · 2018-11-12T16:42:29Z

Was the previous printer producing a fat arrow here or something?

Nope, it was only the non-null assertion in the template (person!.name) that tripped stuff up. It's actually surprising that the TypeScript printer has worked so well until that tiny syntactical difference.

JoostK · 2018-11-26T21:16:17Z

@petebacondarwin @alxhub After a rebase this is now green again, should be good to go. Let me know if we do want to elide the ! in ngtsc directly, if so I'll update the PR accordingly.

petebacondarwin

This fix looks good to me. It needs to have an ngtsc test too IMO. I am going to add one.
Then leave it to @alxhub to do final approval.

I realised that ngtsc outputs its AST through a JS renderer so that the non-JS AST nodes are removed, which makes a test inside ngtsc_spec.ts not possible.

petebacondarwin

LGTM! @alxhub over to you. I have rebased on master, just to be sure!

If a template contains specific TypeScript syntax, such as a non-null assertion, the code that is emitted from ngcc into a JavaScript bundle should not retain such syntax as it is invalid in JS. A full-blown TypeScript emit of a complete ts.SourceFile would be required to be able to emit JS and possibly downlevel into a lower language target, which is not an option for ngcc as it currently operates on partial ASTs, not full source files. Instead, ngtsc no longer produces TypeScript specific syntax in the first place, such that TypeScript print logic will only generate JS code.

alxhub · 2018-12-13T01:25:09Z

http://test/OCL:225285098:BASE:225285258:1544664277856:38d1de5

angular-automatic-lock-bot · 2019-09-14T10:52:37Z

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

googlebot added the cla: yes label Nov 11, 2018

JoostK force-pushed the ngcc-js-output branch from bae3c1b to 0a8cedd Compare November 11, 2018 14:29

petebacondarwin suggested changes Nov 11, 2018

View reviewed changes

Comment thread packages/compiler/src/output/abstract_js_emitter.ts Resolved

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours freq2: medium comp: ivy risk: medium labels Nov 11, 2018

ngbot Bot added this to the needsTriage milestone Nov 11, 2018

petebacondarwin modified the milestones: needsTriage, Ivy Nov 11, 2018

ngbot Bot modified the milestones: Ivy, needsTriage Nov 11, 2018

petebacondarwin added the type: bug/fix label Nov 11, 2018

ngbot Bot modified the milestones: needsTriage, Backlog Nov 11, 2018

petebacondarwin modified the milestones: Backlog, Ivy Nov 11, 2018

JoostK force-pushed the ngcc-js-output branch 2 times, most recently from 8c97aa2 to 8cc985f Compare November 11, 2018 23:44

petebacondarwin suggested changes Nov 12, 2018

View reviewed changes

petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 12, 2018

JoostK force-pushed the ngcc-js-output branch 5 times, most recently from 95abaaf to 78e3da4 Compare November 26, 2018 20:25

gkalpak mentioned this pull request Dec 7, 2018

ivy+docs-infra: compile angular.io with Ivy #26947

Closed

11 of 13 tasks

JoostK force-pushed the ngcc-js-output branch 2 times, most recently from 8b2fc25 to 518b043 Compare December 10, 2018 18:24

JoostK changed the title ~~fix(ivy): ngcc should render JavaScript instead of TypeScript~~ fix(ivy): ngcc should not emit TypeScript syntax Dec 10, 2018

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 12, 2018

petebacondarwin previously requested changes Dec 12, 2018

View reviewed changes

petebacondarwin approved these changes Dec 12, 2018

View reviewed changes

petebacondarwin force-pushed the ngcc-js-output branch from 518b043 to e02c917 Compare December 12, 2018 13:55

JoostK force-pushed the ngcc-js-output branch 2 times, most recently from 518b043 to aebacd3 Compare December 12, 2018 19:53

JoostK force-pushed the ngcc-js-output branch from aebacd3 to b087d68 Compare December 12, 2018 20:06

alxhub approved these changes Dec 13, 2018

View reviewed changes

alxhub added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 13, 2018

mhevery closed this in 023bd31 Dec 14, 2018

JoostK deleted the ngcc-js-output branch December 15, 2018 16:23

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019

Navigation Menu

Repository navigation

fix(ivy): ngcc should not emit TypeScript syntax - #27051

fix(ivy): ngcc should not emit TypeScript syntax#27051
JoostK wants to merge 1 commit into
angular:masterfrom
JoostK:ngcc-js-output

JoostK commented Nov 11, 2018

Uh oh!

petebacondarwin commented Nov 11, 2018

Uh oh!

Uh oh!

petebacondarwin commented Nov 11, 2018

Uh oh!

petebacondarwin Nov 12, 2018

Uh oh!

petebacondarwin Nov 12, 2018

Uh oh!

JoostK Nov 12, 2018

Uh oh!

JoostK commented Nov 26, 2018

Uh oh!

petebacondarwin left a comment

Uh oh!

petebacondarwin left a comment •

edited

Loading

Uh oh!

alxhub commented Dec 13, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

JoostK commented Nov 11, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

petebacondarwin commented Nov 11, 2018

Uh oh!

Uh oh!

petebacondarwin commented Nov 11, 2018

Uh oh!

petebacondarwin Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

JoostK Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

JoostK commented Nov 26, 2018

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub commented Dec 13, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

petebacondarwin left a comment •

edited

Loading