Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Hey Nathan,
There is an isPlaying property of animations that you can check before calling play() or cancel().

Trying to cancel() an animation that is not currently playing is probably caused by an error in the application code - thus we decided to throw in such cases. If it is intentional - you can always do the check with isPlaying?

Thanks @vakrilov - we always check isPlaying before attempting to call cancel. It's a side effect likely as a result of CSS processing keyframe animations in unforeseen ways (views getting destroyed when navigating or some other reasons, etc) - that neither you nor I can anticipate and really should not be considered exceptional.

A brief inspection of tns-core-modules reveals 198 matches across 67 files where athrow is raised.
This breaks down to an average ~3 errors for every source file.
This looks over 538 individual files accounting for 2.5 Mb of source code in tns-core-modules.

https://stackoverflow.com/a/77164

beware, throwing [...] exception can kill a senior user who is just a slow typer.

+1 excellent answer. I am so frustrated by developers working on API's that I have to consume, and throw Exceptions for every little thing. VERY few cases really require exceptions.

Exceptions should be reserved for what's truly exceptional.

@vakrilov Is this animation condition truly exceptional?

Reasons not to use exceptions:
Sometimes it's overkill if the error handling is simple.
Throwing and catching exceptions is generally significantly more expensive...

The framework core currently makes developing bug free apps more difficult due to the extraneous and voluminous usage of throw.

What do you recommend?

/cc @jlooper @tjvantoll @sipacate @sebawita @rdlauer

Hey @NathanWalker
In this particular example - indeed trying to play() an animation that is already playing might not be a case for exception, although it might still be an indicator of faulty logic. If the general opinion is that this will make a better and more easy to use API - sure, let's change it.

Looking at the crash analysis report you've attached - it is the exception thrown in keyframe-animation.ts file that is causing trouble, so I would suggest applying this approach in this file also.

As for throwing exceptions in general - I absolutely agree that exceptions should not be used as a control flow mechanism. Non of that APIs in the core framework are designed in a way that requires you to catch exceptions in order achieve your goal.

What we do use as a guiding principle when writing code is fail early/fail early. Most of the throw statements in the code are there to warn you about a problem on the first possible place when it is visible. It is better to get a crash with a stack-trace than debug a program that is just not working and you don't know why. The exceptions are meant to point you to the exact place of a potential bug while you are developing and testing, not to crash your app in production.

That said, I don't agree that judging the quality and ergonomics of a framework, based on the number of throws in the code is fair.

Thank you for the understanding @vakrilov and pointing out keyframe-animation.ts file, likely the worst offender since CSS animation issues have been reported by others in several other outlets (forums,issues,etc.). I have updated the PR to include an adequate fix for that file as well.

Big believer in fail fast/fail early however around animation handling doing the correct thing to help a sequence continue/succeed vs. fail is a far better route to take. Consider the comment in the SO post you mentioned:

Consider this invalid HTML: <div><b>Bold Text!</div> It's missing the </b>, but web browsers choose to do the opposite of "fail early" by trying to recover from the problem and hoping it doesn't make things worse. In their case, it's a sensible route to take.

The browser vendors do the right thing. Here we want to help an animation sequence/condition continue while still allowing the developer to dive deeper by turning tracing on to gain deeper insight if they need to. An appreciated priority would be on ensuring dev's can create animations via CSS keyframes or otherwise without coming to find out some 1% of users find a path that was impossible to find during testing and worse not even fixable by the developer themselves as is the case here since it is coming from the core framework in use.

1% of a 1.5 million user base is 15,000 users that were adversely affected by the throw that this PR effectively solves.

Please let me know if there are other concerns to these changes and I can adjust per your advice.

Hey thanks for your explanations.
Indeed you are right that for animations a more forgiving API is perhaps more useful.

I have run the CI tests. Can I ask you to write a couple of unit tests (probably in animation-tests.ts assuring this behavior:

• Calling play() second time does not trow but just rejects.
• Calling cancel() does nothing if animation is not playing).

test

@vakrilov Changes made and tests added 👍

Any way we could see this in a hotfix release? This appears to have been a regression in 3.4.x. I had none of these before 3.4, but 3.4 seems to have surfaced this and I'm seeing it at around a ~2% user rate in production. I'd be happy to take an rc tagged build for this to roll out to production for confirmation.
https://pasteboard.co/H9YhUDp.png

@NathanWalker the unit tests you have written seem to fail now on Android (that's what test do).

Indeed that was helpful because seems there is something we've missed. There is a platform-specific implementation for both play() and cancel() methods for android and ios. The code there will still execute even though the super.play()/super.cancel() calls do not throw.

Ok excellent I’ll fix that up tomorrow since I’m going to bed now. If you knew the fix and wanted to pull my branch to finish it up would be fine, otherwise I’ll get in morning.

NathanWalker#1 ;)

Perfection thank you 👍

test

@vakrilov I'm unable to see the jenkins test detail - just lemme know if any other modifications are needed to get those to clear 👍 - thanks again.

test branch_animations#master

Hey @NathanWalker we are currently investigating the failed tests from the CI and I need some time. I just want to let you know that we are working on it. Most probably the issue is not related to this PR but I need to be sure.

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.