Skip to content
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(animation) get/setKeyframes throws when animation isn't defined. #215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrisgervang
Copy link
Collaborator

@chrisgervang chrisgervang commented Jan 12, 2022

Requesting keyframes for animations that aren't defined shouldn't throw, instead a more graceful behavior of returning undefined (for get) or doing nothing (for set) is preferred.

@chrisgervang chrisgervang added the bug Something isn't working label Jan 12, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1685551732

  • 20 of 20 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.1%) to 28.149%

Totals Coverage Status
Change from base Build 1685540636: 2.1%
Covered Lines: 257
Relevant Lines: 791

💛 - Coveralls

@hkim8567
Copy link
Collaborator

It seems like for both cases(existing or non-existing animation), setKeyframes' behavior is identical. Would it be possible for code to get some feedback or differentiate if it couldn't find the animation?

@chrisgervang
Copy link
Collaborator Author

It seems like for both cases(existing or non-existing animation), setKeyframes' behavior is identical. Would it be possible for code to get some feedback or differentiate if it couldn't find the animation?

In normal runtime, I don't think feedback is useful and can effect performance. There's a logger used in other vis.gl libraries that has levels, and I think it'd be great to print that it couldn't be found at a debug level of 3 or 4. I'll take a note to add this once that logger is working in hubble (I ran into integration issues).

@hkim8567
Copy link
Collaborator

I'm not too familiar with how to get/setKeyframes are being used, but curious to know why it's not ideal to throw an error. If I tried and set animation, and I got a wrong animation id that resulted in getting/SetKeyframes not working, I would want instant feedback on that so I can go back and change my animation id.

@chrisgervang
Copy link
Collaborator Author

They may be used in an async context where the getter/setter call may be statically defined, but the animation won't be set until the data loads. In that case, returning undefined or doing nothing is useful behavior in the final product.

I do think getting some feedback is important, so we can wait to merge until the logger is in place, but I don't typically throw errors unless it's completely necessary. Most JS devs check for undefined.
Deck's error handling system doesn't throw errors by default, since a partially working scene is better than a crashed program. https://deck.gl/docs/api-reference/core/deck#onerror
I think something like this would be a good in-between. It gives feedback without crashing the program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants