jeremyckahn / shifty

The fastest TypeScript animation engine on the web
https://jeremyckahn.github.io/shifty/doc/
MIT License
1.54k stars 88 forks source link

Protected config variable from mutation by adding clone function #58

Closed ZainManji closed 9 years ago

ZainManji commented 9 years ago

When creating a Tweenable object and then trying to seek, the config which was initially passed into the Tweenable object's constructor get's mutated by the timeoutHandler function. It was causing my seeks to be inconsistent.

I added a clone function to make sure the config's from and to property doesn't get mutated.

e.g. var config = {from: {opacity: 0}, to: {opacity:1}, duration 100}; var tween = new Tweenable(duration); tween.seek(20); //Config get's changed to {from: {opacity: 0.2}, to: {opacity:1}, duration 100}; tween.seek(60); //Config get's changed to {from: {opacity: 0.6}, to: {opacity:1}, duration 100};

jeremyckahn commented 9 years ago

Thanks for putting this Pull Request together, but I would prefer not to have this change in the library right now. This behavioral quirk is documented in the README, and for a majority of use cases, it is not a problem. For your use case, I suggest cloning the configuration object before passing it to Shifty. There's a performance cost associated with cloning objects, and not all environments provide the JSON host object. Additionally, this implementation detail change may break Shifty for other users if they upgrade.

I can see this being a good fit for a 2.0 release, but I don't want to change the library in a potentially incompatible way for other users of 1.x. I will leave this issue open as an improvement for 2.0.

SBoudrias commented 9 years ago

Hey @jeremyckahn, I think this behavior is actually a bug.

Thing is when the animation is over, it cannot go back or be replayed as the value of from is completely lost.

The issue @ZainManji have is not about reusing opt.from himself, it's that shifty is losing the initial values and don't know how to seek back once the tween is completed.

It would make complete sense for shifty not to mutate the property and retain the animation config at all time.

This is a big blocker for us.

jeremyckahn commented 9 years ago

Thinking about it more, I can see that there's a good case to be made for not mutating the provided config objects. I would prefer to stay away from the JSON host object and instead use the internal shallowCopy method to achieve the same effect. @ZainManji or @SBoudrias, can you pull down my bugfix/clone-input-values branch and see if that works for you? If it seems good to you, I will merge it into master.

jeremyckahn commented 9 years ago

Did you get a chance to try out my alternative fix, from my previous comment?

ZainManji commented 9 years ago

It works. It's not really an issue for me anymore, but it would be nice to have that implemented.

jeremyckahn commented 9 years ago

Fixed in 20158059a76d75802a0d0c824ae9fdf9ff4e517f. Thanks for reporting!