-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(ng-add): allow using noop animations #13429
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
fix(ng-add): allow using noop animations #13429
Conversation
|
|
||
| describe('animations disabled', () => { | ||
|
|
||
| it('should not add @angular/animations to package.json', () => { |
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.
Isn't this always required, though, since @angular/material imports stuff from @angular/animations?
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.
I think it depends from where people import. Most imports from @angular/animations are just types that won't be part of the JS.
My reasoning for this option was just that there might be people who don't want the schematics to touch their package.json/app.module.ts. For the general public, I think it would be better if we just add the NoopAnimationsModule or BrowserAnimationsModule and always ensure @angular/animations is set up?
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's probably worth checking, but I'm reasonable sure that the app won't build if @angular/animations isn't installed. I don't want there to be any paths where the user ends up with an app that doesn't build.
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.
Yeah, it would be wrong to open any "path" where the user ends up with an application that doesn't build.
I've updated this PR to either add the NoopAnimationsModule or BrowserAnimationsModule. This also includes a safety check that ensures that we don't "magically" add the BrowserAnimationsModule or NoopAnimationsModule if the project already explicitly uses an animation module. Please have another look.
0398dce to
36f448d
Compare
jelbourn
left a comment
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
* Currently we always install `@angular/animations` and add the `BrowserAnimationsModule` to the project module. There should be a prompt/option that allows developers to use the `NoopAnimationsModule`. * Also no longer adds the `BrowserAnimationsModule` if the project already uses the `NoopAnimationsModule` (avoiding magic). Same applies for the `NoopAnimationsModule`. We won't add the noop animations module if the browser animations module is configured. Handle existing animation modules
36f448d to
2ac6eaf
Compare
* Currently we always install `@angular/animations` and add the `BrowserAnimationsModule` to the project module. There should be a prompt/option that allows developers to use the `NoopAnimationsModule`. * Also no longer adds the `BrowserAnimationsModule` if the project already uses the `NoopAnimationsModule` (avoiding magic). Same applies for the `NoopAnimationsModule`. We won't add the noop animations module if the browser animations module is configured. Handle existing animation modules
|
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. |
Currently we always install
@angular/animationsand add theBrowserAnimationsModuleto the project module. There should be a prompt/option that allows developers to use theNoopAnimationsModule.No longer adds the
BrowserAnimationsModuleif the project already uses theNoopAnimationsModule(avoiding magic). Same applies for theNoopAnimationsModule. We won't add the noop animations module if the browser animations module is configured.