Repository navigation
Update Ivy opt-in doc - #29955
Update Ivy opt-in doc#29955filipesilva wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
@alexeagle has the deprecation gone in yet?
Also, should we say deprecated in Ivy or just deprecated?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I recommend removing the third list item (lines 40-49) and adding a new section below
|
You can preview 054f4b5 at https://pr29955-054f4b5.ngbuilds.io/. |
jbogarthyde
left a comment
There was a problem hiding this comment.
Put route syntax change in context
There was a problem hiding this comment.
I recommend removing the third list item (lines 40-49) and adding a new section below
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
We seem to have a dedicated document to this change coming in #29730. @alexeagle should this information be there?
There was a problem hiding this comment.
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.
|
@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? |
|
You can preview c1a19b5 at https://pr29955-c1a19b5.ngbuilds.io/. |
|
See revision above - the issue here is what you have to do if you enable ivy and then disable it. |
|
@jbogarthyde the only thing a user needs to do if they enable ivy and then disable it is to flip the |
|
Great -- Then you can take all of this out. |
| - 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done, please take a look.
|
You can preview 8289257 at https://pr29955-8289257.ngbuilds.io/. |
|
I don't think we should merge this until we're certain that the fix-lazy-routes can remain in ng update 8 |
| @@ -13,14 +13,11 @@ ng new shiny-ivy-app --enable-ivy | |||
|
|
|||
| The new project is automatically configured for Ivy. | |||
There was a problem hiding this comment.
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.
| @@ -13,14 +13,11 @@ ng new shiny-ivy-app --enable-ivy | |||
|
|
|||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Went with Idea 2, but have no strong opinion either way.
| @@ -13,14 +13,11 @@ ng new shiny-ivy-app --enable-ivy | |||
|
|
|||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Changed title to be just Opting into Angular Ivy. Ivy is supported in other toolchain, but it is more complicated to use.
|
You can preview c3d0cd3 at https://pr29955-c3d0cd3.ngbuilds.io/. |
IgorMinar
left a comment
There was a problem hiding this comment.
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.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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?
Other information