fix(animations): allow animations with unsupported CSS properties

[dario-piotrowicz]

currently animations with unsupported CSS properties cause a hard error
and the crash of the animation itself, instead of this behaviour just
ignore such properties and provide a warning for the developer in
the console (only in dev mode)

resolves #23195

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

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:

Issue

Issue Number: #23195

Does this PR introduce a breaking change?

• Yes
• No

I don't think it's a breaking change, unless someone was actually in some circumstances relying on the animations crushing when finding unsupported CSS properties 🤔

Other information

You can notice that the warnings I've added are strings which is inconsistent with the errors being anys, I believe the errors should actually also be strings and I've already opened #44726 to update their type 🙂

[jessicajaniuk]

Thanks, @dario-piotrowicz! I like this a lot. Definitely take a look at the comment below. One thing I've been thinking about as I read this is that there's similar code used every time you output them to console.warn. This suggests to me maybe an abstraction makes sense. What do you think?

[dario-piotrowicz]

Thanks, @dario-piotrowicz! I like this a lot. Definitely take a look at the comment below. One thing I've been thinking about as I read this is that there's similar code used every time you output them to console.warn. This suggests to me maybe an abstraction makes sense. What do you think?

Thanks! 😃

Yes an abstraction would make total sense, the issue is that if you look at the console.warns they are not all the same, I am actually following what the errors do (before each of my console.warns you can see an error being thrown and that I am generating the warning string in the same manner as the error throw).

I am not sure why they are different and figured the safest thing would be to follow that for the introduction of warnings, but I could definitely abstract the console warnings in some reusable way and then we can pick the errors up separately?

or would it make sense to just keep the warnings as they are for this PR and abstract both errors and warnings at the same time in a separate PR? 🤔

regardless there are many ways these errors can pop up, I haven't really tried triggering them all, I will have to have an investigation in this too to see if there is a reason we have different formats for errors in different places or not (probably not, but still would be good to have a look around I think 😄)

[dario-piotrowicz]

@jessicajaniuk I've looked into the errors and they are all just results of buildAnimationAst calls, so it doesn't really look like there is any reason to present them differently (on the contrary, it would be good to display them in a consistent format)

So I'll add a commit to add some shared functionality for the errors and warnings 🙂👍

[dario-piotrowicz]

@jessicajaniuk I've added shared functions for both the errors throwing and the warnings logging, please have a look 😄

---

By the way, I originally added the new functions to the util.ts file but that didn't work as it would create a circular dependency:

The issue is that currently browser/src/util.ts depends on the browser/src/render/shared.ts which to me seems quite wrong, the dependency seems to be of a single function (isNode) so just moving that into util should solve things 🤔 , what do you think @jessicajaniuk? if you agree with the change, would it be ok if I just added a todo comment to make the change in this PR and do the actual refactoring in a follow-up PR? (just to try not to increase the scope of the current one)

[jessicajaniuk]

@dario-piotrowicz I managed to break this again with the Error tree shaking pull request. So you'll want to take a look again. We'll want to be sure that warnings are also tree shakable, as well. Definitely take a look and let me know if you need any pointers. I'm pretty sure we can still get this in the v14 release.

[dario-piotrowicz]

@jessicajaniuk PR fixed, for the warning I tried to follow what you did for the errors in the way that made more sense to me, hopefully I got it right, please let me know what you think 🙂

---

PS: there were so many things to change that I just force-pushed a single commit in and cleared all the rest, sorry if it creates any inconvenience 🙇

[jessicajaniuk]

@dario-piotrowicz This is super close. I like the changes, and I have a few suggestions for making it a bit cleaner here. Should be very quick to address and we should be able to land it.

[jessicajaniuk]

LGTM 🍪

Thanks for working through this, @dario-piotrowicz!

[dario-piotrowicz]

LGTM cookie

Thanks for working through this, @dario-piotrowicz!

Thank you for the awesome reviewing, sorry I went in the wrong direction a couple of times! 😜

[alxhub]

@dario-piotrowicz currently this is tagged as feat, meaning it won't be mergeable into the 13.2.x branch. However, this definitely feels like a bugfix instead. Should we update the commit tag and land this as a fix?

[dario-piotrowicz]

@dario-piotrowicz currently this is tagged as feat, meaning it won't be mergeable into the 13.2.x branch. However, this definitely feels like a bugfix instead. Should we update the commit tag and land this as a fix?

Sorry I am not really sure.... the original code was purposely stopping animations for unsupported properties, and I changed it to ignore them instead... so it does sort of sound like a feature (even if a very small one) 🤷

I don't even know what difference it makes to land this as a feat or a bugfix, I guess I don't mind either way, please let me know what you think/suggest

(@jessicajaniuk, what do you think? 🙂)

[dario-piotrowicz]

@alxhub Updated to fix 🙂👍

[atscott]

FYI - This had a conflict with 13.2.x. Please open a new PR to target that branch only.

[atscott]

This PR was merged into the repository by commit f8dc660.

[angular-automatic-lock-bot]

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.}