greensock / GSAP

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

Incorrect require path in minified files. #180

Closed patrickbjohnson closed 7 years ago

patrickbjohnson commented 7 years ago

Currently using the npm GSAP library in a project and attempted to require the minified versions of various minified version of the files (plugins, easing, utils).

For a while none of the library were loading correctly and after poking around, realized some of these files reference TweenLite.js incorrectly.

Example from Draggable.min.js:

...require("../TweenLite.js"),require("../plugins/CSSPlugin.js")...

In this case TweenLite.js is not one level up from the minfied files. However, TweenLite.min.js is.

Quick screenshot to also show the directory structure from the npm module:

9de33a175ca199f3f02514af6c5e0272 _screen 20shot 202016-12-02 20at 209 40 26 20am

Suggestion:

I'm making an assumption, but I imagine the files are automatically compiled and the incorrect path references is happening within the compilation process.

With that being said, maybe it'd be helpful to require the correct type of the required file. Meaning, non-minified files require non-minified source (TweenLite.js, etc) and the minified files bet the minified source (TweenLite.min.js, etc).

jackdoyle commented 7 years ago

You're absolutely right - never really imagined that folks using NPM would tap into the minified files since they almost always have their own minification step in their build process. Plus the "main" file points to the uncompressed TweenMax. Hm. Let me chew on this a bit. I totally see your point about minified files requiring minified files.

Curious if anyone else has a need for this?

Also curious why you decided to tap into the minified stuff rather than uncompressed (not criticizing at all - genuinely curious)

patrickbjohnson commented 7 years ago

We usually tap into the minified version if provided under the assumptions it's been tested (I realize it can be an unsafe assumption depending on library). And that is mostly because aggressive minifiers (Google Closure Compiler) can have issues breaking code depending on the optimization levels set. Also A library as thorough as this one I'd probably use the minified version in most cases is to save some build time not compressing libs that already have a compressed version available.

We're more than happy to use the unminified version and compile ourselves though.

In terms of other issues, I did find this SO thread where it seems the person may have had the same issue and this one as well. However, I imagine if others have ran into it and tried the unminified version successfully they just used that and compiled on their own.

fregante commented 7 years ago

I don't think the issue is related to minification (although here minification made it visible) but rather that Draggable has raw require calls. This means that, whether you use min.js or not, this will actually bundle TweenMax and TweenLite plus CSSPlugin.js twice.

require('gsap/src/uncompressed/TweenMax');
require('gsap/src/uncompressed/utils/Draggable');

You can verify this:

# setup
npm install --global gzip-size browserify

echo "require('gsap/src/uncompressed/TweenLite');require('gsap/src/uncompressed/plugins/CSSPlugin.js');require('gsap/src/uncompressed/utils/Draggable');" | browserify - | gzip-size
# 99360 # TweenLite + CSSPlugin + Draggable

echo "require('gsap/src/uncompressed/utils/Draggable');" | browserify - | gzip-size
# 99335 # same size, still TweenLite + CSSPlugin + Draggable

echo "require('gsap/src/uncompressed/TweenMax')" | browserify - | gzip-size
# 96702 # TweenMax (includes TweenLite, CSSPlugin, …)

echo "require('gsap/src/uncompressed/TweenMax');require('gsap/src/uncompressed/utils/Draggable');" | browserify --full-paths - | gzip-size
# 191999 # TweenMax (includes TweenLite, CSSPlugin, …) + TweenLite + CSSPlugin + Draggable

This circles back to #170. In this case a simpler interim solution would be to drop the require calls inside Draggable and just rely on the globals like the rest of GSAP.

jackdoyle commented 7 years ago

I see what you mean but I worry that dumping the require calls would frustrate folks who just import Draggable (especially in existing projects which would likely break after implementing this suggestion). They'd have to know that they also have to load at least TweenLite and CSSPlugin.

I suppose an alternate solution would be to have it require TweenMax instead, but that increases the minimum footprint. In other words, if someone is just using Draggable (and doesn't need the extras in TweenMax), they couldn't get away with just loading TweenLite and CSSPlugin. Do you think this is preferable?

fregante commented 7 years ago

They'd have to know that they also have to load at least TweenLite and CSSPlugin.

Breaking existing setups is a rightful concern, but the docs do already say that you need to include those two plugins separately.

Having Draggable require TweenMax only shifts the problem because if I require TweenLite + Draggable I'm back to square one.

Not sure if there's any non-breaking solution. Probably you can just print an explanatory link to the console if GSAP detects duplicate/unnecessary imports.

fregante commented 7 years ago

A non-breaking fix for this issue would be to change the outlined lines from require("../TweenLite.js"),require("../plugins/CSSPlugin.js") to require("../TweenLite.min.js"),require("../plugins/CSSPlugin.min.js").

For v2 perhaps you can remove those requires entirely as suggested earlier

jackdoyle commented 7 years ago

This should be resolved in 1.19.1.