Skip to content

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Jul 29, 2019

  • feat(@schematics/angular): introduce Ivy workspace concept

At the moment, to enable Ivy as the view rendering engine we are adding enableIvy: true in each of the applications tsconfigs.
While this works this is not ideal when users want to opt-out, as users will need to modify multiple tsconfig.

With the introduction of Ivy libraries for development this will be even harder and cumbersome.

We added the enableIvy option to the workspace schematic and remove the option for the application schematic.

ng new <project-name> --enable-ivy will pass down down the option the workspace schematic and an Ivy workspace will be created.
Other schematics such as application and library will read the workspace (root) tsconfig to verify if a application or library with
Ivy structure and configuration should be created.

  • feat(@schematics/angular): introduce Ivy libraries for development

Since NGCC is non incremental and in library projects we have the original TS sources
we don't need to build a library using the VE and transform it using NGCC. Instead we can build the library using NGTSC (Ivy) directly
as this enables faster incremental compilations and a better development experience.

Libraries now have a production configuration, which enabled VE compilations. As it is not recommended to publish NGTSC (Ivy)
built libraries to NPM repositories, since Ivy libraries are not backwards compatible with the legacy View Engine.

  • feat(@schematics/angular): ivy library migration

Add a migration to migrate existing libraries to the new library layout considering it will be the default in version 9.

@@ -19,12 +19,6 @@
},
"x-prompt": "What name would you like to use for the application?"
},
"enableIvy": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this is experimental, I don't think we need to deprecate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that's ok.

Copy link
Member

Choose a reason for hiding this comment

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

Don't we still need this option? The migration won't run until 9 so the option won't be at the root tsconfig yet.

@@ -48,7 +48,6 @@ Note: There's a limit of 20 custom dimensions.
| 5 | `Flag: --style` | `string` |
| 6 | `--collection` | `string` |
| 7 | `--buildEventLog` | `boolean` |
| 8 | `Flag: --enableIvy` | `boolean` |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to inform @StephenFluin about this, if we are going to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to keep it to see if users are opting out on project creation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But the flag itself no longer exists on ng generate application, hence there is no data to collect.

enableIvy exists on ng new and ng generate workspace

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's true. This file doesn't mention anything about what commands capture it though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, I will add this to ng-new to that this matrix is captured there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alan-agius4 alan-agius4 requested a review from filipesilva July 29, 2019 11:39
@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Jul 29, 2019
@alan-agius4 alan-agius4 marked this pull request as ready for review July 29, 2019 12:07
@alan-agius4
Copy link
Collaborator Author

@filipesilva, we should have some documentation on AIO and make it clearer not to publish Ivy libraries to NPM.

Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

Just a minor comment and a test request, everything else looks good. Great work!

@@ -48,7 +48,6 @@ Note: There's a limit of 20 custom dimensions.
| 5 | `Flag: --style` | `string` |
| 6 | `--collection` | `string` |
| 7 | `--buildEventLog` | `boolean` |
| 8 | `Flag: --enableIvy` | `boolean` |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to keep it to see if users are opting out on project creation.

At the moment, to enable Ivy as the view rendering engine we are adding `enableIvy: true` in each of the applications tsconfigs.
While this works this is not ideal when users want to opt-out, as users will need to modify multiple tsconfig.

With the introduction of Ivy libraries for development this will be even harder and cumbersome.

We added the `enableIvy` option to the `workspace` schematic and remove the option for the `application` schematic.

`ng new <project-name> --enable-ivy` will pass down down the option the workspace schematic and an Ivy workspace will be created.
Other schematics such as `application` and `library` will read the workspace (root) tsconfig to verify if a application or library with
 Ivy structure and configuration should be created.
Since `NGCC` is non incremental and in library projects we have the original TS sources
we don't need to build a library using the `VE` and transform it using `NGCC`. Instead we can build the library using `NGTSC` (Ivy) directly
 as this enables faster incremental compilations and a better development experience.

 Libraries now have a `production` configuration, which enabled `VE` compilations. As it is not recommended to publish NGTSC (Ivy)
 built libraries to NPM repositories, since Ivy libraries are not backwards compatible with the legacy View Engine.
Add a migration to migrate existing libraries to the new library layout considering it will be the default in version 9.
Previously, we were not testing web workers under Ivy as we were overriding tsconfig.app.json entirely without including enableIvy.

When enabling Ivy we are getting warning of files that are part of the compilation which causing a warning to show and break the test.
@@ -19,12 +19,6 @@
},
"x-prompt": "What name would you like to use for the application?"
},
"enableIvy": {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we still need this option? The migration won't run until 9 so the option won't be at the root tsconfig yet.

* Returns `true` when the workspace root tsconfig is set to use Ivy
* as the Angular rendering engine.
*/
export function isIvyWorkspace(tree: Tree): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is the correct file for this function since its not related to reading/writing the workspace file.

The function will also not handle extended configurations which would make it problematic to generate an application with ivy enabled in such a scenario.

Copy link
Collaborator Author

@alan-agius4 alan-agius4 Jul 30, 2019

Choose a reason for hiding this comment

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

Is it common to have extended root level tsconfig? (Not application specific)? Example. /tsconfig.json extends /tsconfig.base.json, pretty much in our schematics we only handle /tsconfig.json as a base for other tsconfigs.

Re the file, any thoughts were it so go?

@alan-agius4
Copy link
Collaborator Author

@clydin

Don't we still need this option? The migration won't run until 9 so the option won't be at the root tsconfig yet.

No it's not needed any more, since now we have the concept of an ivy workspace instead of an Ivy application.

Though that is a good question about existing workspaces which are not Ivy but want to generate an Ivy application. Should this be supported?

//cc @filipesilva

@clydin
Copy link
Member

clydin commented Jul 30, 2019

There's also the situation of existing ivy generated workspaces that are not configured as ivy workspaces.

@alan-agius4 alan-agius4 added the needs: discussion On the agenda for team meeting to determine next steps label Jul 30, 2019
@filipesilva
Copy link
Contributor

I think it's not a deal breaker if people have to manually set enableIvy. Realistically speaking you don't make apps/libs very often. And when you do, you mostly want to use the same compiler. I don't think many people would have to set it manually.

@alan-agius4 alan-agius4 added state: blocked and removed needs: discussion On the agenda for team meeting to determine next steps labels Aug 8, 2019
@alan-agius4
Copy link
Collaborator Author

Superseded by #15320

@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
state: blocked target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants