greensock / GSAP

GSAP (GreenSock Animation Platform), a JavaScript animation library for the modern web
https://gsap.com
19.85k stars 1.72k forks source link

then() -> TypeError: f is not a function #351

Closed naranjamecanica closed 4 years ago

naranjamecanica commented 4 years ago

I'm working on a upgrade to V3 in a project and somehow I get random errors regarding the handling of promises.

image

I'm unable to pin why it's happening, since it's happing at multiple tween at random occurrences..

Maybe you can shed some light on it?

Thanks! ❤️

jackdoyle commented 4 years ago

Gosh, I have no idea but it kinda seems like an odd value is getting passed in as onFulfilled (not a function). If you can provide a reduced test case that shows the issue clearly, we'd love to have a peek. Nobody else has reported anything like that either. Are you using the latest version of GSAP?

naranjamecanica commented 4 years ago

I'll try to get the error in a smaller test environment, the problem is that I can't find a pattern yet when it's happening, because different tweens are giving the same error at different states moments.

jackdoyle commented 4 years ago

Well, even if it's just a super small example that sometimes acts up, that might work. We just need some way to recreate the issue on our end.

naranjamecanica commented 4 years ago

I've changed our wrapper methods, whey where using async structure so you could wait for the tween to finish (v2). I've removed that since promises are now build in and I don't see any error anymore. If it pops up again, i'll open this issue again. Thanks!

naranjamecanica commented 4 years ago

Hmm, I've started to encounter the same error again. Sadly I can not reproduce it in a CodePen. Digging deeper, it seems that _resolve() gets called a second time, and onFulfilled is defined but f() is undefined at that time.

This is because the variable f gets overwritten, changing the new resolved f to a local variable resolves all my issues, is this something you can implement safely?

then(onFulfilled) {
    return new Promise(resolve => {
        let f = onFulfilled || _passThrough,
            _resolve = () => {
                let _then = this.then;
                this.then = null; // temporarily null the then() method to avoid an infinite loop (see https://github.com/greensock/GSAP/issues/322)
                const rf = f(this);
                if (rf && (rf.then || rf === this)) {
                    this._prom = rf;
                    this.then = _then;
                }
                resolve(rf);
                this.then = _then;
            };
        if (this._initted && (this.totalProgress() === 1 && this._ts >= 0) || (!this._tTime && this._ts < 0)) {
            _resolve();
        } else {
            this._prom = _resolve;
        }
    });
}

Thanks again!

jackdoyle commented 4 years ago

I can add a check to see if onFulfilled is a function and if it's not, ignore it. From what you're saying, something is getting passed to then() as onFulfilled but it's not a function. Weird. Not sure what's going on in your app, but at least this check that I'll add in the next version should prevent the error from getting thrown.

naranjamecanica commented 4 years ago

onFulfilled is a function in both occurrences, so a check on that does not solve the problem.

The problem is this part: f = f(this);

The contents of let f is replaced by the return value of onFulfilled(), so a second time this value is not the same as the first time.

I'm using gsap in a tween mixin written TypeScript, so maybe there something happing in de magic translation with scopes...

jackdoyle commented 4 years ago

Hm, I can definitely lock the scope in that function so that it can't drift in the next release. Perhaps that'll solve your issue. Nobody else has reported anything similar so I'm not sure what's going on in your particular context but hopefully the next release will work around whatever it is. Should be out in the next few days.

jackdoyle commented 4 years ago

Please try the new version and let us know if that resolves things for you, @naranjamecanica.

naranjamecanica commented 4 years ago

Thanks for the update, I've upgraded and still the same errors and behaviour. Like I said, onFulFilled is always a function. The resolver gets called a second time and since the variable f gets overwritten the first time it's not the same a second time. Making a local variable like I said above from the result solves this error.

I also have other problems using await gsap.to();. It looks like this._prom() also gets called when a tween is overwritten? When I was using v2 I used the gsap-then library, which uses the onComplete callback.

For now i've resorted back to patching your then() function to something similar used in gsap-then:

gsap.core.Animation.prototype.then = function (onFulfilled) {
  var _this = this;

  const then = this.then;
  return new Promise(resolve => {
    const existing = this.eventCallback('onComplete');
    this.eventCallback('onComplete', function () {
      if (existing) {
        existing.apply(_this, arguments);
      } 

      // temporarily null the then() method to avoid an infinite loop (see https://github.com/greensock/GSAP/issues/322)
      _this.then = null;

      if (typeof onFulfilled === 'function') {
        onFulfilled(_this);
      }

      resolve();
      _this.then = then;
    });
  });
};
Ch1pStar commented 4 years ago

+1 for this issue. The problem only started occurring after updating to 3.1.0(from 3.0.1).

jackdoyle commented 4 years ago

Please try 3.1.1. We can't use the function you recommended because it doesn't handle some important scenarios properly, but I did tweak some of the code so that it can't run into that error anymore with f not being a function.

naranjamecanica commented 4 years ago

image

Just a quick response: sorry... still the same error, and I see that you still overwrite the f variable from a higher scope without checking so the next time resolve gets called. When I'm behind my computer with a bit more time I'll try to make a working version based on your code.

jackdoyle commented 4 years ago

Yeah, sorry about that - it makes things difficult when nobody can provide a reduced test case and I have no way of reproducing the issue, but I think this should work:

then(onFulfilled) {
    let self = this;
    return new Promise(resolve => {
        let f = _isFunction(onFulfilled) ? onFulfilled : _passThrough,
            _resolve = () => {
                let _then = self.then;
                self.then = null; // temporarily null the then() method to avoid an infinite loop (see https://github.com/greensock/GSAP/issues/322)
                _isFunction(f) && (f = f(self)) && (f.then || f === self) && (self.then = _then);
                resolve(f);
                self.then = _then;
            };
        if (self._initted && (self.totalProgress() === 1 && self._ts >= 0) || (!self._tTime && self._ts < 0)) {
            _resolve();
        } else {
            self._prom = _resolve;
        }
    });
}
killroy42 commented 4 years ago

It appears you're assuming onFulfilled returns a valid promise-like thing, which isn't a given. Perhaps it should be f = Promise.resolve(f(this));

jackdoyle commented 4 years ago

It appears you're assuming onFulfilled returns a valid promise

Hm, I don't think so. It could be a Promise-like object (with a "then()" method) or a function or really anything.

Were you saying that the latest function I provided didn't work properly for you, @killroy42 ?

killroy42 commented 4 years ago

No, sorry. Just that a promise function (the function passed to .then) could return any value, not just valid, promise-like objects. Those will usually be turned into promises inside the mechanisms, but if this code handles that directly, that might not happen. By passing the return value into Promise.resolve() it would turn a "bare" value into a promise. Just a common issue I've encountered in the past working with mixed code bases. Might be worth a try if there is still a problem.

For example: let onFulfilled = 'abc'; then: let f = onFulfilled || _passThrough; // f = 'abc' therefore: const rf = f(this); // Error, f is not a function

PS: Your modified code does the funcion checks. I was referring to the original source.

jackdoyle commented 4 years ago

Ah, okay. Thanks for clarifying @killroy42.

If anyone runs into trouble with the latest code I provided, please let me know.

lregla commented 4 years ago

Seeing the same thing in my implementation.

For now here (until I'm able to post an isolated test case) are the key constraints:

jackdoyle commented 4 years ago

I think part of the problem is that a Promise isn't supposed to be re-used. My understanding is that it either gets fulfilled or it fails...that's it. Are you expecting that then() will be called every time the playhead reaches the end of the timeline? Perhaps that's the root of the problem.

In most cases, I'd say people are better served by using simple callbacks like onComplete.

@lregla you haven't tried the latest code that I posted above, have you? (It's not in 3.1.1)

lregla commented 4 years ago

Going to try the above posted code next (I assumed incorrectly it was in v3.1.1)

I was able to reproduce the console error in this here sample: https://jsfiddle.net/lregla/1jufhL4q/82/ This is a very basic rendition of how I have the timelines implemented in my app.

As you can see I'm not setting any callbacks after the animation is complete. In fact, the animation ends up working well and my only issue is the error logged at the console.

jackdoyle commented 4 years ago

Should be fixed in the latest release (3.2.0)