Cleanup Ngc Api as preparation for watch mode by tbosch · Pull Request #18633 · angular/angular

tbosch · 2017-08-10T15:15:39Z

Note: The first commit is from #18494

googlebot · 2017-08-10T15:15:47Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

mary-poppins · 2017-08-10T15:28:42Z

You can preview 4d8d0b5 at https://pr18633-4d8d0b5.ngbuilds.io/.

chuckjaz · 2017-08-10T17:07:34Z

Since this doesn't merge the angular options and set genDir and basePath like readConfiguration does, this causes:

this.basePath = path.normalize(path.join(options.basePath !, '.')).replace(/\\/g, '/');

to fail in module_filename_resolver.ts. This code should compensate for the missing work done in readConfiguration or modify readConfiguration to allow the work to be shared.

mary-poppins · 2017-08-10T22:20:35Z

You can preview fdaa7b5 at https://pr18633-fdaa7b5.ngbuilds.io/.

mary-poppins · 2017-08-11T00:31:39Z

You can preview 6b012a9 at https://pr18633-6b012a9.ngbuilds.io/.

googlebot · 2017-08-11T14:29:31Z

CLAs look good, thanks!

mary-poppins · 2017-08-11T14:39:40Z

You can preview e3d126a at https://pr18633-e3d126a.ngbuilds.io/.

alexeagle · 2017-08-11T15:52:32Z

we should use TS to parse the command line, so that it's possible to pass options that override or replace the tsconfig.json file
like tsc-wrapped does: https://github.com/angular/angular/blob/master/packages/tsc-wrapped/src/main.ts#L199

Added a TODO

alexeagle · 2017-08-11T15:53:22Z

are we able to remove this and move cli args fully into the tsconfig file yet?

Yes, you can specify all i18n options in the config now. However we keep the command line options for backwards compatibility...

alexeagle · 2017-08-11T15:54:19Z

we should not call process.exit, says @evmar - instead set process.exitCode and let the process terminate

alexeagle · 2017-08-11T15:56:00Z

it's not necessary to copy from TypeScript; I moved this to a public API ts.formatDiagnostics
like here:

angular/packages/compiler-cli/src/perform-compile.ts

Line 28 in 679608d

if (isTsDiagnostics(diags)) {

I want to apply it to a single diagnostic, so that the diagnostics array can contain both, ts.Diagnostics and ng.Diagnostics. However, I think I can use that method for a single diagnostic as well.

alexeagle · 2017-08-11T15:57:13Z

note, and FYI: this is duplicating code here
https://github.com/bazelbuild/rules_typescript/blob/master/internal/tsc_wrapped/tsconfig.ts#L99
but that's probably okay as it's not much, and we don't have a plan for sharing bazel libraries on npm yet

alexeagle · 2017-08-11T15:58:11Z

passing basePath is a smell, it's not actually a free variable, it is required to be the absolute path to the directory containing tsconfig.json or else paths produced by TS moduleResolver won't match up

Mmh, so how would I get it then if it was no passed in?

alexeagle · 2017-08-11T15:59:19Z

nit: google TS style guide says to use Number(major) to coerce the type

alexeagle · 2017-08-11T16:00:17Z

spelling: complier -> compiler

This is in preparation for watch mode.

mary-poppins · 2017-08-11T17:55:07Z

You can preview 3c04703 at https://pr18633-3c04703.ngbuilds.io/.

tbosch · 2017-08-11T18:54:47Z

Travis was red because of a flake, rerunning now.

angular-automatic-lock-bot · 2019-09-12T06:22: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: no label Aug 10, 2017

tbosch requested a review from chuckjaz August 10, 2017 15:16

chuckjaz suggested changes Aug 10, 2017

View reviewed changes

tbosch force-pushed the ngc_api branch from 4d8d0b5 to fdaa7b5 Compare August 10, 2017 22:10

chuckjaz approved these changes Aug 10, 2017

View reviewed changes

tbosch force-pushed the ngc_api branch from fdaa7b5 to 6b012a9 Compare August 11, 2017 00:20

tbosch added target: major This PR is targeted for the next major release area: core Issues related to the framework runtime refactoring Issue that involves refactoring or code-cleanup labels Aug 11, 2017

tbosch force-pushed the ngc_api branch from 6b012a9 to e3d126a Compare August 11, 2017 14:29

googlebot added cla: yes and removed cla: no labels Aug 11, 2017

alexeagle approved these changes Aug 11, 2017

View reviewed changes

refactor(compiler-cli): cleanup API for transformer based ngc

3c04703

This is in preparation for watch mode.

tbosch force-pushed the ngc_api branch from e3d126a to 3c04703 Compare August 11, 2017 17:43

tbosch added the action: merge The PR is ready for merge by the caretaker label Aug 11, 2017

hansl merged commit 27d901a into angular:master Aug 11, 2017

tbosch deleted the ngc_api branch August 14, 2017 16:50

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

Conversation

tbosch commented Aug 10, 2017

Uh oh!

googlebot commented Aug 10, 2017

Uh oh!

mary-poppins commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Aug 10, 2017

Uh oh!

mary-poppins commented Aug 11, 2017

Uh oh!

googlebot commented Aug 11, 2017

Uh oh!

mary-poppins commented Aug 11, 2017

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!

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!

mary-poppins commented Aug 11, 2017

Uh oh!

tbosch commented Aug 11, 2017

Uh oh!

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

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants