Skip to content

Update Ivy opt-in doc - #29955

Closed
filipesilva wants to merge 4 commits into
angular:masterfrom
filipesilva:ivy-optin-no-aot
Closed

filipesilva wants to merge 4 commits into
angular:masterfrom
filipesilva:ivy-optin-no-aot

Conversation

@filipesilva

@filipesilva filipesilva commented Apr 17, 2019 •

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?

The doc asks users to always use AOT in Ivy, and says the loadChildren string syntax is not supported.

What is the new behavior?

The doc does not ask users to always use AOT in Ivy, and says the loadChildren string syntax is deprecated.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@filipesilva
filipesilva requested review from a team and jbogarthyde April 17, 2019 09:50
@filipesilva
filipesilva requested a review from alexeagle April 17, 2019 09:50
Comment thread aio/content/guide/ivy.md Outdated

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.

@alexeagle has the deprecation gone in yet?

Also, should we say deprecated in Ivy or just deprecated?

@jbogarthyde jbogarthyde Apr 17, 2019 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the string syntax still supported if Ivy is not enabled? Is it deprecated in v8, to be replaced by the import syntax? Will that still be "experimental"?
@jenniferfell Does our deprecation policy cover this case?
I think we may need to actually explain what's happening and why, rather than just refer to it in a code comment.

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.

The string syntax is supported in both Ivy and View Engine on v8, but should be marked as deprecated. It should be replaced by the import syntax after the deprecation period ends.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs more explanation about why you have to do this, and what the connection is with Ivy.
It sounds like the new technique is implemented as part of Ivy, but you want people to switch to it even when Ivy is not enabled, by using a temporary experimental implementation. Is that right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I recommend removing the third list item (lines 40-49) and adding a new section below

@mary-poppins

Copy link
Copy Markdown

You can preview 054f4b5 at https://pr29955-054f4b5.ngbuilds.io/.

@jbogarthyde jbogarthyde left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Put route syntax change in context

Comment thread aio/content/guide/ivy.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I recommend removing the third list item (lines 40-49) and adding a new section below

Comment thread aio/content/guide/ivy.md Outdated

@jbogarthyde jbogarthyde Apr 17, 2019 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove second item, and add this new subsection:

Supporting import of lazy routes without Ivy

The string syntax for the loadChildren route specification is deprecated in Angular version 8, in favor of new syntax that uses an import statement.

(link_to_deprecation_doc when available, in place of following para and example)

As part of updating an application to Angular version 8, it is recommended that you update to the new import syntax.
You can use angular-lazy-routes-fix to perform the transformation automatically.

For example:

const routes: Routes = [{
  path: 'lazy',
  // The string syntax for loadChildren is deprecated:
  //   loadChildren: './lazy/lazy.module#LazyModule'
  // This has been transformed to use the new import syntax.
  loadChildren: () => import('./lazy/lazy.module').then(m => m.LazyModule)
}];

Ivy supports both the deprecated string syntax and the recommended import syntax.
If you have updated your lazy routes to use the import() statement but are not using Ivy,
make sure your default build options in angular.json include the flag "experimentalImportFactories": true.

@jbogarthyde jbogarthyde Apr 17, 2019 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When there is a deprecation doc to point to, this can be simplified to point to that.
Some of this can be used there. @jenniferfell

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree that this content belongs in the upcoming Deprecation Summary doc. After PR 29979 lands, please create a PR against the new deprecation.md doc (#29979). Thanks!

@filipesilva filipesilva Apr 19, 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.

We seem to have a dedicated document to this change coming in #29730. @alexeagle should this information be there?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, I found a place for it there - in that case, we should use the first sentence, a link, and the last paragraph.


Supporting import of lazy routes without Ivy

The string syntax for the loadChildren route specification is deprecated in Angular version 8, in favor of new syntax that uses an import statement. See (link_to_deprecation_doc when available).

Ivy supports both the deprecated string syntax and the recommended import syntax.
If you have updated your lazy routes to use the import() statement but are not using Ivy,
make sure your default build options in angular.json include the flag "experimentalImportFactories": true.

@ngbot ngbot Bot added this to the needsTriage milestone Apr 17, 2019
@filipesilva filipesilva added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 19, 2019
@filipesilva

Copy link
Copy Markdown
Contributor Author

@jbogarthyde @jenniferfell discussed with @alexeagle and instead we should remove the lazy route change from this doc altogether since it is not necessary for Ivy at all.

This leaves the doc reading weirdly, since it was an enumeration of steps that now consists of a single step. Do you have suggestion on how to best reword it?

@filipesilva
filipesilva requested a review from jbogarthyde April 19, 2019 15:54
@mary-poppins

Copy link
Copy Markdown

You can preview c1a19b5 at https://pr29955-c1a19b5.ngbuilds.io/.

@jbogarthyde

Copy link
Copy Markdown
Contributor

See revision above - the issue here is what you have to do if you enable ivy and then disable it.

@filipesilva

Copy link
Copy Markdown
Contributor Author

@jbogarthyde the only thing a user needs to do if they enable ivy and then disable it is to flip the enableIvy flag in tsconfig. There is nothing else a user needs to do. The lazy routes work with the string syntax and with the import syntax in both Ivy and VE.

@jbogarthyde

Copy link
Copy Markdown
Contributor

Great -- Then you can take all of this out.

@filipesilva filipesilva added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 19, 2019
Comment thread aio/content/guide/ivy.md Outdated
- Set `enableIvy` to false in `tsconfig.app.json`, or remove it completely.
- Add `"experimentalImportFactories": true` to your default build options in `angular.json` to support the import statement in `loadChildren` outside Ivy.
- Remove `"aot": true` from your default build options if you didn't have it there before.
- Set `enableIvy` to `false` in `tsconfig.app.json`, or remove it completely. No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make it one sentence:

To stop using the Ivy compiler, set enableIvy to false in tsconfig.app.json, or remove it completely.

I think it doesn't need to be a separate section. Just put it at the end of the previous section.

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, please take a look.

@mary-poppins

Copy link
Copy Markdown

You can preview 8289257 at https://pr29955-8289257.ngbuilds.io/.

@alexeagle

Copy link
Copy Markdown
Contributor

I don't think we should merge this until we're certain that the fix-lazy-routes can remain in ng update 8

Comment thread aio/content/guide/ivy.md Outdated
@@ -13,14 +13,11 @@ ng new shiny-ivy-app --enable-ivy

The new project is automatically configured for Ivy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An idea to handle the now-solo bullet:
The new project is automatically configured for Ivy. Specifically, the enableIvy option is set to true in the project's tsconfig.app.json file.

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.

LGTM, added.

Comment thread aio/content/guide/ivy.md
@@ -13,14 +13,11 @@ ng new shiny-ivy-app --enable-ivy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re "Starting a new project in Ivy." We talk about ng new as "creating" a project. "Starting" can mean many things, including running or serving.

Idea 1: this pair of headings...
Creating a new project to use Ivy
Updating an existing project to use Ivy

Idea 2: this pair of headings...
Using Ivy in a new project
Using Ivy in an existing project

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.

Went with Idea 2, but have no strong opinion either way.

Comment thread aio/content/guide/ivy.md
@@ -13,14 +13,11 @@ ng new shiny-ivy-app --enable-ivy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A thought on the page title. We have mostly stopped highlighting "with the CLI" because 1) it's just how you do things now, it's not a special add-on 2) we don't describe other ways to the tasks. The current page title begs the question: Can I opt into Ivy without the CLI?

I think the page title can just be: Opting into Angular Ivy

If the point is that Ivy isn't yet supported in non-CLI applications, then I would add an alert box with that note near the top of this document.

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.

Changed title to be just Opting into Angular Ivy. Ivy is supported in other toolchain, but it is more complicated to use.

@mary-poppins

Copy link
Copy Markdown

You can preview c3d0cd3 at https://pr29955-c3d0cd3.ngbuilds.io/.

@IgorMinar IgorMinar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this lgtm as is, we can add more stuff to this page with wording about known issues and expectation setting in a follow up PR.

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: review The PR is still awaiting reviews from at least one requested reviewer state: blocked labels Apr 24, 2019
@IgorMinar
IgorMinar removed the request for review from alexeagle April 24, 2019 21:29
@IgorMinar IgorMinar modified the milestones: needsTriage, version 8 Apr 24, 2019
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@angular-automatic-lock-bot

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 15, 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 target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants