Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request for Better Type Definitions and Interfaces #533

Closed
roiiiu opened this issue Apr 17, 2023 · 10 comments
Closed

Request for Better Type Definitions and Interfaces #533

roiiiu opened this issue Apr 17, 2023 · 10 comments

Comments

@roiiiu
Copy link

roiiiu commented Apr 17, 2023

I have found gsap to be the most convenient animation library I have used so far. However, its TypeScript support leaves room for improvement. Due to the lack of precise type definitions, I often find myself having to refer to the official documentation more frequently than I would like.
For instance, I wonder if it would be beneficial to add literal property union types for the ease attribute, instead of relying on strings. This would not only make the code more self-explanatory and easier to maintain, but also boost productivity and confidence in designing complex animation sequences.
image
image
image

@jackdoyle
Copy link
Member

Thanks for the suggestion, @roiiiu

The ease must accommodate ANY string because you can create a CustomEase, CustomBounce, CustomWiggle, or even call gsap.registerEase() and assign an arbitrary string as the name. So do you think it'd still be helpful to do something like this?

type EaseName = "none"
            | "power1" | "power1.in" | "power1.out" | "power1.inOut"
            | "power2" | "power2.in" | "power2.out" | "power2.inOut"
            | "power3" | "power3.in" | "power3.out" | "power3.inOut"
            | "power4" | "power4.in" | "power4.out" | "power4.inOut"
            | "back" | "back.in" | "back.out" | "back.inOut"
            | "bounce" | "bounce.in" | "bounce.out" | "bounce.inOut"
            | "circ" | "circ.in" | "circ.out" | "circ.inOut"
            | "elastic" | "elastic.in" | "elastic.out" | "elastic.inOut"
            | "expo" | "expo.in" | "expo.out" | "expo.inOut"
            | "sine" | "sine.in" | "sine.out" | "sine.inOut"
            | "rough({template: none, strength: 1, points: 20, taper: none, randomize: true, clamp: false})" | "slow(0.7, 0.7, false)" | "steps(12)" | "expoScale(1, 2)";

And then:

ease?: EaseName | EaseFunction | string;

?

@roiiiu
Copy link
Author

roiiiu commented Apr 18, 2023

Thanks for the suggestion, @roiiiu

The ease must accommodate ANY string because you can create a CustomEase, CustomBounce, CustomWiggle, or even call gsap.registerEase() and assign an arbitrary string as the name. So do you think it'd still be helpful to do something like this?

type EaseName = "none"
            | "power1" | "power1.in" | "power1.out" | "power1.inOut"
            | "power2" | "power2.in" | "power2.out" | "power2.inOut"
            | "power3" | "power3.in" | "power3.out" | "power3.inOut"
            | "power4" | "power4.in" | "power4.out" | "power4.inOut"
            | "back" | "back.in" | "back.out" | "back.inOut"
            | "bounce" | "bounce.in" | "bounce.out" | "bounce.inOut"
            | "circ" | "circ.in" | "circ.out" | "circ.inOut"
            | "elastic" | "elastic.in" | "elastic.out" | "elastic.inOut"
            | "expo" | "expo.in" | "expo.out" | "expo.inOut"
            | "sine" | "sine.in" | "sine.out" | "sine.inOut"
            | "rough({template: none, strength: 1, points: 20, taper: none, randomize: true, clamp: false})" | "slow(0.7, 0.7, false)" | "steps(12)" | "expoScale(1, 2)";

And then:

ease?: EaseName | EaseFunction | string;

?

I am sorry for my limited knowledge about the library, but I still believe that using string to specify so many property variables is not the best practice. However, the cost of making changes may have some impact on the current users.😌

@jackdoyle
Copy link
Member

Oh, we will definitely keep the named strings for eases. In the very old days, we had ease objects for all the eases which technically made strict typing easier but it was clunky because it required more imports/exports (file size tradeoff) and it wasn't conducive to custom eases.

Plus there are over 11 million sites that rely on GSAP and tons of developers are very accustom to using the named strings for easing, so if we suddenly moved away from that, it would be extremely annoying for those devs and would cause breaking changes. I see absolutely no reason to do that, sorry.

However, if there's something we could do in the TypeScript definitions to make your life easier, I'm very open to that. I'm not much of a TypeScript person, so my question to you was more about whether or not that structure I shared above would actually be helpful at all (like for code hinting in your editor). In other words, if the ease accepts an EaseName as well as a string, does that give you the code hinting you want or is there no benefit whatsoever to having that EaseName type since we must allow a type of string anyway?

@charnog
Copy link

charnog commented Apr 18, 2023

For the official playground and VSCode, it doesn't make any difference. A-la 'asdf' and 'zxcv' are subtypes of string.
image
image

WebStorm understands it, and gives the hint (they have their own tuned implementation of TS language server):
image
image

I would say "yes, we need it" if it were consistent across all implementations, especially in VSCode, as many folks use it. I prefer to leave it as a string, at least for now.

By the way, there are a few things to remember; try to read them as one word:
"powerX" and "back" and "bounce", "circ", "elastic", "expo", "sine".

Then you can add . and change the type via "in" or "out" or "inOut".

@jackdoyle
Copy link
Member

I would say "yes, we need it" if it were consistent across all implementations, especially in VSCode, as many folks use it. I prefer to leave it as a string, at least for now.

I read that a few times and am still fuzzy. You say that my proposed change DOES help in some editors like WebStorm, right? So "yes we need it"...and then you quickly follow that with "I prefer to leave it as a string" which sounds like "no, we don't need it...stick with a generic string as the type". Can you please clarify what you meant?

If there's benefit for some (and it doesn't hurt anyone), I'm inclined to do it.

By the way, there are a few things to remember; try to read them as one word: "powerX" and "back" and "bounce", "circ", "elastic", "expo", "sine".

Then you can add . and change the type via "in" or "out" or "inOut".

Again, this left me confused. Can you rephrase and make your suggestion very clear please? Sorry if I'm missing something obvious.

@charnog
Copy link

charnog commented Apr 18, 2023

Oops, my bad.

The message has the following structure:

  1. The first part shows where it works and where it doesn't (without implying whether it should be implemented or not).
  2. The second part presents my opinion on whether it should be implemented or not

If there's benefit for some (and it doesn't hurt anyone), I'm inclined to do it.

If so then I suggest to use Template Literal Types

image

You can also inline Dir to template, if you don't want to create a distinct type for it.

type Ease = "none" | EaseName | `${EaseName}.${"in" | "out" | "inOut"}` | string;

@jackdoyle
Copy link
Member

Thanks for the suggestion, @charnog

Do you know of any compatibility issues with using the template literal types? Should this be pretty universally compatible these days?:

  type EaseName = "power1" | "power2" | "power3" | "power4" | "back" | "bounce" | "circ" | "elastic" | "expo" | "sine";

  type EaseDirection = "in" | "out" | "inOut";

  type EaseString = "none" | "rough({template: none, strength: 1, points: 20, taper: none, randomize: true, clamp: false})" | "slow(0.7, 0.7, false)" | "steps(12)" | "expoScale(1, 2)" | EaseName | `${EaseName}.${EaseDirection}` | string;
ease?: EaseString | EaseFunction;

From a quick glance, they were added in TypeScript 4.1 in late 2020. I don't have a good feel for how dangerous it would be to add into our codebase (I don't want to cause problems in legacy projects for example).

@charnog
Copy link

charnog commented Apr 20, 2023

I don't have a good feel for how dangerous it would be to add into our codebase (I don't want to cause problems in legacy projects for example).

If so, then yes, you are right, it may cause problems, and it's better to use a 'plain old string union' as mentioned in this message.

In that case, there are no concerns about 'broken types'. Previously, the type was just string. If you update the type to be more specific but still include string in the type definition, then nothing will actually change. Users who used 'myCustomEaseName' will still rely on a string within the updated type. WebStorm will show the suggestions for the new types (I don't know why VSCode doesn't).

By the way, I noticed that you wrote "rough({template: none, strength: 1, points: 20, taper: none, randomize: true, clamp: false})" | "slow(0.7, 0.7, false)" | "steps(12)" | "expoScale(1, 2)". Unfortunately, it won't be interpolated, and users will have to use it exactly as you wrote in the type. You can simply removed it, string part of the updated type will allow users to write whatever they want, but more specific 'power1.in' ... will allow IDEs to suggest the simple ones.
image

It would be possible with template literals though, like this: slow(${number}, ${number}, ${boolean})
image

@jackdoyle
Copy link
Member

That's very helpful, thanks.

My goal with adding the "rough(...)", "slow(...)", etc. was to just help people who can't quite remember the syntax/parameters. I figured they'd just select it from the code hint suggestions and then go back in and edit whatever parts of the string they need to. Since it's still a string, it wouldn't get flagged as invalid. But am I wrong about that? Would users be forced to use that rough(...) string EXACTLY as it's defined in the type?

@charnog
Copy link

charnog commented Apr 20, 2023

That's very helpful, thanks.

You're welcome!

I figured they'd just select it from the code hint suggestions and then go back in and edit whatever parts of the string they need to.

That's an interesting approach; I've never thought about it that way. In my opinion, the type system isn't quite the right place to store the hints. I would suggest implementing official snippets for VSCode, WebStorm (IDEA), or another IDE with a large user base. Click, click, click, click.

Would users be forced to use that rough(...) string EXACTLY as it's defined in the type?

No, they would not. You're right in saying, "Since it's still a string, it wouldn't get flagged as invalid."
image
image

jackdoyle added a commit that referenced this issue Jun 6, 2023
- NEW: Ever had ScrollTriggered animations at the very top of your page start out partially-scrubbed? Now you can clamp() them! Wrap your ScrollTrigger start/end values with "clamp()" to prevent them from "leaking" outside the bounds of the page. They'll never start before a scroll position of 0, nor will they finish after the maximum scroll. Clamped ScrollTriggers make all your "above the fold" elements start off in their native positions.

