Skip to content

fix(compiler-cli): NgTscPlugin should clean up after program creation - #37599

Closed
alxhub wants to merge 1 commit into
angular:masterfrom
alxhub:plugin/fix-cleanup
Closed

fix(compiler-cli): NgTscPlugin should clean up after program creation#37599
alxhub wants to merge 1 commit into
angular:masterfrom
alxhub:plugin/fix-cleanup

Conversation

@alxhub

@alxhub alxhub commented Jun 15, 2020

Copy link
Copy Markdown
Member

NgTscPlugin wraps an incoming ts.CompilerHost in an NgCompilerHost
which patches all ts.SourceFiles in the program with referenced shims. In
order to not emit triple-slash <reference> directives that refer to shim
files, these patches have to be cleaned up by calling
NgCompilerHost.postProgramCreationCleanup() before emit. Previously, the
NgTscPlugin was not doing this cleanup.

This commit fixes the bug (which is a one-line change), plus adds a
mechanism in the NgtscTestEnvironment to run compilation via the plugin,
allowing for a test to be written.

@alxhub
alxhub requested a review from gregmagolan June 15, 2020 23:54
@alxhub alxhub added the target: patch This PR is targeted for the next patch release label Jun 15, 2020
alexeagle
alexeagle approved these changes Jun 16, 2020
View reviewed changes

@alexeagle alexeagle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks a bunch for fitting this into the v10 release! It's nice to see the fix can be made without anything Bazel-specific like changing rules_typescript.

gregmagolan
gregmagolan approved these changes Jun 16, 2020
View reviewed changes

@gregmagolan gregmagolan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Thanks @alxhub

@alexeagle

alexeagle commented Jun 16, 2020

Copy link
Copy Markdown
Contributor

FYI I patched that line into rules_nodejs to confirm that it's green downstream bazel-contrib/rules_nodejs#1932

JoostK
JoostK approved these changes Jun 16, 2020
View reviewed changes

@JoostK JoostK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with comments

Comment thread packages/compiler-cli/test/ngtsc/plugin_spec.ts Resolved
Comment thread packages/compiler-cli/test/ngtsc/env.ts Resolved
@JoostK JoostK 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 Jun 16, 2020
@alxhub
fix(compiler-cli): NgTscPlugin should clean up after program creation
9128f92 
`NgTscPlugin` wraps an incoming `ts.CompilerHost` in an `NgCompilerHost`
which patches all `ts.SourceFile`s in the program with referenced shims. In
order to not emit triple-slash `<reference>` directives that refer to shim
files, these patches have to be cleaned up by calling
`NgCompilerHost.postProgramCreationCleanup()` before emit. Previously, the
`NgTscPlugin` was not doing this cleanup.

This commit fixes the bug (which is a one-line change), plus adds a
mechanism in the `NgtscTestEnvironment` to run compilation via the plugin,
allowing for a test to be written.
@alxhub
alxhub force-pushed the plugin/fix-cleanup branch from 27528e3 to 9128f92 Compare June 16, 2020 19:16
@alxhub

alxhub commented Jun 16, 2020

Copy link
Copy Markdown
Member Author

Presubmit

@mhevery mhevery added the area: compiler Issues related to `ngc`, Angular's template compiler label Jun 22, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Jun 22, 2020
@alexeagle

alexeagle commented Jun 30, 2020

Copy link
Copy Markdown
Contributor

Agh, this wasn't included in Angular 10? So anyone using Bazel will have problems upgrading. I wonder if any such issues were already reported

@alexeagle

alexeagle commented Jun 30, 2020

Copy link
Copy Markdown
Contributor

oh it looks like maybe the fix went in #37690 already so this PR just adds a specific test case for the regression we saw in ts_library

@JoostK

JoostK commented Jun 30, 2020

Copy link
Copy Markdown
Member

oh it looks like maybe the fix went in #37690 already so this PR just adds a specific test case for the regression we saw in ts_library

@alexeagle Yes, that's correct. 10.0.0 was still broken, but 10.0.1 fixed the issue in #37690.

@alexeagle

alexeagle commented Jun 30, 2020

Copy link
Copy Markdown
Contributor

Cool we upgraded the Bazel example and removed the patch, all good on our end.

@alxhub alxhub closed this Feb 19, 2021
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Mar 22, 2021

Copy link
Copy Markdown

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 Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoostK JoostK JoostK approved these changes
+2 more reviewers
@alexeagle alexeagle alexeagle approved these changes
@gregmagolan gregmagolan gregmagolan approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

6 participants

@alxhub @alexeagle @JoostK @gregmagolan @mhevery @googlebot