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

Make Ivy the default in Angular! #32219

Closed
wants to merge 2 commits into from
Closed

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Aug 20, 2019

🍃

@alxhub alxhub added the target: major This PR is targeted for the next major release label Aug 20, 2019
@alxhub alxhub force-pushed the ivy/default branch 7 times, most recently from efd1c0a to b3ca554 Compare August 20, 2019 21:22
@alxhub alxhub marked this pull request as ready for review August 20, 2019 21:28
@alxhub alxhub requested review from a team as code owners August 20, 2019 21:28
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

lgtm but I wish you left a note why the tsc passthrough option was removed - I suspect that it's because we implemented it initially to make Ivy JIT testing easier but then realized that we don't need it and we should just use tsc proper instead. Can you please clarify? thanks!

@alxhub
Copy link
Member Author

alxhub commented Aug 20, 2019

@IgorMinar I've been focused on getting the tests in the repo fixed - I still need to go back and add proper commit messages. Doing that now!

This option makes ngc behave as tsc, and was originally implemented before
ngtsc existed. It was designed so we could build JIT-only versions of
Angular packages to begin testing Ivy early, and is not used at all in our
current setup.
This commit switches the default value of the enableIvy flag to true.
Applications that run ngc will now by default receive an Ivy build!

This does not affect the way Bazel builds in the Angular repo work, since
those are still switched based on the value of the --define=compile flag.
Additionally, projects using @angular/bazel still use View Engine builds
by default.

Since most of the Angular repo tests are still written against View Engine
(particularly because we still publish VE packages to NPM), this switch
also requires lots of `enableIvy: false` flags in tsconfigs throughout the
repo.

Congrats to the team for reaching this milestone!
@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit comp: ivy action: merge The PR is ready for merge by the caretaker labels Aug 20, 2019
@ngbot ngbot bot added this to the needsTriage milestone Aug 20, 2019
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Aug 20, 2019

@alxhub alxhub added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Aug 20, 2019
@alxhub
Copy link
Member Author

alxhub commented Aug 20, 2019

Caretaker: I restarted a flaky CI test (test_docs_examples_ivy), but just in case it flakes again I'm adding merge-assistance.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Aug 20, 2019
@AndrewKushnir
Copy link
Contributor

FYI, Ivy and VE presubmits look normal.

AndrewKushnir pushed a commit that referenced this pull request Aug 20, 2019
This commit switches the default value of the enableIvy flag to true.
Applications that run ngc will now by default receive an Ivy build!

This does not affect the way Bazel builds in the Angular repo work, since
those are still switched based on the value of the --define=compile flag.
Additionally, projects using @angular/bazel still use View Engine builds
by default.

Since most of the Angular repo tests are still written against View Engine
(particularly because we still publish VE packages to NPM), this switch
also requires lots of `enableIvy: false` flags in tsconfigs throughout the
repo.

Congrats to the team for reaching this milestone!

PR Close #32219
ngdevelop-tech pushed a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
This option makes ngc behave as tsc, and was originally implemented before
ngtsc existed. It was designed so we could build JIT-only versions of
Angular packages to begin testing Ivy early, and is not used at all in our
current setup.

PR Close angular#32219
ngdevelop-tech pushed a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
This commit switches the default value of the enableIvy flag to true.
Applications that run ngc will now by default receive an Ivy build!

This does not affect the way Bazel builds in the Angular repo work, since
those are still switched based on the value of the --define=compile flag.
Additionally, projects using @angular/bazel still use View Engine builds
by default.

Since most of the Angular repo tests are still written against View Engine
(particularly because we still publish VE packages to NPM), this switch
also requires lots of `enableIvy: false` flags in tsconfigs throughout the
repo.

Congrats to the team for reaching this milestone!

PR Close angular#32219
const fs = require('fs');
const configPath = 'demo/tsconfig.json';
const config = {
...JSON.parse(fs.readFileSync(configPath, 'utf8')),
Copy link
Contributor

Choose a reason for hiding this comment

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

drive-by nit: I learned yesterday that there's no need to string-decode the buffer first, JSON.parse handles buffers just fine

sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
This option makes ngc behave as tsc, and was originally implemented before
ngtsc existed. It was designed so we could build JIT-only versions of
Angular packages to begin testing Ivy early, and is not used at all in our
current setup.

PR Close angular#32219
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
This commit switches the default value of the enableIvy flag to true.
Applications that run ngc will now by default receive an Ivy build!

This does not affect the way Bazel builds in the Angular repo work, since
those are still switched based on the value of the --define=compile flag.
Additionally, projects using @angular/bazel still use View Engine builds
by default.

Since most of the Angular repo tests are still written against View Engine
(particularly because we still publish VE packages to NPM), this switch
also requires lots of `enableIvy: false` flags in tsconfigs throughout the
repo.

Congrats to the team for reaching this milestone!

PR Close angular#32219
@angular-automatic-lock-bot
Copy link

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 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants