refactor(ivy): introduce the 'core' package and split apart NgtscProgram by alxhub · Pull Request #34887 · angular/angular

alxhub · 2020-01-21T22:55:41Z

Previously, NgtscProgram lived in the main @angular/compiler-cli package
alongside the legacy View Engine compiler. As a result, the main package
depended on all of the ngtsc internal packages, and a significant portion of
ngtsc logic lived in NgtscProgram.

This commit refactors NgtscProgram and moves the main logic of compilation
into a new 'core' package. The new package defines a new API which enables
implementers of TypeScript compilers (compilers built using the TS API) to
support Angular transpilation as well. It involves a new NgCompiler type
which takes a ts.Program and performs Angular analysis and transformations,
as well as an NgCompilerHost which wraps an input ts.CompilerHost and adds
any extra Angular files.

Together, these two classes are used to implement a new NgtscProgram which
adapts the legacy api.Program interface used by the View Engine compiler
onto operations on the new types. The new NgtscProgram implementation is
significantly smaller and easier to reason about.

The new NgCompilerHost replaces the previous GeneratedShimsHostWrapper which
lived in the 'shims' package.

A new 'resource' package is added to support the HostResourceLoader which
previously lived in the outer compiler package.

As a result of the refactoring, the dependencies of the outer
@angular/compiler-cli package on ngtsc internal packages are significantly
trimmed.

This refactoring was driven by the desire to build a plugin interface to the
compiler so that tsc_wrapped (another consumer of the TS compiler APIs) can
perform Angular transpilation on user request.

JoostK

LGTM, with some more minor comments

packages/compiler-cli/src/ngtsc/core/api.ts

JoostK · 2020-01-21T23:18:00Z

packages/compiler-cli/src/ngtsc/core/src/compiler.ts

This would be worth having in a unit test, I don't think we have such a test at the moment.

Agreed. This was a little tricky to do at this level of abstraction - I think we could benefit from some testing utilities here eventually. Still, the test looks pretty reasonable.

petebacondarwin

Nice cleanup! Lots of minor NITS. Nothing to get too worried about.

packages/compiler-cli/src/ngtsc/core/README.md

petebacondarwin · 2020-01-22T08:10:00Z

packages/compiler-cli/src/ngtsc/core/README.md

This is a long line. Do we not worry about line length in READMEs?

Line breaks in Markdown are semantic, so long lines are actually what we want here. Otherwise we're breaking the formatting.

In github they're not: https://github.github.com/gfm/#paragraphs

You need an empty blank line to indicate an end of paragraph.

petebacondarwin · 2020-01-22T08:13:13Z

packages/compiler-cli/src/ngtsc/core/README.md

I thought Webpack was synchronous with regard to Angular compilation?

Nope! The actual TS side is (particularly the module resolution hooks that ngcc cares about). But the Angular analysis is async, because producing styleUrls or templateUrl contents might require running through another Webpack invocation, which I believe is accomplished by forking the webpack-ing process?

petebacondarwin · 2020-01-22T08:18:03Z

packages/compiler-cli/src/ngtsc/core/api.ts

Perhaps complete the example by describing what the return value might be?

I believe path/to/containingFile.ts is supposed to be the example, but I've clarified it to be a lot more clear.

petebacondarwin · 2020-01-22T08:25:30Z

packages/compiler-cli/src/ngtsc/core/api.ts

By host do we really mean CompilerHost? Or can this interface be used in other contexts?

It can be used in other contexts :) actually, the compiler has an internal interface named FileToModuleHost which is identical to this one. I've modified the PR to unify them (I like the name UnifiedModulesHost anyway as it's a lot more clear about what the intent of the interface is - a unified view of the module namespace.

packages/compiler-cli/src/ngtsc/core/src/host.ts

petebacondarwin · 2020-01-22T09:12:05Z

packages/compiler-cli/src/ngtsc/core/src/host.ts

We could use absoluteFrom rather than resolve here, given that TS internal paths are guaranteed to be absolute.

petebacondarwin · 2020-01-22T09:38:02Z

packages/compiler-cli/src/ngtsc/program.ts

Is there a tsickle issue to track for this?

petebacondarwin · 2020-01-22T09:46:32Z

packages/compiler-cli/src/ngtsc/core/src/compiler.ts

What casts?

Joost had me refactor them away. Comment deleted!

petebacondarwin · 2020-01-22T09:51:39Z

packages/compiler-cli/src/ngtsc/core/src/compiler.ts

