-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Angular: Add built-in Ivy support instead of relying on addon #15229
Conversation
This addon integrates storybook-angular-addon-ivy into the mono-repo. This allows us to remove the ivy addon package.
Nx Cloud ReportCI ran the following commands for commit ffc861a. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch
Sent with 💌 from NxCloud. |
options | ||
); | ||
|
||
if (!angularOptions.enableIvy) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should actually default to "true" if user-land has not passed in enableIvy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for:
- Fixing the default value handling
- documentation
*/ | ||
export const runNgcc = () => { | ||
ngccProcess({ | ||
// should be async: true but does not work due to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we change this or should we wait ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work without being async. The downside is if someone starts the user app + storybook at the same time and both start ngcc
then storybook will most likely crash
So it's an edge case but it also does not work yet to set it to async
@shilman I added docs and successfully tested it locally with |
@shilman any idea why the e2e-tests-core step fails? |
@kroeder This is falling on next also. @gaetanmaisse is on top of it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Issue:
What I did
This PR integrates storybook-angular-addon-ivy into
the mono-repo. This allows us to remove the ivy addon
package.
How to test
tba