- NEW: For ScrollSmoother, you can now wrap your data-speed value in "clamp()" to make the associated element start out in its natural position if it's within the viewport at the top of the page (meaning the resulting ScrollTrigger would otherwise have a negative start value), and it also will have the same effect if the element is at the very bottom of the page and the associated effect's ScrollTrigger would normally have an end value that exceeds the maximum scroll position. See https://greensock.com/forums/topic/36177-scrollsmoother-data-speed-and-firstlast-folds/

- NEW: ScrollSmoother officially recognizes a new "speed" property that you can use to make the page scroll faster or slower.

- IMPROVED: TypeScript string-based eases added to improve code hinting in some editors. See #533

- IMPROVED: if you set dragClickables: false in a React app, since React adds an onclick handler on the root, things wouldn't be draggable. So having an onclick handler no longer causes something to be deemed "clickable". You can always use the clickableTest function if you'd like to run your own logic. See https://greensock.com/forums/topic/36854-using-a-handle-touchpoint-to-manipulate-a-draggable-object-allowing-for-form-interaction/

- IMPROVED: PixiPlugin now recognizes the new location for filters in Pixi version 7.1 and later. See https://greensock.com/forums/topic/36997-pixi-plugin-deprecation-v7/

- FIXED: regression in 3.11.5 could cause a motionPath tween to throw an error if the target is not a DOM element and you're animating transform-related properties like "x" or "y". See #531

- FIXED: if a Flip-related tween is reverted when it is paused, it wouldn't clear out the inline styles properly. Related to this thread in the forums: https://greensock.com/forums/topic/36259-flip-resize/

- FIXED: ScrollSmoother might appear to jump slightly on the very first scroll in a specific situation. See https://greensock.com/forums/topic/35244-there-is-a-lag-with-page-scrolling-while-using-scrollsmoother/

- FIXED: Some frameworks like Vue make the value of Refs into Proxies, so when you store a gsap.context() or gsap.matchMedia() in a Ref and then revert() it (perhaps onUnmount()), the Proxy that the framework imposed interfered with GSAP's ability to match up the original object with the Proxy, so a matchMedia() may not get properly killed in some edge cases. See https://greensock.com/forums/topic/36313-use-matchmedia-with-scrolltrigger-pin-in-nuxt3-will-get-error-after-breakpoint-change/

- FIXED: if you create a timeline inside a gsap.context() with a callback like onComplete it may still get called after revert() is called on the context. Timelines are now properly killed in the context.revert() call. See https://greensock.com/forums/topic/35365-react-and-gsap-timelines-callbacks/

- FIXED: if you disable() a Draggable that's in the middle of an inertia animation, it'll kill() that animation.

- FIXED: in 3.11.5 if you set overflow-y: scroll on the <body> and you used ScrollTrigger to pin something on a page that natively doesn't have any scroll, it might horizontally size things slightly wrong (as if there was an extra scrollbar's worth of space).

- FIXED: if you set a "speed" on a ScrollSmoother and then the content resizes when the page is scrolled all the way to the bottom, it may jump the scroll position incorrectly. See https://greensock.com/forums/topic/36602-scrollsmoother-with-accordion-animation-makes-page-jump/

- FIXED: if you create a staggered tween inside a gsap.context() and then kill()/revert() the context, it could throw an error. See #536

- FIXED: if you use selector text for an endTrigger of a ScrollTrigger that was created inside of a scoped gsap.context(), it may not honor that scope. See #537

- FIXED: if you initiate a ScrollTriggered from() animation that has a stagger BEFORE ScrollSmoother is created, it may render incorrectly initially. See https://greensock.com/forums/topic/36777-scrollsmoother-splittext-nextjs/ and https://codepen.io/GreenSock/pen/eYPyPpd?editors=0010

- FIXED: if you create a ScrollSmoother AFTER already creating ScrollTriggers, and you revert() and then re-create the ScrollSmoother (sometimes happens in environments like Next.js), it may revert() those pre-existing ScrollTriggers even if they weren't created inside the context in which the ScrollSmoother was created. See https://greensock.com/forums/topic/36777-scrollsmoother-splittext-nextjs/

- FIXED: if you apply a ScrollTrigger to an element that also has data-speed and ScrollSmoother is controlling it, the end scroll position may be miscalculated. See https://greensock.com/forums/topic/36930-scrollsmoother-with-data-speed-causes-wrong-scrolltrigger-positions/

- FIXED: if you create a gsap.context() or gsap.matchMedia() inside a scoped gsap.context() (nested), it wouldn't honor the outer scope as the default.

- FIXED: a regression in 3.11.x could cause timeline event callbacks and onEnter/onLeave ScrollTrigger callbacks not to fire properly on a ScrollTrigger-scrubbed timeline when the page is reloaded after scrolling down (scroll restoration). See #538

- FIXED: if you use data-speed="auto" on an element with ScrollSmoother, a very specific aspect ratio could cause the element to move in the wrong direction. See https://greensock.com/forums/topic/37185-issues-with-scrollsmoother-for-images-past-a-certain-width/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants