janpaepke / ScrollMagic

The javascript library for magical scroll interactions.
http://ScrollMagic.io
Other
14.9k stars 2.17k forks source link

AMD loading does not work with webpack #188

Closed aaronjensen closed 10 years ago

aaronjensen commented 10 years ago

I've not seen the particular pattern you're using, it doesn't follow UMD guidelines: https://github.com/umdjs/umd

It breaks on webpack (at least, I can't speak for browserify) because it looks for define on this || window. webpack defines define globally, but invokes the script in its own fresh context, so you won't find define there.

Normally I could work around this with the imports-loader, but scrollmagic is also looking for dependencies in this || window rather than just asking for them via bare word. The only way to load scrollmagic is to use the script-loader and that is bad because it necessitates jquery and GSAP being loaded in the same way.

One final problem is that scrollmagic defines two modules in one file. This doesn't work in webpack either. You could probably add something like this at the end:

define(['ScrollMagic', 'ScrollScene'], function (ScrollMagic, ScrollScene) {
  return {
    "ScrollMagic": ScrollMagic,
    "ScrollScene": ScrollScene
  };
});
janpaepke commented 10 years ago

Hi Aaron,

thanks for bringing this to my attention. I did write this after looking at the UMD design, but thought this would be a little bit better, because I would use a define method both for browser globals and require.js applications. Obviously I didn't know about the problems with webpack.

I don't really understand your solution though. With it there will be 3 defines in total. Is this on purpose?

I want to do this properly, not just a quick fix. The problem is that in require.js it is advised against to have two module definitions in one file. But to me it doesn't make any sense to split ScrollMagic and ScrollScene up, because ScrollScene is more of a sub class of ScrollMagic. One doesn't work without the other. It doesn't make any sense to load them individually. Also it would make the file structure more complicated, especially for users that use browser globals and wouldn't even need this.

So I'd really like to find a good structure this work properly for both – using AMD and browser globals.

Note: Dependencies will be completely removed in ScrollMagic 2.0 (currently in the development branch). The new file structure will be:

Plus a mirrored version with minified files.

If possible I would like to avoid providing an alternate version with the definitions split up into multiple files for AMD users. I would like to do this "the right way" so any advice is very welcome.

regards, J

aaronjensen commented 10 years ago

Hi, there actually is a slight problem with the 3rd define in that in a non-amd situation it'll define window[''] = { ScrollMagic, ScrollScene } which is a little goofy/intrusive.

If you want to keep them in one file that is fine, but you must return a single object, have one define/export. I'm not sure what requirejs does with multiple defines, webpack throws them out and just uses the last one.

If I was doing it from scratch, what I would do is set it up exactly like https://github.com/umdjs/umd/blob/master/amdWeb.js having factory return an object with both ScrollMagic and ScrollScene. In the non-amd clause, split that hash up and set those two on root.

I didn't want to do it that way because I wasn't familiar w/ other apm handlers. It sounds like though that'd probably work well for require.js too given what you said about them not liking multiple requires in one file.

One other option to consider is to change your API to not require two classes. Something like this:

var controller = new ScrollMagic();
controller.addScene({duration: 100}, function(scene) {
  scene.setPin("...");
});

addScene could be chainable and then you only have the one class.

aaronjensen commented 10 years ago

191 should work well, though you might consider my other suggestion for 2.0 of doing away with/hiding the ScrollScene class somehow, either behind a method like the proposed addScene above, or just piggyback it on ScrollMagic: new ScrollMagic.Scene or new controller.Scene. Of those, I prefer the addScene option, it feels the most javascripty imo.

aaronjensen commented 10 years ago

I should also say that ScrollMagic is awesome, great work! It's amazing how easy it makes things that would otherwise be very difficult.

janpaepke commented 10 years ago

Hi aaron, I understand your solution but it is not a good one. Removing the ScrollScene class or hiding it within ScrollMagic is more similar to how Superscrollorama behaved. The reason I split it up is so that you have a definite reference for each of them. This way you can do things like changing the scene's duration on the fly using scene.duration(200), which would not be possible if the ScrollScene class is not available.

I will think more about the best way to implement this.

thank you for the compliment - it's mostly magic. ;)

aaronjensen commented 10 years ago

Ah, in that case, addScene could still return the scene object. There's no need to expose the actual class, right?

janpaepke commented 10 years ago

there is. because scene are controller agnostic. You could remove a scene from a controller and add it to another one. There are many other reasons.

But anyway: I will have a closer look at how GSAP solved this, because they also have the TweenMax, TimelineMax, TweenLite etc. globals from one file and it does work with requirejs.

I will figure it out...

aaronjensen commented 10 years ago

I wouldn't look at GSAP, they're pretty broken in AMD world. They just global all the things even if you don't want it to.

You can just look at Backbone, in their case, Backbone is a namespace. So if you want two separate classes you'd just have ScrollMagic.Controller and ScrollMagic.Scene. That gives you one require and people could abbreviate when they require SM.Controller, SM.Scene.

janpaepke commented 10 years ago

That sounds like the way to go. I will look into it.

timkelty commented 10 years ago

@aaronjensen If I need to use this with WebPack now, do you suggest I use your fork, or is there another workaround?

aaronjensen commented 10 years ago

@timkelty https://github.com/janpaepke/ScrollMagic/pull/191 should be merged soon it sounds like, so you can use my fork for now. The other work around is to use the script loader, but that's bad news.

timkelty commented 10 years ago

Thanks @aaronjensen

janpaepke commented 10 years ago

All right guys. Version 1.3.0 is released, thanks @aaronjensen for your help with it. Please check here to see how to use ScrollMagic with AMD: https://github.com/janpaepke/ScrollMagic/wiki/Getting-Started-:-Using-AMD

janpaepke commented 10 years ago

BTW: let me know what you think of the new pattern. I am considering making ScrollMagic.Controller and ScrollMagic.Scene the default constructors for browser globals as well in ScrollMagic 2.0 for consistency reasons.

timkelty commented 10 years ago

1.3 is working great from me with Webpack, with one issue (more related to GSAP):

In webpack.config I have:

var aliases = {
  jquery: paths.vendorAbsolute + 'jquery/dist/jquery.js',
  TweenMax: paths.vendorAbsolute + 'gsap/src/uncompressed/TweenMax.js',
  CSSPlugin: paths.vendorAbsolute + 'gsap/src/uncompressed/plugins/CSSPlugin.js',
  TimelineMax: paths.vendorAbsolute + 'gsap/src/uncompressed/TimelineMax.js',
  EasePack: paths.vendorAbsolute + 'gsap/src/uncompressed/easing/EasePack.js',
  TweenLite: paths.vendorAbsolute + 'gsap/src/uncompressed/TweenLite.js'
};

This makes the dependencies of ScrollMagic load fine. However, if I try to TweenMax as a dep in my own module, it gives me something, but not TweenMax...

For example, this works (relying on ScrollMagic pulling in TweenMax dep):

/* global TweenMax */
define(['ScrollMagic'], function(ScrollMagic) {
...
          TweenMax.fromTo(...);
...

But if I do this, fromTo is not defined:

define(['ScrollMagic', 'TweenMax'], function(ScrollMagic, TweenMax) {
...
          console.log(TweenMax);
          TweenMax.fromTo(...);
...

Here, TweenMax is logged as:

function (vars) {
                    SimpleTimeline.call(this, vars);
                    this._labels = {};
                    this.autoRemoveChildren = (this.vars.autoRemoveChildren === true);
                    this.smoothChildTiming = (this.vars.smoothChildTiming === true);
                    this._sortChildren = true;
                    this._onUpdate = this.vars.onUpdate;
                    var v = this.vars,
                        val, p;
                    for (p in v) {
                        val = v[p];
                        if (_isArray(val)) if (val.join("").indexOf("{self}") !== -1) {
                            v[p] = this._swapSelfInParams(val);
                        }
                    }
                    if (_isArray(v.tweens)) {
                        this.add(v.tweens, 0, v.align, v.stagger);
                    }
                } 

Weird....?! @aaronjensen any ideas?

aaronjensen commented 10 years ago

I think this is a gsap problem. Their amd implementation seems to be problematic. We just do what works (your first example) v

Aaron

On Thu, Nov 13, 2014 at 7:29 AM, Tim Kelty notifications@github.com wrote:

1.3 is working great from me with Webpack, with one issue: In webpack.config I have:

var aliases = {
  jquery: paths.vendorAbsolute + 'jquery/dist/jquery.js',
  TweenMax: paths.vendorAbsolute + 'gsap/src/uncompressed/TweenMax.js',
  CSSPlugin: paths.vendorAbsolute + 'gsap/src/uncompressed/plugins/CSSPlugin.js',
  TimelineMax: paths.vendorAbsolute + 'gsap/src/uncompressed/TimelineMax.js',
  EasePack: paths.vendorAbsolute + 'gsap/src/uncompressed/easing/EasePack.js',
  TweenLite: paths.vendorAbsolute + 'gsap/src/uncompressed/TweenLite.js'
};

This makes the dependencies of ScrollMagic load fine. However, if I try to TweenMax as a dep in my own module, it gives me something, but not TweenMax... For example, this works (relying on ScrollMagic pulling in TweenMax dep):

/* global TweenMax */
define(['ScrollMagic'], function(ScrollMagic) {
...
          TweenMax.fromTo(...);
...

But if I do this, fromTo is not defined:

define(['ScrollMagic', 'TweenMax'], function(ScrollMagic, TweenMax) {
...
          console.log(TweenMax);
          TweenMax.fromTo(...);
...

Here, TweenMax is logged as:

function (vars) {
                  SimpleTimeline.call(this, vars);
                  this._labels = {};
                  this.autoRemoveChildren = (this.vars.autoRemoveChildren === true);
                  this.smoothChildTiming = (this.vars.smoothChildTiming === true);
                  this._sortChildren = true;
                  this._onUpdate = this.vars.onUpdate;
                  var v = this.vars,
                      val, p;
                  for (p in v) {
                      val = v[p];
                      if (_isArray(val)) if (val.join("").indexOf("{self}") !== -1) {
                          v[p] = this._swapSelfInParams(val);
                      }
                  }
                  if (_isArray(v.tweens)) {
                      this.add(v.tweens, 0, v.align, v.stagger);
                  }
              } 

Weird....?! @aaronjensen any ideas?

Reply to this email directly or view it on GitHub: https://github.com/janpaepke/ScrollMagic/issues/188#issuecomment-62907084

aaronjensen commented 10 years ago

@janpaepke cool, thanks. What do you think about changing the package name in package.json to scrollmagic (lowercase) and/or publishing to npm? Thanks!

timkelty commented 10 years ago

+1 for that, currently I just have it in my package.json like this:

    "ScrollMagic": "git://github.com/janpaepke/ScrollMagic.git#v1.3.0",
janpaepke commented 10 years ago

mh I don't know. ScrollMagic isn't really a node module. there are no node exports and its purely a frontend framework. I'll probably just keep it on bower and jquery plugins. My suggestion for you would be to add bower as a node dependency and then do this inside the scripts section of package.json:

"postinstall": "node node_modules/bower/bin/bower install"

I will rename the module to lowercase for 2.00

What do you think of my suggestion to switch the global constructors as described?

timkelty commented 10 years ago

We use NPM for everything (including frontend), eliminating the need for bower. I'd hedge my bets that this will become more common: http://blog.npmjs.org/post/101775448305/npm-and-front-end-packaging

If something doesn't have a package.json, we use https://github.com/shama/napa Works really nice!

aaronjensen commented 10 years ago

I second what @timkelty said (though I didn't know about napa, nice!) All of the larger libraries (and even some of the smaller libraries) are starting to show up on npm now. It's quite nice not to have to worry about having two package managers. GSAP is on npm, for example: https://www.npmjs.org/package/gsap

What do you think of my suggestion to switch the global constructors as described?

agree 100%.

janpaepke commented 10 years ago

well gsap does have a node module export. I don't because ScrollMagic makes no sense in a pure node environment.

But the article you posted does sound interesting and convincing. I will also consider this for ScrollMagic 2.0

aaronjensen commented 10 years ago

I don't know that you need a commonjs export. We are using it via npm right now without that and it's fine. 

Aaron

On Thu, Nov 13, 2014 at 9:01 AM, Jan Paepke notifications@github.com wrote:

well gsap does have a node module export. I don't because ScrollMagic makes no sense in a pure node environment. But the article you posted does sound interesting and convincing.

I will also consider this for ScrollMagic 2.0

Reply to this email directly or view it on GitHub: https://github.com/janpaepke/ScrollMagic/issues/188#issuecomment-62926467