Promise.all hangs

[olsonpm]

Your system information

• VelocityJS version: 2.0.2
• Browser:
  firefox 59.0.2 64-bit
  chrome 66.0.3359.139 64-bit
• Operating System: Arch linux

Checklist

• Is this an issue with code?: Yes
• Is this an issue with documentation?: No
• Have you reproduced this in other browsers?: only firefox and chrome, didn't try others
• Have you checked for updates?: Using latest 2.x
• Have you checked for similar open issues?: Yes

Please remember that this is an issue tracker, support requests should be posted on StackOverflow - see CONTRIBUTING.md

Please describe your issue in as much detail as possible:

Describe what you expected should happen and what did happen.

Expected: I expected to use Promise.all(arrayOfAnimations) without issue.
Actual: Browser freezes

Steps for reproducing this issue (code):

Here is a minimal jsfiddle for reproducing the bug

Below is the vue code I used to understand the issue a little better

1. https://codesandbox.io/s/0472r20wjw
2. click [Good Animate] to see the expected behavior
3. click [Buggy Animate] to freeze your browser

The difference between Good and Buggy is that Good calls then on each animation before passing it through Promise.all. I believe the issue has to do with the promise implementation - for some reason in 2.x the promises are really arrays which Promise.all doesn't play well with.

[Rycochet]

Ugh, not sure exactly what can be done about this one - it's literally impossible to have them as real Promises, but they copy the important bits across into the returns so that they can be used as Thenables.

This is going to need a bit of thought to handle - my initial one being "supply a Velocity.all([...anims])" method that can handle passing both real Promises and Velocity results.

Any ideas would be welcome :-)

[olsonpm]

Sorry I didn't look into the source - normally I would I was just having one of those days where things don't work.

Do you mind giving a brief explanation for why animate() can't return a real promise? Anyway I can put in some time tomorrow to browse alternative solutions.

[Rycochet]

It's supposed to return the real object that was passed to it (unless it's a single Element when it will return an array instead) - this is because the source object may be any number of types, from the result of an jQuery call, through a document.querySelectorAll etc. The prototype chain needs to be correct for this first, and then the Promise parts are injected on top of them - but only the .then, .catch, and .finally (if it exists). Each of these is copied with .bind(...) so they believe they're part of the original Promise when called (and that's where this is likely going wrong).

Another possibility would be making sure that all members of the Promise are copied (via .bind), but the more that's copied the more there's a possibility to break something. At the very least this needs tests added to ensure it works right when fixed.

[olsonpm]

That makes a lot of sense - thanks much. I'll definitely get back tomorrow after playing around with the code.

[olsonpm]

Well I think I figured it out, but I stopped when typescript started yelling at me

The diff

diff --git a/src/core.ts b/src/core.ts
index cd6c38e..c75b679 100644
--- a/src/core.ts
+++ b/src/core.ts
@@ -140,6 +140,7 @@ function VelocityFn(this: VelocityElements | void, ...__args: any[]): VelocityRe
 	}
 	// Create the promise if supported and wanted.
 	if (Promise && getValue(optionsMap && optionsMap.promise, defaults.promise)) {
+		let _then
 		promise = new Promise(function(_resolve, _reject) {
 			rejecter = _reject;
 			// IMPORTANT:
@@ -151,20 +152,24 @@ function VelocityFn(this: VelocityElements | void, ...__args: any[]): VelocityRe
 			// before the <code>.then()</code> function gets called.
 			resolver = function(args: VelocityResult) {
 				if (isVelocityResult(args)) {
-					const _then = args && args.then;
+					_then = args && args.then;
 
 					if (_then) {
 						args.then = undefined; // Preserving enumeration etc
 					}
 					_resolve(args);
-					if (_then) {
-						args.then = _then;
-					}
 				} else {
 					_resolve(args);
 				}
 			};
 		});
+
+		promise = promise.then((args: VelocityResult) => {
+			if (_then) {
+				args.then = _then;
+			}
+			return args
+		})
 		if (elements) {
 			defineProperty(elements, "then", promise.then.bind(promise));
 			defineProperty(elements, "catch", promise.catch.bind(promise));

I believe the issue is here - mainly the assumption that resolve is synchronous. Now granted I don't understand the issue fully because I only briefly looked at the promise spec and a couple implementations e.g. bluebird - but my understanding after playing around is that resolve is not necessarily synchronous so when we re-attach then onto our velocity object, its still behaving as though our object is a promise and never resolves.

My fix then is to immediately .then our internal promise and re-assign _then in the callback. This is still guaranteed to run before the consumer's callback and so should be functionally equivalent. It works in the couple tests I ran in chrome.

I wanted to run this change against the test-suite but didn't have the patience to tackle typescript.

It's worth noting my initial assumption that the velocity object being an array was the cause is false. From what I've read, Promise.all resolves its contents and Promise.resolve handles thenables so your approach is fine.

i.e. this works as expected

const myPromise = []
myPromise.then = resolve => {
  setTimeout(resolve, 500)
}

Promise.resolve(myPromise).then(() => {
  console.log('500ms later...')
})

[Rycochet]

If that's it then it's really cool - I'll have a look when I get some time (sadly that will be the weekend - one of the reasons things are stalling lol).

I will say that if you get the hang of Typescript and the typings then it's something you'd not want to go back to plain Javascript from - but this is one of those times when you'd be trying to leverage stuff and it's really not nice ;-)

[olsonpm]

When you take a look, here's the real minimal repro

And no rush on getting a fix out.

// test.js
let resolver

const fakeVelocityObject = []

const internalPromise = new Promise(_resolve => {
  resolver = velocityResult => {
    const _then = velocityResult.then
    velocityResult.then = undefined
    _resolve(velocityResult)
    velocityResult.then = _then
  }
})

fakeVelocityObject.then = internalPromise.then.bind(internalPromise)

Promise.resolve(fakeVelocityObject).then(() => {
  console.log('this never runs')
})

setTimeout(() => resolver(fakeVelocityObject), 500)

$ node test # hangs

[Rycochet]

The fix idea you came up with couldn't work - it broke one side or the other, so instead I've bypassed it entirely, and exposed a Velocity(...).promise object instead, so use that instead of the velocity animation result for anything like this.

[Rycochet]

The specific test used for this might be a little more obvious -

Promise
	.all([
		Velocity(getTarget(), defaultProperties, defaultOptions).promise,
		Velocity(getTarget(), defaultProperties, defaultOptions).promise,
	])
	.then(() => {
		assert.ok(true, "Promise.all fulfilled when all animations have finished.");
	})
	.then(done);

[Tarpsvo]

Hmm, same freezing happened to me (CPU task queue loop, 100% CPU usage indefinitely).

Code:

const _animateLineOut = () => lineElement
    .velocity({ width: '2px' }, { duration: 350, easing: 'ease-in-out' })
    .velocity({ width: '20px', height: '20px', borderRadius: '20px' }, { duration: 150, easing: 'ease-in-out'  })

const _animateAppIn = () => appElement
    .velocity({ opacity: 1 }, { duration: 250 })


_animateLineOut()
  .then(() => _animateAppIn())
  .then(() => console.info('complete'))

The final .then() is never reached, CPU usage for that tab is stuck at 100%.

Let it be said that I'm sorry if this was not constructive. I've only just started playing with velocity.js 2.

Also, have you any suggestions how to properly queue animations across multiple elements besides the given, currently mysteriously malfunctioning, promises method? The queue option is apparently element-specific. Thanks!

---

Minor update, workaround

Managed to work around this issue by wrapping the .velocity() calls in actual Promise objects.

const _animateLineOut = () => new Promise(resolve =>
  lineElement
      .velocity({ width: '2px' }, { duration: 350, easing: 'ease-in-out' })
      .velocity({ width: '20px', height: '20px', borderRadius: '20px' }, { duration: 150, easing: 'ease-in-out', complete: () => resolve() })
)

const _animateAppIn = () => new Promise(resolve =>
  appElement
    .velocity({ opacity: 1 }, { duration: 250, complete: () => resolve() })
)


_animateLineOut()
  .then(() => _animateAppIn())
  .then(() => console.info('complete'))

[Rycochet]

Your first piece of code there looks like it should be working (though does have an extra layer of arrow function that's not needed) -

const _animateLineOut = () => lineElement
    .velocity({ width: '2px' }, { duration: 350, easing: 'ease-in-out' })
    .velocity({ width: '20px', height: '20px', borderRadius: '20px' }, { duration: 150, easing: 'ease-in-out'  });

const _animateAppIn = () => appElement
    .velocity({ opacity: 1 }, { duration: 250 });

_animateLineOut()
  .then(_animateAppIn)
  .then(() => console.info('complete'));

There will be potential for multi-element sequences at some point in the not-to-distant future (ie, being able to synchronise several together) - I've just got to finish the spec for it - which would get rid f the need for promises in a simple situation like this.

I do just want to check that you're not using 2.0.4-beta as released, and if so then this should be opened as a new issue as that should really be working (the tests are still somewhat lacking sadly).