Skip to content

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

Closed
filipesilva wants to merge 1 commit into
angular:masterfrom
filipesilva:enableivy-true
Closed

feat(compiler-cli): make enableIvy ngtsc/true equivalent#28616
filipesilva wants to merge 1 commit into
angular:masterfrom
filipesilva:enableivy-true

Conversation

@filipesilva

@filipesilva filipesilva commented Feb 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 review from a team and alxhub February 8, 2019 11:50
@mary-poppins

mary-poppins commented Feb 8, 2019

Copy link
Copy Markdown

You can preview 2273de1 at https://pr28616-2273de1.ngbuilds.io/.

@filipesilva filipesilva mentioned this pull request Feb 8, 2019
Closed
3 of 14 tasks
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Feb 8, 2019
@filipesilva
fix(@schematics/angular): use enableIvy as true
1e937ca 
Blocked on angular/angular#28616
@filipesilva filipesilva mentioned this pull request Feb 8, 2019
Merged
@JoostK

JoostK commented Feb 8, 2019

Copy link
Copy Markdown
Member

Related: #27306

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

alxhub commented Feb 11, 2019

Copy link
Copy Markdown
Member

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 requested a review from a team February 11, 2019 19:54
@mary-poppins

mary-poppins commented Feb 11, 2019

Copy link
Copy Markdown

You can preview e646af3 at https://pr28616-e646af3.ngbuilds.io/.

@mary-poppins

mary-poppins commented Feb 12, 2019

Copy link
Copy Markdown

You can preview c66eeb3 at https://pr28616-c66eeb3.ngbuilds.io/.

@mary-poppins

mary-poppins commented Feb 12, 2019

Copy link
Copy Markdown

You can preview e266ce3 at https://pr28616-e266ce3.ngbuilds.io/.

@filipesilva filipesilva added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Feb 12, 2019
alxhub
alxhub requested changes Feb 12, 2019
View reviewed changes
Comment thread packages/compiler-cli/src/perform_compile.ts Outdated

@alxhub alxhub Feb 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@filipesilva filipesilva Feb 13, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mary-poppins

mary-poppins commented Feb 12, 2019

Copy link
Copy Markdown

You can preview 51fa0cc at https://pr28616-51fa0cc.ngbuilds.io/.

@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

mary-poppins commented Feb 13, 2019

Copy link
Copy Markdown

You can preview 9b0e0f4 at https://pr28616-9b0e0f4.ngbuilds.io/.

@mary-poppins

mary-poppins commented Feb 13, 2019

Copy link
Copy Markdown

You can preview ff92ef7 at https://pr28616-ff92ef7.ngbuilds.io/.

@filipesilva
feat(compiler-cli): make enableIvy ngtsc/true equivalent
86158af 
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.
@mary-poppins

mary-poppins commented Feb 13, 2019

Copy link
Copy Markdown

You can preview 86158af at https://pr28616-86158af.ngbuilds.io/.

@filipesilva filipesilva added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 18, 2019
@filipesilva filipesilva added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Feb 18, 2019
@alexeagle

alexeagle commented Feb 18, 2019

Copy link
Copy Markdown
Contributor

presubmit

@alexeagle alexeagle added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels 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
fix(@schematics/angular): use enableIvy as true
5dcd70b 
Blocked on angular/angular#28616
@filipesilva
filipesilva deleted the enableivy-true branch February 22, 2019 15:16
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Feb 22, 2019
@filipesilva
fix(@schematics/angular): use enableIvy as true
b803fc4 
Blocked on angular/angular#28616
mgechev pushed a commit to angular/angular-cli that referenced this pull request Feb 22, 2019
@filipesilva @mgechev
fix(@schematics/angular): use enableIvy as true
becd2f5 
Blocked on angular/angular#28616
@angular-automatic-lock-bot

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

Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alxhub alxhub alxhub approved these changes
+1 more reviewer
@alexeagle alexeagle alexeagle approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

7 participants

@filipesilva @mary-poppins @JoostK @alxhub @alexeagle @mhevery @googlebot