ngtsc as a plugin! #34792

alxhub · 2020-01-15T20:12:31Z

It's happening!!

This PR implements NgTscPlugin on top of some refactored compiler APIs. Hopefully this will allow integration with ts_library in the near future.

There's lots of rough edges left here, but it looks promising already!

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

alexeagle · 2020-01-18T00:41:39Z

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

+  getNextProgram(): ts.Program;
+
+  prepareEmit(): {
+    transformers: {


could you use the type ts.CustomTransformers here?

alexeagle · 2020-01-21T17:29:32Z

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

+ * enable progress to be made in parallel, the upstream interface isn't implemented directly.
+ * Instead, `TscPlugin` here is structurally assignable to what tsc_wrapped expects.
+ */
+interface TscPlugin {


could you add a member name = 'ngtsc' so that it matches the interface LitPlugin introduced for gathering diagnostics?

alexeagle · 2020-01-21T17:31:45Z

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

+ */
+interface TscPlugin {
+  wrapHost(
+      host: PluginCompilerHost, inputFiles: ReadonlyArray<string>,


why does this need to be a PluginCompilerHost? in our work last week we wrapped a ts.CompilerHost which was the only thing constructed at this point

I've refactored it to take a ts.CompilerHost plus the optional fileNameToModuleName.

alexeagle · 2020-01-21T17:41:27Z

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

+interface TscPlugin {
+  wrapHost(
+      host: PluginCompilerHost, inputFiles: ReadonlyArray<string>,
+      options: ts.CompilerOptions): ts.CompilerHost;


and I think it needs to return a PluginCompilerHost so we can locate the inputFiles when we construct a ts.Program

alexeagle · 2020-01-21T18:08:55Z

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

+    if (file === undefined) {
+      return this.diagnostics;
+    } else {
+      return this.diagnostics.filter(diag => diag.file === file);


this is broken - diag.file is the .ng.html file but file is the .ts file

JoostK

Stellar work! I like how the compiler objects are nicely separated and how the shim generator host has been centralized into the NgCompilerHost. Some minor comments for your consideration.

JoostK · 2020-01-18T23:15:22Z

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

+          compilation.traitCompiler, compilation.reflector, importRewriter,
+          compilation.defaultImportTracker, compilation.isCore, this.closureCompilerEnabled),
+      // clang-format off
+      aliasTransformFactory(compilation.traitCompiler.exportStatements) as ts.TransformerFactory<ts.SourceFile>,


Would it be possible to change the type of aliasTransformFactory to get rid of the ts.Bundle in its generic type argument?

Done. Much cleaner!

JoostK · 2020-01-18T23:16:42Z

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

+      // not
+      // being assignable to `unknown` when wrapped in `Readonly`).


Weird formatting here

JoostK · 2020-01-18T23:22:25Z

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

+ * optional).
+ */
+export type RequiredCompilerHostDelegations = {
+  [M in keyof Required<ExtendedTsCompilerHost>]: ExtendedTsCompilerHost[M]


Suggested change

[M in keyof Required<ExtendedTsCompilerHost>]: ExtendedTsCompilerHost[M]

[M in keyof Required<ExtendedTsCompilerHost>]: ExtendedTsCompilerHost[M];

JoostK · 2020-01-18T23:30:24Z

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

+
+  get compiler(): NgCompiler {
+    if (this._compiler === null) {
+      throw new Error('Lifecycle error: getDiagnostics() called before setupCompilation()');


This message looks inaccurate.

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

JoostK

LGTM, with some more minor comments

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

JoostK · 2020-01-21T22:43:27Z

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

+      return this.diagnostics.filter(diag => {
+        if (diag.file === file) {
+          return true;
+        } else if (isTemplateDiagnostic(diag) && diag.componentFile === file) {


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

I agree, but I'm gonna defer it because we don't have any testing infrastructure that invokes the compiler at this level yet.

alxhub · 2020-01-21T22:56:43Z

This is blocked on the upstream g3 work to standardize the plugin API, as well as #34887 landing first.

alexeagle · 2020-01-31T18:04:01Z

Should be unblocked for g3sync now - I commented out the relevant bit in tsc_wrapped

This commit moves the calculation of `ignoreFiles` - the set of files to be ignored by a consumer of the `NgCompiler` API - from its `prepareEmit` operation to its initialization. It's now available as a field on `NgCompiler`. This will allow a consumer to skip gathering diagnostics for `ignoreFiles` as well as skip emit.

This commit implements an experimental integration with tsc_wrapped, where it can load the Angular compiler as a plugin and perform Angular transpilation at a user's request. This is an alternative to the current ngc_wrapped mechanism, which is a fork of tsc_wrapped from several years ago. tsc_wrapped has improved significantly since then, and this feature will allow Angular to benefit from those improvements. Currently the plugin API between tsc_wrapped and the Angular compiler is a work in progress, so NgTscPlugin does not yet implement any interfaces from @bazel/typescript (the home of tsc_wrapped). Instead, an interface is defined locally to guide this standardization.

alxhub · 2020-02-06T00:25:34Z

Presubmit

alxhub · 2020-02-06T01:29:49Z

Caretaker: I cannot force g3 status ("This SHA and context has reached the maximum number of statuses" error). Presubmits are good (only Ivy failures are screenshot diffs & timeouts). This is ready to merge.

I've marked it as master-only - I'll open a separate PR with only the refactor commit for the 9.0.x branch.

#34792) This commit implements an experimental integration with tsc_wrapped, where it can load the Angular compiler as a plugin and perform Angular transpilation at a user's request. This is an alternative to the current ngc_wrapped mechanism, which is a fork of tsc_wrapped from several years ago. tsc_wrapped has improved significantly since then, and this feature will allow Angular to benefit from those improvements. Currently the plugin API between tsc_wrapped and the Angular compiler is a work in progress, so NgTscPlugin does not yet implement any interfaces from @bazel/typescript (the home of tsc_wrapped). Instead, an interface is defined locally to guide this standardization. PR Close #34792

…ngular#34792) This commit moves the calculation of `ignoreFiles` - the set of files to be ignored by a consumer of the `NgCompiler` API - from its `prepareEmit` operation to its initialization. It's now available as a field on `NgCompiler`. This will allow a consumer to skip gathering diagnostics for `ignoreFiles` as well as skip emit. PR Close angular#34792

…34792) (#35346) This commit moves the calculation of `ignoreFiles` - the set of files to be ignored by a consumer of the `NgCompiler` API - from its `prepareEmit` operation to its initialization. It's now available as a field on `NgCompiler`. This will allow a consumer to skip gathering diagnostics for `ignoreFiles` as well as skip emit. PR Close #34792 PR Close #35346

…ngular#34792) This commit moves the calculation of `ignoreFiles` - the set of files to be ignored by a consumer of the `NgCompiler` API - from its `prepareEmit` operation to its initialization. It's now available as a field on `NgCompiler`. This will allow a consumer to skip gathering diagnostics for `ignoreFiles` as well as skip emit. PR Close angular#34792

angular#34792) This commit implements an experimental integration with tsc_wrapped, where it can load the Angular compiler as a plugin and perform Angular transpilation at a user's request. This is an alternative to the current ngc_wrapped mechanism, which is a fork of tsc_wrapped from several years ago. tsc_wrapped has improved significantly since then, and this feature will allow Angular to benefit from those improvements. Currently the plugin API between tsc_wrapped and the Angular compiler is a work in progress, so NgTscPlugin does not yet implement any interfaces from @bazel/typescript (the home of tsc_wrapped). Instead, an interface is defined locally to guide this standardization. PR Close angular#34792

LayZeeDK · 2020-02-13T23:43:24Z

So this is something Bazel-related? Would anyone care to explain what the big deal is? 🤔

…ngular#34792) This commit moves the calculation of `ignoreFiles` - the set of files to be ignored by a consumer of the `NgCompiler` API - from its `prepareEmit` operation to its initialization. It's now available as a field on `NgCompiler`. This will allow a consumer to skip gathering diagnostics for `ignoreFiles` as well as skip emit. PR Close angular#34792

angular#34792) This commit implements an experimental integration with tsc_wrapped, where it can load the Angular compiler as a plugin and perform Angular transpilation at a user's request. This is an alternative to the current ngc_wrapped mechanism, which is a fork of tsc_wrapped from several years ago. tsc_wrapped has improved significantly since then, and this feature will allow Angular to benefit from those improvements. Currently the plugin API between tsc_wrapped and the Angular compiler is a work in progress, so NgTscPlugin does not yet implement any interfaces from @bazel/typescript (the home of tsc_wrapped). Instead, an interface is defined locally to guide this standardization. PR Close angular#34792

angular-automatic-lock-bot · 2020-03-15T16:09:19Z

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 Jan 15, 2020

alxhub force-pushed the ngtsc/plugin branch 2 times, most recently from 1e39e60 to a51dfe9 Compare January 18, 2020 00:23

alxhub added target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 18, 2020

alxhub requested review from JoostK and alexeagle and removed request for JoostK January 18, 2020 00:23

alxhub force-pushed the ngtsc/plugin branch from a51dfe9 to c1cb0ae Compare January 18, 2020 00:31

alxhub marked this pull request as ready for review January 18, 2020 00:31

alxhub requested review from a team as code owners January 18, 2020 00:31

alexeagle reviewed Jan 18, 2020

View reviewed changes

packages/compiler-cli/src/ngtsc/tsc_plugin.ts Show resolved Hide resolved

alexeagle reviewed Jan 18, 2020

View reviewed changes

alexeagle reviewed Jan 21, 2020

View reviewed changes

JoostK reviewed Jan 21, 2020

View reviewed changes

alxhub force-pushed the ngtsc/plugin branch 5 times, most recently from e2ccefe to 8f45d7c Compare January 21, 2020 22:40

JoostK approved these changes Jan 21, 2020

View reviewed changes

alxhub force-pushed the ngtsc/plugin branch from 8f45d7c to 6b9d24f Compare January 21, 2020 22:52

alxhub added the state: blocked label Jan 21, 2020

matsko added the comp: ivy label Jan 22, 2020

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

alxhub force-pushed the ngtsc/plugin branch from 8f45d7c to 804dbda Compare January 24, 2020 18:52

alexeagle removed the state: blocked label Jan 31, 2020

alxhub force-pushed the ngtsc/plugin branch from 804dbda to 890bc9f Compare February 4, 2020 17:51

alxhub added 2 commits February 4, 2020 10:21

alxhub force-pushed the ngtsc/plugin branch from 890bc9f to 0b9d5a4 Compare February 4, 2020 18:26

mhevery closed this in 14aa6d0 Feb 6, 2020

alxhub mentioned this pull request Feb 11, 2020

refactor(ivy): compute ignoreFiles for compilation on initialization … #35346

Closed

AndrewKushnir mentioned this pull request Mar 3, 2020

build: adding a script to compare commits in master and stable branches #35130

Closed

3 tasks

alan-agius4 mentioned this pull request Mar 5, 2020

Module '"../../../../@bazel/typescript/internal/tsc_wrapped/plugin_api"' has no exported member 'TscPlugin' #35855

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ngtsc as a plugin! #34792

ngtsc as a plugin! #34792

alxhub commented Jan 15, 2020 •

edited

alexeagle Jan 18, 2020

alxhub Jan 21, 2020

alexeagle Jan 21, 2020

alxhub Jan 21, 2020

alexeagle Jan 21, 2020

alxhub Jan 21, 2020

alexeagle Jan 21, 2020

alxhub Jan 21, 2020

alexeagle Jan 21, 2020 •

edited

alxhub Jan 21, 2020

JoostK left a comment

JoostK Jan 18, 2020

alxhub Jan 21, 2020

JoostK Jan 18, 2020

alxhub Jan 21, 2020

JoostK Jan 18, 2020

alxhub Jan 21, 2020

JoostK Jan 18, 2020

alxhub Jan 21, 2020

JoostK left a comment

JoostK Jan 21, 2020

alxhub Feb 4, 2020

alxhub commented Jan 21, 2020

alexeagle commented Jan 31, 2020

alxhub commented Feb 6, 2020

alxhub commented Feb 6, 2020 •

edited

LayZeeDK commented Feb 13, 2020

angular-automatic-lock-bot bot commented Mar 15, 2020

		// not
		// being assignable to `unknown` when wrapped in `Readonly`).

	[M in keyof Required<ExtendedTsCompilerHost>]: ExtendedTsCompilerHost[M]
	[M in keyof Required<ExtendedTsCompilerHost>]: ExtendedTsCompilerHost[M];

ngtsc as a plugin! #34792

ngtsc as a plugin! #34792

Conversation

alxhub commented Jan 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeagle Jan 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxhub commented Jan 21, 2020

alexeagle commented Jan 31, 2020

alxhub commented Feb 6, 2020

alxhub commented Feb 6, 2020 • edited

LayZeeDK commented Feb 13, 2020

angular-automatic-lock-bot bot commented Mar 15, 2020

alxhub commented Jan 15, 2020 •

edited

alexeagle Jan 21, 2020 •

edited

alxhub commented Feb 6, 2020 •

edited