Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): remove ngcc postinstall migration #33727

Closed

Conversation

@filipesilva
Copy link
Member

filipesilva commented Nov 11, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

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?

angular/angular-cli#16017

What is the new behavior?

No ngcc added automatically to postinstall

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Partially address angular/angular-cli#16017

@filipesilva filipesilva requested a review from jbogarthyde Nov 11, 2019
@filipesilva filipesilva requested review from angular/fw-core as code owners Nov 11, 2019
@googlebot googlebot added the cla: yes label Nov 11, 2019
@filipesilva filipesilva requested review from mgechev and IgorMinar Nov 11, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 11, 2019

Copy link
Contributor

jbogarthyde left a comment

Organizational suggestions

@@ -38,7 +38,8 @@ In the `angular.json` workspace configuration file, set the default build option

Ivy applications can be built with libraries that were created with the View Engine compiler.
This compatibility is provided by a tool known as the Angular compatibility compiler (`ngcc`).
CLI commands run `ngcc` as needed, either after npm installation of dependencies or when performing an Angular build.
CLI commands run `ngcc` as needed performing an Angular build.
You can also run `ngcc` after npm installation for faster compilation as described in [Speeding up ngcc compilation](#speeding-up-ngcc-compilation).

This comment has been minimized.

Copy link
@jbogarthyde

jbogarthyde Nov 11, 2019

Contributor

The new section should not be separated from this, it's part of the same topic. How about making it a subsection?
Change this to something like:

Speed up compilation for large projects

To speed up compilation for a large project with many dependent libraries, you can use a post-install script to run ngcc in parallel.

move section content here

@@ -38,7 +38,8 @@ In the `angular.json` workspace configuration file, set the default build option

Ivy applications can be built with libraries that were created with the View Engine compiler.
This compatibility is provided by a tool known as the Angular compatibility compiler (`ngcc`).
CLI commands run `ngcc` as needed, either after npm installation of dependencies or when performing an Angular build.
CLI commands run `ngcc` as needed performing an Angular build.
You can also run `ngcc` after npm installation for faster compilation as described in [Speeding up ngcc compilation](#speeding-up-ngcc-compilation).

This comment has been minimized.

Copy link
@jbogarthyde

jbogarthyde Nov 11, 2019

Contributor

Make this a subsection as well :

Maintaining library compatibility

@kara kara added the comp: core label Nov 11, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 11, 2019
@filipesilva

This comment has been minimized.

Copy link
Member Author

filipesilva commented Nov 12, 2019

@jbogarthyde added your suggestions, and also added an alert box with a caveat to this approach, PTAL.

@filipesilva filipesilva force-pushed the filipesilva:remove-ngcc-postinstall branch from 5bcb9df to c68c5eb Nov 12, 2019
@filipesilva filipesilva requested a review from angular/fw-docs-packaging as a code owner Nov 12, 2019
@filipesilva filipesilva force-pushed the filipesilva:remove-ngcc-postinstall branch from c68c5eb to 58cbf44 Nov 12, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 12, 2019

@filipesilva filipesilva force-pushed the filipesilva:remove-ngcc-postinstall branch from 58cbf44 to 03346cb Nov 12, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 12, 2019

Copy link
Contributor

jbogarthyde left a comment

Please change heading levels - looks good otherwise.

CLI commands run `ngcc` as needed performing an Angular build.

{@a speeding-up-ngcc-compilation}
## Speeding up ngcc compilation

This comment has been minimized.

Copy link
@jbogarthyde

jbogarthyde Nov 12, 2019

Contributor

Please make these subsections (h3)

</div>

{@a maintaining-library-compatibility}
## Maintaining library compatibility

This comment has been minimized.

Copy link
@jbogarthyde

jbogarthyde Nov 12, 2019

Contributor

subsection (h3)

filipesilva added 2 commits Nov 11, 2019
@filipesilva filipesilva force-pushed the filipesilva:remove-ngcc-postinstall branch from 03346cb to d88be56 Nov 12, 2019
@filipesilva

This comment has been minimized.

Copy link
Member Author

filipesilva commented Nov 12, 2019

Changed the headers, thanks for the review @jbogarthyde!

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 12, 2019

Copy link
Member

IgorMinar left a comment

I thought that we were going to make this migration optional (you'd have to invoke it via an explicit command) but I think the change to package.json is trivial enough that documenting it is sufficient.

thanks!

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Nov 12, 2019

merge-assistance: no g3 impact - we don't run this migration in g3

@kara kara closed this in 1389d17 Nov 12, 2019
kara added a commit that referenced this pull request Nov 12, 2019
kara added a commit that referenced this pull request Nov 12, 2019
PR Close #33727
kara added a commit that referenced this pull request Nov 12, 2019
kara added a commit that referenced this pull request Nov 12, 2019
kara added a commit that referenced this pull request Nov 12, 2019
PR Close #33727
@filipesilva filipesilva deleted the filipesilva:remove-ngcc-postinstall branch Nov 13, 2019
timdeschryver added a commit to ngrx/platform that referenced this pull request Nov 16, 2019
It isn't part of the migration see angular/angular#33727 and angular/angular-cli#16145
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.