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

This line of code has possibility to cause a `RangeError: Maximum call stack size exceeded at String.match ()` #545

Closed Paper-Folding closed 1 year ago

Paper-Folding commented 1 year ago

Here is the final location that caused the error:
https://github.com/greensock/GSAP/blob/472d4170d4aa3cceb8a3880d7cad700f22fbc545/src/CSSPlugin.js#L322

I met a RangeError: Maximum call stack size exceeded on my production site, this issue does not happen very often, but it happened indeed.

I must apologize that I can't provide a very detailed issue report now (because I can't locate where it is actual called in my code under a production mode), now I unminimized my code and add source map, once this issue happens again, I'll report more details here.

Paper-Folding commented 1 year ago

Okay, the call stack(I analyzed with https://github.com/sokra/source-map-visualization):
https://github.com/greensock/GSAP/blob/472d4170d4aa3cceb8a3880d7cad700f22fbc545/src/CSSPlugin.js#L322 https://github.com/greensock/GSAP/blob/472d4170d4aa3cceb8a3880d7cad700f22fbc545/src/CSSPlugin.js#L1095 https://github.com/greensock/GSAP/blob/472d4170d4aa3cceb8a3880d7cad700f22fbc545/src/gsap-core.js#L2204

Paper-Folding commented 1 year ago

And it's so weird that the issue happens in CSSPlugin.js, I did not even explicitly import CSSPlugin.js in my source file, I only use:

import gsap from "gsap";
import Draggable from "gsap/Draggable";

in my source file, will this import CSSPlugin, too? If so, can I prevent using CSSPlugin?

jackdoyle commented 1 year ago

Hm, that sounds odd. Can you please provide any (or all) of the following?:

  1. A minimal demo illustrating the issue
  2. The exact error message. Was it only "RangeError: Maximum call stack size exceeded at String.match()"? Any other clues?
  3. The value(s) you're feeding into the tween that's triggering that error. I wonder if it's a MASSIVE string with hundreds of thousands of numbers or something.

I just can't imagine why that line would throw the error. Nobody else has reported anything similar. I'd love to see a demo, though.

jackdoyle commented 1 year ago

And to answer your question about CSSPlugin, that's automatically included in the core because almost everybody ends up wanting to tween CSS-related values. If you truly have no such needs at all in your app, yes, you technically should be able to import gsap from "gsap/gsap-core" which would exclude CSSPlugin (but again, that's almost never a good idea). The fact that you're getting the error in that CSSPlugin file tells me that you actually are trying to animate something on a DOM element, likely CSS-related, so you shouldn't be excluding CSSPlugin.

Paper-Folding commented 1 year ago

I finally resolved this error, this error happens because my Draggable.js is using a low version cdn while gsap is the newest high version in my production site (using cdn, so I forget to update url for both of them). In development mode, I'm using npm so the version is always the latest, so that's way I can't catch a error in development mode.

jackdoyle commented 1 year ago

Huh, very interesting. I still don't understand why having an old version of Draggable would cause that line to error in CSSPlugin. Are you sure things are resolved? I mean you should definitely be using a more recent version...I just want to make sure the real issue was resolved.

Paper-Folding commented 1 year ago

Huh, very interesting. I still don't understand why having an old version of Draggable would cause that line to error in CSSPlugin. Are you sure things are resolved? I mean you should definitely be using a more recent version...I just want to make sure the real issue was resolved.

Yes, it is resolved. From my user who faced this problem yesterday.

jackdoyle commented 1 year ago

For the record, there was a regression in 3.12.0 that could cause an error like this (not from that line you mentioned though) if you create a Draggable, enable inertia, and then call disable() on that Draggable instance. It should be resolved in the next release, 3.12.2 due out soon. I just wonder if maybe that was the root cause here.

jackdoyle commented 1 year ago

@Paper-Folding can you share any details about what exactly resolved it on your end? I don't think this had anything to do with GSAP specifically, but I'm curious about what solved the problem for you.

Paper-Folding commented 1 year ago

@Paper-Folding can you share any details about what exactly resolved it on your end? I don't think this had anything to do with GSAP specifically, but I'm curious about what solved the problem for you.

Well, if you insist.... Intertia plugin 3.11 does not compatible with Draggable 3.12 (especially when .kill method is called on Draggable instance). That's all I have tested. And I really appreciate your work, I must apologize for being so vague above.

Paper-Folding commented 1 year ago

And I resolved the problem by removing Inertia Plugin, users weren't actually allergic to drag inertia a lot, they only care dragging.

jackdoyle commented 1 year ago

Yes, that issue was resolved in 3.12.2 of Draggable as I mentioned above. If you become aware of any other issues, please do let us know.