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

feat(compiler-cli): make enableIvy ngtsc/true equivalent #28616

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@filipesilva
Copy link
Member

commented Feb 8, 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?

Currently setting enableIvy to true runs a hybrid mode of ngc and ngtsc. This is counterintuitive given the name of the flag itself.

What is the new behavior?

This PR makes the true value equivalent to the previous ngtsc, and ngtsc becomes an alias for true. Effectively this removes the hybrid mode as well since there's no other way to enable it.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I'm not sure if the true option is currently used anywhere.

@alxhub can you advise on:

  • ways to test this change
  • if there should be a migration path documented
  • if a new value should be added to enableIvy to keep the existing hybrid behaviour (the current true value)

@filipesilva filipesilva requested a review from alxhub Feb 8, 2019

@filipesilva filipesilva requested a review from angular/fw-compiler as a code owner Feb 8, 2019

@googlebot googlebot added the cla: yes label Feb 8, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Feb 8, 2019

@filipesilva filipesilva referenced this pull request Feb 8, 2019

Closed

docs: add ivy opt-in docs #28569

3 of 14 tasks complete

filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Feb 8, 2019

@JoostK

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Related: #27306

@mhevery mhevery added the comp: ivy label Feb 8, 2019

@ngbot ngbot bot added this to the needsTriage milestone Feb 8, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

ways to test this change

Switch ng_module.bzl to use the true value instead of "ngtsc".

  • if there should be a migration path documented

Nah, nothing depends on the old behavior afaik.

  • if a new value should be added to enableIvy to keep the existing hybrid behaviour (the current true value)

Nope, we'll remove it soon.

@filipesilva filipesilva force-pushed the filipesilva:enableivy-true branch from 2273de1 to e646af3 Feb 11, 2019

@filipesilva filipesilva requested a review from angular/fw-dev-infra as a code owner Feb 11, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Feb 11, 2019

@filipesilva filipesilva force-pushed the filipesilva:enableivy-true branch from e646af3 to c66eeb3 Feb 12, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Feb 12, 2019

@filipesilva filipesilva force-pushed the filipesilva:enableivy-true branch from c66eeb3 to e266ce3 Feb 12, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Feb 12, 2019

@@ -120,6 +120,10 @@ export function calcProjectFileAndBasePath(project: string):

export function createNgCompilerOptions(
basePath: string, config: any, tsOptions: ts.CompilerOptions): api.CompilerOptions {
// enableIvy `true` is an alias for `ngtsc`.
if (config.angularCompilerOptions && config.angularCompilerOptions.enableIvy === true) {

This comment has been minimized.

Copy link
@alxhub

alxhub Feb 12, 2019

Contributor

I'd rather go the other way. ngtsc should be an alias for true.

This comment has been minimized.

Copy link
@filipesilva

filipesilva Feb 13, 2019

Author Member

Done.

@filipesilva filipesilva force-pushed the filipesilva:enableivy-true branch from e266ce3 to 51fa0cc Feb 12, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Feb 12, 2019

@filipesilva filipesilva force-pushed the filipesilva:enableivy-true branch 2 times, most recently from 9b0e0f4 to ff92ef7 Feb 13, 2019

@filipesilva filipesilva changed the title feat(compiler-cli): enableIvy true value is an alias for ngtsc feat(compiler-cli): make enableIvy ngtsc/true equivalent Feb 13, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Feb 13, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Feb 13, 2019

feat(compiler-cli): make enableIvy ngtsc/true equivalent
Currently setting `enableIvy` to true runs a hybrid mode of `ngc` and `ngtsc`. This is counterintuitive given the name of the flag itself.

This PR makes the `true` value equivalent to the previous `ngtsc`, and `ngtsc` becomes an alias for `true`. Effectively this removes the hybrid mode as well since there's no other way to enable it.

@filipesilva filipesilva force-pushed the filipesilva:enableivy-true branch from ff92ef7 to 86158af Feb 13, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Feb 13, 2019

@alxhub

alxhub approved these changes Feb 15, 2019

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

@IgorMinar IgorMinar closed this in 1923c2f Feb 19, 2019

filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Feb 20, 2019

@filipesilva filipesilva deleted the filipesilva:enableivy-true branch Feb 22, 2019

filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Feb 22, 2019

mgechev added a commit to angular/angular-cli that referenced this pull request Feb 22, 2019

Kiku-git added a commit to Kiku-git/angular-cli that referenced this pull request Mar 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.