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

shifty.queue.js when installing from npm? #90

Closed philraj closed 7 years ago

philraj commented 7 years ago

What's the recommended way to add queueing functionality when installing from npm and loading via Webpack? The docs are a little vague. I can't really tell if this is even a supported use case without custom-building it myself.

jeremyckahn commented 7 years ago

Hi @philraj, thanks for opening this issue. shifty.queue.js was removed from the project quite a while ago, and I am not familiar enough with Webpack to give useful guidance here. Shifty should be amenable to adding that sort of functionality around it, but it is not built in to any modern versions of the library.

I will close this issue for now, as there is no change that needs to happen to the project, but I am happy to continue to discuss this here.

philraj commented 7 years ago

Yeah fair enough, it would be nice if the docs were updated to reflect that but I understand that time is precious and not always plentiful.

I ended up just making myself a promise wrapper to do some light sequencing. The only issue I'm having is I discovered a bug with the way Shifty checks for initial state before starting a tween.

Let's say if I create a tween by writing

const tweener = new Tweenable({ x: 5 });
tweener.tween({
  to: { x: 10 },
  step: (state) => console.log(state)
});

The step callback never receives the object I passed to the constructor as state. I traced this to line 353 of shifty.js, inside Tweenable#setConfig:

this._currentState = shallowCopy({}, config.from) || this.get();

Because shallowCopy always returns the targetObj even if it has no own properties, and the empty object is truthy, this line always assigns an empty object to this._currentState if no "from" was configured in a call to Tweenable#tween, shortcircuiting the call to this.get().

The same happens when calling tween on a Tweenable that already moved to a final state in a previous call.

I guess a fix would be to either return undefined from shallowCopy if no srcObj was passed in, or rewrite that line to check if the result of shallowCopy was an empty object. I avoided making a pull request because I wasn't sure I arrived at the best way to do it, but I can prepare one if you'd like.

Also, I can submit this as a separate issue if you want, I just didn't want spam the project.

jeremyckahn commented 7 years ago

Sorry for my late replies, I'm particularly busy this week, so I'm a little slow to respond to Github issues.

Thanks for finding and debugging this issue, @philraj! I'm surprised that this hasn't come up sooner. I believe your deduction is correct, and rather than change the behavior of shallowCopy, I would rather rewrite the line in setConfig — it's a safer fix.

This looks like an easy enough change, but I won't have time to properly fix this (i.e. with a unit test or two) until this weekend at the earliest. If you need this sooner, feel free to submit a PR and I will be sure to get it reviewed and merged in as soon as I am able to.

Please do open a new Github issue to track this bug, as it's unrelated to the original issue that was reported. Additionally, if you decide to make a PR, you'll need to make the change in shifty.core.js and run grunt build to get it into shifty.js.

Thanks offering to help out with this!


it would be nice if the docs were updated to reflect that

I'm a little unclear on what you are referencing here. Are you referring to how someone might build queuing functionality around Shifty? I feel that it's a little out of scope for library documentation; I prefer to keep that as concise and focused on core functionality as possible, rather than on domain-specific use cases. I'm not ruling it out, I just don't want to inundate new users who just want to get started with the project. However, if it can be kept short and simple, I'd be fine with creating an "Examples" section in the README to cover this.

philraj commented 7 years ago

Hi, thanks for your reply. I'll see if I can find some time to do a PR, but things just got a little busy for me too so no promises for now.

As for the docs... I think I just made a mistake! I could swear I read the line "shifty.queue.js is included in shifty.min.js for your convenience" somewhere in the docs or the source, but searching for it now all I can find is a result on searchcode.com that must be cached from ages ago. So scratch that comment.

My solution for queueing with a promise wrapper was simple enough, and I think you made Rekapi to solve that problem anyway, so I think you're right – no need to add anything related to that in the docs.

jeremyckahn commented 7 years ago

FYI @philraj: https://github.com/jeremyckahn/shifty/pull/91

(Please review!)

jeremyckahn commented 7 years ago

@philraj, have you had a chance to review #91?

philraj commented 7 years ago

Sorry, I had to switch to a different project and things have been pretty busy. I'll try to find some time later tonight. Looking at the code though, it seems like it'll work. I'll let you know.

philraj commented 7 years ago

Yep, seems to work now. Had an issue at first but I think it was webpack being tempermental. Thanks!

jeremyckahn commented 7 years ago

Merged. Thanks for reviewing!

This is now available in 1.5.4.