Probably not part of this PR but it seems strange for this to be in the compiler rather than inside a trait? Am I missing something?

Trait is an operation with a pretty specific shape - match a decorator or some other aspect of the class, extract information from it in local & global steps, and then add some static fields to the class (mimicking the execution of said decorator at runtime).

Other forms of static analysis & mutation, like detecting static methods or functions which return ModuleWithProviders, don't really fit this model. We could create a similar kind of abstraction (maybe upstream ngcc's Migration concept?).

OK - may be for another PR :-)

Previously, NgtscProgram lived in the main @angular/compiler-cli package alongside the legacy View Engine compiler. As a result, the main package depended on all of the ngtsc internal packages, and a significant portion of ngtsc logic lived in NgtscProgram. This commit refactors NgtscProgram and moves the main logic of compilation into a new 'core' package. The new package defines a new API which enables implementers of TypeScript compilers (compilers built using the TS API) to support Angular transpilation as well. It involves a new NgCompiler type which takes a ts.Program and performs Angular analysis and transformations, as well as an NgCompilerHost which wraps an input ts.CompilerHost and adds any extra Angular files. Together, these two classes are used to implement a new NgtscProgram which adapts the legacy api.Program interface used by the View Engine compiler onto operations on the new types. The new NgtscProgram implementation is significantly smaller and easier to reason about. The new NgCompilerHost replaces the previous GeneratedShimsHostWrapper which lived in the 'shims' package. A new 'resource' package is added to support the HostResourceLoader which previously lived in the outer compiler package. As a result of the refactoring, the dependencies of the outer @angular/compiler-cli package on ngtsc internal packages are significantly trimmed. This refactoring was driven by the desire to build a plugin interface to the compiler so that tsc_wrapped (another consumer of the TS compiler APIs) can perform Angular transpilation on user request.

…ram (#34887) Previously, NgtscProgram lived in the main @angular/compiler-cli package alongside the legacy View Engine compiler. As a result, the main package depended on all of the ngtsc internal packages, and a significant portion of ngtsc logic lived in NgtscProgram. This commit refactors NgtscProgram and moves the main logic of compilation into a new 'core' package. The new package defines a new API which enables implementers of TypeScript compilers (compilers built using the TS API) to support Angular transpilation as well. It involves a new NgCompiler type which takes a ts.Program and performs Angular analysis and transformations, as well as an NgCompilerHost which wraps an input ts.CompilerHost and adds any extra Angular files. Together, these two classes are used to implement a new NgtscProgram which adapts the legacy api.Program interface used by the View Engine compiler onto operations on the new types. The new NgtscProgram implementation is significantly smaller and easier to reason about. The new NgCompilerHost replaces the previous GeneratedShimsHostWrapper which lived in the 'shims' package. A new 'resource' package is added to support the HostResourceLoader which previously lived in the outer compiler package. As a result of the refactoring, the dependencies of the outer @angular/compiler-cli package on ngtsc internal packages are significantly trimmed. This refactoring was driven by the desire to build a plugin interface to the compiler so that tsc_wrapped (another consumer of the TS compiler APIs) can perform Angular transpilation on user request. PR Close #34887

angular-automatic-lock-bot · 2020-02-24T16:08:15Z

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

alxhub requested review from a team January 21, 2020 22:55

googlebot added the cla: yes label Jan 21, 2020

alxhub mentioned this pull request Jan 21, 2020

ngtsc as a plugin! #34792

Closed

alxhub added the target: patch This PR is targeted for the next patch release label Jan 21, 2020

alxhub requested a review from JoostK January 21, 2020 23:11

JoostK approved these changes Jan 21, 2020

View reviewed changes

petebacondarwin approved these changes Jan 22, 2020

View reviewed changes

matsko added the comp: ivy label Jan 22, 2020

ngbot bot added this to the needsTriage milestone Jan 22, 2020

alxhub added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Jan 23, 2020

alxhub force-pushed the ngtsc/core-refactor branch 2 times, most recently from dc8100b to 7af2ce9 Compare January 23, 2020 20:11

alxhub force-pushed the ngtsc/core-refactor branch from 7af2ce9 to dd02363 Compare January 23, 2020 21:17

alxhub added the action: merge The PR is ready for merge by the caretaker label Jan 24, 2020

AndrewKushnir closed this in 24b2f1d Jan 24, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 24, 2020

Conversation

alxhub commented Jan 21, 2020

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants