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

docs: add ivy opt-in docs #28569

Closed
wants to merge 1 commit into from

Conversation

filipesilva
Copy link
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 new behavior?

Adds a Ivy opt-in instructions.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@@ -0,0 +1,68 @@
# Using Ivy with Angular CLI
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the best place to add this page in?

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure. @jenniferfell can you please propose some place?

Copy link
Contributor

Choose a reason for hiding this comment

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

This doc should be placed in the aio/content/guide directory. I think it would be good also to rename the doc to something like ivy-preview if this is going to be temporary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed and added under the Techniques section of navigation as Ivy Preview. @StephenFluin @jenniferfell can you confirm this location is the appropriate one?

aio/content/cli/ivy.md Outdated Show resolved Hide resolved
aio/content/cli/ivy.md Outdated Show resolved Hide resolved
@mary-poppins
Copy link

You can preview 0982573 at https://pr28569-0982573.ngbuilds.io/.

aio/content/cli/ivy.md Outdated Show resolved Hide resolved
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.

great start!

Let's wait for the blog post to land and experimentalIvy boolean flag to land and then merge this PR

@@ -0,0 +1,68 @@
# Using Ivy with Angular CLI
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure. @jenniferfell can you please propose some place?

aio/content/cli/ivy.md Outdated Show resolved Hide resolved
aio/content/cli/ivy.md Outdated Show resolved Hide resolved
aio/content/cli/ivy.md Outdated Show resolved Hide resolved
aio/content/cli/ivy.md Outdated Show resolved Hide resolved
@IgorMinar IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews state: blocked labels Feb 8, 2019
@filipesilva
Copy link
Contributor Author

Blocked on #28616.

@mary-poppins
Copy link

You can preview 58af057 at https://pr28569-58af057.ngbuilds.io/.


Using a postinstall hook to run `ngcc` is just a temporary integration.
We expect `ngcc` to be seamlessly integrated into the Angular CLI build pipeline in the future before the full Ivy rollout.
Once that's implemented `ngcc` will and not be visible to developers.
Copy link
Contributor

Choose a reason for hiding this comment

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

@filipesilva, this is really good!

Should we mentioned the limitation that if one has multiple projects within the workspace, all of them will need to be build using Ivy? As users will have cryptic error messages on projects that rely on ngc but ngcc was run.

At least until ngcc is around and not part of the build pipeline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note about that:

Until that happens, opting into Ivy means that all projects in a single CLI workspace will be compiled with Ivy.

@filipesilva
Copy link
Contributor Author

@jenniferfell can you review and advise on where the best place to link this page in is please?

@filipesilva filipesilva added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: blocked state: WIP action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 20, 2019
@mary-poppins
Copy link

You can preview a743c6d at https://pr28569-a743c6d.ngbuilds.io/.

{
"compilerOptions": { ... },
"angularCompilerOptions": {
"enableIvy": true,
Copy link
Member

Choose a reason for hiding this comment

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

Isn't "allowEmptyCodegenFiles": true required any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is still required right now. But #28685 (or a similar approach) will make it not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it here for now, to remove later when the other PR is in.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@mary-poppins
Copy link

You can preview 6a57d06 at https://pr28569-6a57d06.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 7cdd898 at https://pr28569-7cdd898.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 16b9b39 at https://pr28569-16b9b39.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 3a91668 at https://pr28569-3a91668.ngbuilds.io/.

@mary-poppins
Copy link

You can preview e6211d6 at https://pr28569-e6211d6.ngbuilds.io/.

@filipesilva filipesilva removed action: review The PR is still awaiting reviews from at least one requested reviewer action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 26, 2019
@filipesilva
Copy link
Contributor Author

filipesilva commented Feb 26, 2019

To summarize, this PR is ready to merge but there are still open action items that should be addressed and whose changes are to be fed back into this document:


Using a postinstall hook to run `ngcc` is just a temporary integration.
We expect `ngcc` to be seamlessly integrated into the Angular CLI build pipeline in the future before the full Ivy rollout.
Once that's implemented `ngcc` will and not be visible to developers.

Choose a reason for hiding this comment

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

remove 'and' from this sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey thanks for taking a look! Yes that shouldn't be there, removed now.

@mary-poppins
Copy link

You can preview c030eeb at https://pr28569-c030eeb.ngbuilds.io/.

{
"url": "guide/ivy",
"title": "Angular Ivy",
"tooltip": "Opt-ing into Angular Ivy with Angular CLI."
Copy link
Member

Choose a reason for hiding this comment

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

Opting is a word - no hyphen needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed mentions of opt-ing to opting but left mentions of opt-in as is.

@mary-poppins
Copy link

You can preview a907c80 at https://pr28569-a907c80.ngbuilds.io/.

@benlesh benlesh closed this in f01247f Feb 26, 2019
mgechev pushed a commit to angular/angular-cli that referenced this pull request Feb 26, 2019
filipesilva added a commit to filipesilva/angular that referenced this pull request Feb 27, 2019
Followup to angular#28569 (comment) and angular/angular-cli#13773.

Note: this flag will only work on `@angular/cli@8.0.0-beta.3` (currently unreleased) and above.
@filipesilva filipesilva mentioned this pull request Feb 27, 2019
14 tasks
@filipesilva filipesilva deleted the docs-ivy-optin branch February 28, 2019 10:07
filipesilva added a commit to filipesilva/angular that referenced this pull request Mar 6, 2019
Followup to angular#28569 (comment) and angular/angular-cli#13773.

Note: this flag will only work on `@angular/cli@8.0.0-beta.3` (currently unreleased) and above.
AndrewKushnir pushed a commit that referenced this pull request Mar 6, 2019
Followup to #28569 (comment) and angular/angular-cli#13773.

Note: this flag will only work on `@angular/cli@8.0.0-beta.3` (currently unreleased) and above.

PR Close #29010
@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 14, 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 aio: preview cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet