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

Webpack: Importing GSAP in multiple places returns empty object #261

Closed loilo closed 6 years ago

loilo commented 6 years ago

Bundling parts of GSAP (e.g. TweenLite.js) fails if done in multiple webpack entry points:

// in entry point A
import TweenLiteA from 'gsap/TweenLite'

// in entry point B
import TweenLiteB from 'gsap/TweenLite'

Now if you include both bundled entry points, A and B, in a HTML page, TweenLiteA will correctly point to the TweenLite function, however, TweenLiteB will be an empty object.

I made a minimal repro to make this issue easier to grasp, you can find it at Loilo/repro-webpack-gsap.

Note: While TweenLite is actually bundled in each a.js and b.js in the repro, the problem is the same when it's splitted out to a separate file and imported from there.

I'm not sure if this is an issue for the GreenSock or rather the webpack team — let me know if I can help with anything here. 🙂

jackdoyle commented 6 years ago

Howdy @Loilo. Sorry to hear about the hassle. And thanks for the very clear reduced test case! I wish everyone was as thoughtful as you when reporting a problem.

That sounds like a Webpack thing quite frankly. However, we're working on a more ES6-friendly release of GSAP which you can preview here: https://greensock.com/temp/GSAP-ES6.zip (this link will only be valid for the next week or two). When I swapped those files in for the ones in node_modules/gsap, it seemed to work great. Agreed? Notice any other issues?

loilo commented 6 years ago

Works like a charm. Thanks for taking the time and sharing this!

For other folks visiting this issue, I've just pushed a new branch on the repro repository with the gsap module replaced by the ESM approach mentioned above. You can find it at Loilo/repro-webpack-gsap#gsap-esm-preview.


Other issues... well, the _activate array in the index.js comes around as a bit strange to me, because I wouldn't expect an ES modules approach to work like this. If I explicitely import { TweenLite } from 'gsap', I'd expect to only get that into my bundle.

I get the rationale behind it and I'm sure you've got more insight into your users' expectations, but imo that kind of torpedos the ESM approach as a whole.

That being said, I'd love to have such an entry point from where I could just pick parts to use and have the rest being treeshaken (i.e. the current index.js without the _activate).

EDIT: Nevermind, I've overlooked some files and it seems that all.js is exactly what I was asking for. 👍


That little off-topic aside, I'm going to cc this to some webpack people to see what they're thinking.

jackdoyle commented 6 years ago

Glad it worked!

As for the whole _activate array thing, yes, it's a tricky situation because we work REALLY hard to maintain backward compatibility as much as possible, and without that _activate stuff in index.js, many people's projects may break due to the default tree shaking features in tools like Webpack. For example, if they did this:

import "gsap";
TweenMax.to(..., {x:100, backgroundColor: "red"});

It wouldn't work. CSSPlugin and most (if not all) of the other classes would be dumped. Not cool.

But yes, the new "gsap/all" is exactly the solution we wanted to deliver to folks like you. One place to import anything. Done.

And of course you can do things like:

import TweenLite from "gsap/TweenLite";

And that should keep things lean. The only other bummer (which I don't really know how to get around) is that plugins will get dumped by tree shaking unless you specifically reference them somewhere, like:

import { TweenLite, CSSPlugin } from "gsap/all";
TweenLite.to(element, 1, {backgroundColor: "red"}); //doh!

So you'll need to declare a variable or something, just to keep tree shaking from dumping it:

import { TweenLite, CSSPlugin } from "gsap/all";
const _activate = CSSPlugin;
TweenLite.to(element, 1, {backgroundColor: "red"}); //works

See what I mean?

Got any other ideas/suggestions? Does everything else work well for you?

loilo commented 6 years ago

Yeah, I guess that's mainly an architectural thing about GSAP, making plugins work just by appearance (i.e. by importing) is not easy in the ESM world.

In our project, I'm currently doing it like this:

import TweenLite from 'gsap/TweenLite'
// alternatively: import { TweenLite } from 'gsap/all'
import 'gsap/CSSPlugin'

I'm not sure if there's a reasonable way to make named imports of plugins work without explicitely having to activate them.

A quick thought for a quick fix (read further for my preferred solution): It would be possible to wrap each plugin in gsap/all in a function, so they are activated when called:

import { TweenLite, CSSPlugin } from 'gsap/all'
CSSPlugin() // Activate!

That would at least introduce a clear way to remove the const _activate hassle.


As this kind of has evolved into a general GSAP ESM thread: Another thing loosely coupled with the issue above is that people (which means me, I can only assume that for other devs) generally don't expect side effects from named ESM imports.

I had to learn the hard way that gsap/all still assigns global variables for the things you import from it. (That may not be a problem in general, but it conflicts silently with the webpack way of exposing global variables. Using that with GSAP will just break things with a similar outcome as in the original issue above, which really is a confusing footgun.)

But again, GSAP not assigning global variables would require an additional plugin activation API for ESM users (as any plugins from outside the gsap repository could no longer rely on globals and would need a new way to be attached to the framework)

And man, that'd probably be a lot of maintenance burden.


Generally, if I could just make any wish for a GSAP ESM API (what a pile of abbrevations 🙃), it would probably look a lot like the plugin activation interface of Vue.js:

// ESM
import { TweenLite, CSSPlugin, ScrollToPlugin } from 'gsap/esm' // No globals, no side effects
TweenLite.use(CSSPlugin, ScrollToPlugin) // Explicitely caused side effects

// Traditional
import 'gsap'
// Activates TweenLite, TweenMax and all the good stuff as usual
// Each plugins would call TweenLite.use(Plugin) implicitely
// -> no breaking changes
jackdoyle commented 6 years ago

Yeah, in its current iteration, I don't think it's feasible to have GSAP not declare any globals at all, but for 2.0.0 that should be very possible.

Your ideal solution looked like basically what we've got now, except you renamed "gsap/all" to "gsap/esm", right? And instead of TweenLite.use(...), I was just suggesting your own variable name like activate = [...] right? For what it's worth, you can currently call any plugin as a function (as you hoped), like CSSPlugin() and that should work to "activate" it as well. Or historically (from the Flash days), you could do TweenPlugin.activate([CSSPlugin, ...]) which is still valid. What's a little uncomfortable about TweenLite.use(...) is that it seems to imply that just TweenLite can use those, but when you activate a plugin it affects TweenMax, TimelineLite, and TimelineMax as well.

Any other suggestions? What do you think about the way everything is organized (and works) in the newer zip that I linked you to (which would be what we publish to NPM)?

loilo commented 6 years ago

Yep, I've renamed all to esm just as an example. Doesn't need to be exactly esm, just something pointing a little bit clearer towards the purpose of the file than all does. Throughout my comments, I'll stick to gsap/all to avoid confusion.

Your objections against TweenLite.use() are completely reasonable if there's shared instance between the Tween*/Timeline* items (didn't dig into the source that deep).

Exporting TweenPlugin sounds a bit verbose to me if its only user-facing purpose would be to activate plugins (correct me if there are other use cases). However, you could just expose an installPlugin(...plugins) function from gsap/all file (which would call TweenPlugin.activate(plugins) behind the scenes).

The need for such an interface is debatable. It's certainly more elegant than an activate array, but as mentioned above, it's mainly rooted in my general scepticism towards side effects in ES modules and preference for explicitness. (This also may or may not have to do with the fact that I still can't get to work webpack with GSAP in anything more complex than the repro repository. I haven't boiled this down to the essence yet, but I'm certainly attach this here when I'm done with that.)

Also in a world without GSAP globals, an activation interface would be necessary for third party plugins. (I'm not deep enough into the ecosystem to be sure if those even are a thing, but googling at least brought up some. So for this paragraph I'm assuming that third party plugins are a thing GSAP supports.) GSAP's very own plugins probably know where to find the objects to modify. However third party plugins don't — GSAP may be included from a CDN and only accessible via global variables. Or from the gsap/all npm package. If that's not clear, plugin activation needs to be explicit, and needs to provide the according Tween*/Timeline* instances to the plugin.


Any other suggestions? What do you think about the way everything is organized (and works) in the newer zip that I linked you to (which would be what we publish to NPM)?

I generally have no objections against the organization of the module, especially because I haven't compared it to the current version yet. 😁

I still have problems with how it works, as mentioned above, but I'll go into that in more detail in a later comment.

lorenzomigliorero commented 6 years ago

Webpack 4.6 with new webpack.NormalModuleReplacementPlugin(/\gsap\b/, resolve(paths.VENDORS, 'GSAP-ES6'))

import { TweenLite } from 'gsap' cause Cannot read property '_gsDefine' of undefined

Seems to be that _gsScope is undefined.

jackdoyle commented 6 years ago

@lorenzomigliorero are you saying that this problem exists in the "new" files from https://greensock.com/temp/GSAP-ES6.zip or the ones that are currently on NPM? (what version?). Do you have a reduced test case that illustrates the problem?

lorenzomigliorero commented 6 years ago

The problem exist with new files, i am preparing a reduced test case.

TheLarkInn commented 6 years ago

@lorenzomigliorero whats the case here for this one when you use NormalModuleReplacementPlugin. Is this to dogfood the newest version mentioned above? Or a internal usecase?

TheLarkInn commented 6 years ago
import { TweenLite, CSSPlugin, ScrollToPlugin } from 'gsap/esm' // No globals, no side effects
TweenLite.use(CSSPlugin, ScrollToPlugin) // Explicitely caused side effects

@Loilo @jackdoyle I think this is actually an excellent technique. One thing to also consider is having the plugin interface "use" also accept type () => Promise<Plugin>. Therefore users can also do:

const CSSPlugin = () => import("./gsap/esm/CSSPlugin"); // only grab that one module since dynamic imports don't treeshake yet

if (someConditionalCaseForUsingTheCSSPluginLazily) {
  TweenLite.use(
     CSSPlugin // now this plugin will get codesplit!
  )
}
lorenzomigliorero commented 6 years ago

@TheLarkInn It's an internal usecase used on a large codebase to match import replacing with:

loilo commented 6 years ago

Okay, although I wasn't able to reduce my case properly, I found the cause of GSAP not working anyway:

// TweenLite
export const _gsScope = (typeof(module) !== "undefined" && module.exports && typeof(global) !== "undefined") ? global : this || window;

This somehow kills my webpack build. It leads to errors like this:

vendor.[chunkhash].js:xxx Uncaught TypeError: Cannot set property Ease of #<Object> which has only a getter
    at Definition.check (vendor.[chunkhash].js:xxx)
    at new Definition (vendor.[chunkhash].js:xxx)
    at window._gsDefine (vendor.[chunkhash].js:xxx)
    at gs._class (vendor.[chunkhash].js:xxx)
    at vendor.[chunkhash].js:xxx
    at Object.<anonymous> (vendor.[chunkhash].js:xxx)
    at Object.5 (vendor.[chunkhash].js:xxx)
    at __webpack_require__ (manifest.[chunkhash].js:xxx)
    at Object.304 (vendor.[chunkhash].js:xxx)
    at __webpack_require__ (manifest.[chunkhash].js:xxx)

Replacing the _gsScope with

export const _gsScope = window;

fixed all my problems. 😁

EDIT: ...in webpack 3. Webpack 4 still brings some other errors I'm not sure about where they originated. I'm currently only guessing they're originating from the GSAP modules expecting this to equal the window object while, in module contexts, it's not.

loilo commented 6 years ago

Okay, I have determined the underlying problem:

In some cases, I included the bundle outputs of the various entry points (let's call them A and B) on the same HTML page.

In webpack 3 (via CommonsChunkPlugin), GSAP was extracted into a vendor chunk and included from there into chunks A and B — everything worked as usual.

However in webpack 4, the automatic splitting did not rate GSAP as a thing that should be extracted into its own chunk and thus, there were two copies of GSAP, one in A, and one in B.

That new webpack behaviour caused all kinds of problems, among others the GSAP errors described above.

I fixed these problems by refactoring our project to only use a single entry point that now conditionally import()s all the files we had as webpack entry points before.

In conclusion: This is an issue that could be fixed in a future, globals-free version of GSAP. For now, I solved my case and documented the cause for future passersby.

lorenzomigliorero commented 6 years ago

Thank you @Loilo. Have you already scheduled a npm update?

loilo commented 6 years ago

An npm update for what?

lorenzomigliorero commented 6 years ago

For compatibility with webpack4

loilo commented 6 years ago

Regarding GSAP? I have not used the official npm version but the ESM one @jackdoyle linked in his comment above.

loilo commented 6 years ago

However, it should not matter which version you use. In both cases, you'd need to deduplicate the GSAP code to only be executed once. However I'm not sure how to achieve that with webpack 4, as the CommonsChunkPlugin has been deprecated.

lorenzomigliorero commented 6 years ago

The thing that i don't understand is because with my configuration it works... I have enable splitChunks to cache only node_modules:

splitChunks: {
    cacheGroups: {
        vendors: {
            chunks: 'all',
            name: 'vendors',
            test: /[\\/]node_modules[\\/]/,
        }
    }
}

Than, i have imported gsap in two different entry points, in vendors.js:

import 'gsap';

and in my app.js:

import { TweenMax } from 'gsap'

And it works, i obtain TweenMax module.

"webpack": "^4.6.0",
"gsap": "^1.20.4"
loilo commented 6 years ago

Well then you probably wrote your splitChunks configuration better than I did. 🙈

I didn't manage to properly split out GSAP, but our setup is a little more complicated than just node_modules. We're running a thin wrapper around webpack, which means our splitChunks configuration is completely computer-generated and looks like /\Users\/dev\/gsap\.js|\/Users\/dev\/another-vendor-file\.js/.

It's very well possible that we messed up that configuration. 😉

jackdoyle commented 6 years ago

So to clarify, are you saying that the new ES6-ish version I linked to above worked well but only if you set _gsScope = window? (in the TweenLite.js file)

Any other issues that anyone sees? I'm a bit concerned about ripping out the whole global || this || window fallback routine because some people use GSAP in browserless configurations. So if I just set _gsScope = window, we may get a bunch of complaints about that. Open to suggestions.

jackdoyle commented 6 years ago

@Loilo I think I figured out what was causing that error in your setup. I've updated the files at https://greensock.com/temp/GSAP-ES6.zip. Better?

loilo commented 6 years ago

Almost works. :+1:

Another problem I had in the first ES6 package and I hadn't shared here yet:

// CSSPlugin.js 
var CSSPlugin = function() {
  TweenPlugin.call(this, "css");
  this._overwriteProps.length = 0;
  this.setRatio = CSSPlugin.prototype.setRatio; //speed optimization (avoid prototype lookup on this "hot" method)
}

In my build, this is undefined inside that function, which obviously breaks the code. I had to replace it with an arrow function (or use .bind(this)).

After applying that change to your ES6 (the original one as well as the latest build you just provided), it works like a charm.

Note 1: Since I've only used CSSPlugin and ScrollToPlugin, I don't know if this problem applies to some of the other plugins as well.

Note 2: Another thing I completely overlooked when filing this issue: The module goes through the babel-loader in webpack, which might be another potential source for errors.

jackdoyle commented 6 years ago

In my build, this is undefined inside that function

That's pretty weird! Is there any way you could send me a reduced test case that I can try on my end? It'd just be super helpful if I could reproduce that. I'm struggling to figure out why "this" would ever be undefined in that function/constructor.

loilo commented 6 years ago

Yeah, I'm not sure myself how this happens. I'll retry creating a reduced case, I guess it'll be worth it. 😁

loilo commented 6 years ago

Okay, got it. The error came up when I followed your suggestion:

For what it's worth, you can currently call any plugin as a function (as you hoped), like CSSPlugin() and that should work to "activate" it as well.

Following that advice, I did the following:

import { TweenLite, CSSPlugin } from 'gsap/all'
CSSPlugin()

And this is where the error happens:

Webpack extracts gsap/all into its own module. Consequently, the CSSPlugin() call from above is compiled to something like this:

Object(_gsap_esm_all__WEBPACK_IMPORTED_MODULE_0__["CSSPlugin"])()

The this context of the CSSPlugin function has not been bound, so when we call it, it will take the context defined by webpack.

The simplest repro can be found here.

jackdoyle commented 6 years ago

Aha! Sorry about that - my bad. I should have suggested:

new CSSPlugin()

The "new" makes all the difference (scope-wise).

Making that change solves things for you, right? I guess that's good news actually - I was worried there was a problem in the GSAP files themselves but it sounds like that's not the case. Please confirm and we'll be one step closer to releasing these files "officially" :)

And thanks for creating the reduced test case! It's so nice working with developers like yourself. I realize it takes a little extra time to put that together, but it is tremendously helpful.

loilo commented 6 years ago

My pleasure. It's the least I can do for being able to use such an amazing library. Also, there might be some extra sympathy when working with a brother in Christ. 😉

So, back to topic: Yes, new CSSPlugin() works. 👍 I had successfully tried that out before but wasn't sure if it was the intended way (since CSSPlugin() on a bound function and new CSSPlugin() actually make a semantic difference).

As far as I can tell, there should be no more issues stopping anyone from using the ES6 version. Problems still remain when somehow ending up with multiple copies of GSAP — due to the conflicting globals — but that hopefully is an edge case (which maybe should be documented somewhere). That might be unexpected for some users, but it's at least not a regression bug, so you're not breaking anything that worked before.

jackdoyle commented 6 years ago

Excellent, thanks for the feedback! [and high-five on the fellow-believer thing]

Can you explain what you mean by

"ending up with multiple copies of GSAP due to the conflicting globals"

What conflicting globals? How could that happen (how could I reproduce it)? Sorry if I'm missing something obvious.

loilo commented 6 years ago

Using the resulting bundles of at least two webpack entry points (where both are using GSAP) on the same HTML page can lead to this situation. It's what I explained in the comment above.

It makes me think that doing this at all is not intended by webpack, however I've not seen the docs discouraging it anywhere.

loilo commented 6 years ago

Hmm. I can no longer reproduce this. I'll go for another try, but maybe this problem somehow just resolved itself (or arose from poor judgement in the first place, we'll see).

EDIT: No, I can definitely no longer reproduce this. Might have stemmed from the _gsScope issue you fixed earlier. 👍

jackdoyle commented 6 years ago

Yay! Yeah, it may have either been that or the fact that it no longer tries to do AMD definitions on globals. Very glad to hear you can't replicate the issue anymore. Seems like we can push this new version out pretty confidently. Thanks again, @Loilo

jackdoyle commented 6 years ago

Things should be vastly improved in 1.20.5 (just released).

See https://greensock.com/docs/NPMUsage for a more official guide. Happy tweening!

loilo commented 6 years ago

Exciting to hear. Thanks for all the good work! 👍