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

Improve error message when stop is called. #115

Merged
merged 4 commits into from
May 13, 2020

Conversation

wparad
Copy link
Contributor

@wparad wparad commented May 12, 2020

Spent way to long trying to figure why errors were being thrown. Also helps the myriad of other libraries using shifty:

kimmobrunfeldt/progressbar.js#255

@jeremyckahn
Copy link
Owner

Hi @wparad, sorry for all the trouble you had tracking this down. Thank you for this PR! This all looks pretty good to me. I will test it out and get it released just as soon as I have the time to!

@wparad
Copy link
Contributor Author

wparad commented May 12, 2020

Hi @wparad, sorry for all the trouble you had tracking this down. Thank you for this PR! This all looks pretty good to me. I will test it out and get it released just as soon as I have the time to!

Awesome to hear. I actually wasn't sure about the _attachment property being passed to the resolve and reject functions as a second parameter. I'm not aware of the native promises or promise libraries doing anything with that. But perhaps I am missing something.

@jeremyckahn
Copy link
Owner

Awesome to hear. I actually wasn't sure about the _attachment property being passed to the resolve and reject functions as a second parameter. I'm not aware of the native promises or promise libraries doing anything with that. But perhaps I am missing something.

Oh, I just realized that this is going to change how tween promises get rejected a tiny bit. I'm okay with that, but I think it warrants a minor version bump when I release this!

In that case, I'd like to roll the _attachment data into the first parameter. I can take care of that after merging this!

@jeremyckahn jeremyckahn changed the base branch from master to develop May 13, 2020 01:03
@jeremyckahn
Copy link
Owner

I had to make a few changes to the PR, but I will merge this now. Thank you for the fix @wparad!

@jeremyckahn jeremyckahn merged commit 82f6b4d into jeremyckahn:develop May 13, 2020
@jeremyckahn
Copy link
Owner

This is in 2.9.0.

@kimmobrunfeldt
Copy link

kimmobrunfeldt commented May 13, 2020

This improved error message is great, but I thought it would be perfectly fine to call the stop method while an animation is running without throwing an error.

@jeremyckahn
Copy link
Owner

You raise a good point @kimmobrunfeldt. It's fair to say that it's not an error to prematurely stop a tween. I like the catch/reject Promise semantics to differentiate between "tween completed" and "tween canceled," though I agree that calling all canceled tweens an error might be a little heavy-handed.

I don't have much time to do significant development on Shifty at the moment, but I'll think on a more robust API for this and roll that into 2.10.0 when I can!

@jeremyckahn
Copy link
Owner

FYI @kimmobrunfeldt @wparad, I just released Shifty 2.10.0 which removes the reject logic entirely (d18b278). After giving it some thought, it didn't seem necessary and was just creating problems. Hopefully this makes Shifty easier to use in your projects!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants