Kris-B / nanoGALLERY

image gallery simplified - jQuery plugin. Touch enabled, responsive, justified/cascading/grid layout and it supports pulling in Flickr, Google Photos and self hosted images.
https://nanogallery2.nanostudio.org/
438 stars 101 forks source link

NGTweenable undefined when loading in a page using AMD #78

Closed jefftmills closed 8 years ago

jefftmills commented 9 years ago

I encountered this problem while upgrading from 5.0.2 to 5.6.0. Because we use requirejs to load our scripts, the embedded "shifty" code detects the presence of "define" and "define.amd" and then also uses the define function to register itself as an AMD module instead of setting a global variable. However, nanoGallery assumes that the global variable is defined and therefore encounters fatal errors.

I found a couple of ways to modify jquery.nanogallery.js to resolve this.

The simpler way was to modify the embedded shifty code to remove the AMD detection and to always set the global variable.

The slightly more involved way was to add an AMD detection wrapper around the nanoGallery initialization (about 6 lines of code), modify the shifty define() call to add a module name parameter (so the wrapper could reference it as a dependency), and move the shifty code to the top of the file.

Kris-B commented 9 years ago

Ok, I'll check this.

Kris-B commented 9 years ago

Can you please check v5.7.0?

jefftmills commented 9 years ago

Yes, 5.7.0 fixes this. Thanks.

jefftmills commented 9 years ago

It looks like I replied too soon. v5.7.0 works when using requirejs to load modules separately (such as in our dev environment), but not after using the requirejs optimizer (http://requirejs.org/docs/optimization.html) to bundle dependencies into a single script (which is done for our production environment).

The problem now is that ngImagesLoaded becomes the entry point for the top-level script on our page. Imagine we have a script example.js that defines a class ExampleClass which requires (loads via requirejs AMD) several other modules including nanoGallery. After bundling all of the dependencies into a single script and loading that script into the page, calling "new ExampleClass()" enters ngImagesLoaded instead of the actual ExampleClass code.

Since I originally reported this, we've been running with a version of v5.6.0 that I modified to apply the "slightly more involved way" that I originally mentioned, so we'll keep running with that. I'm happy to send it to you if you're interested. OTOH, I understand if maintaining more AMD compatibility isn't a goal of this project and we can just re-apply our customization each time we have a need to upgrade to a new version.

Kris-B commented 9 years ago

Yes, I'm interested in your solution!

jefftmills commented 9 years ago

OK, I created a fork at https://github.com/trustradius/nanoGALLERY and re-applied my changes to v5.7.0 and tested it with our development environment and a production-like build. I don't know whether this will help with any other AMD loaders than requirejs because that's the only one we use.

Again, the steps can be summarized as:

  1. modify the shifty define() call to add a module name parameter (so the wrapper can reference it as a dependency)
  2. move the shifty code to the top of the file
  3. add an AMD detection wrapper around the nanoGallery initialization
Kris-B commented 9 years ago

@jefftmills your proposition has been integrated in v5.8.0 thanks

dragomireckijj commented 9 years ago

I was unable to load minified version of nanogallery script with require.js (dependency 'ngTweenable' was trying to load */js/libs/ngTweenable.js which is not exist). After several fails (require.config manipulations, standalone Shifty module) I found easy solution: remove 'ngTweenable' from dependency list of 'jqueryNanoGallery' module definition and constructor function parameters to force nanoGallery use global NGTweenable instance instead of 'ngTweenable' AMD module.

Here is my config:

requirejs.config({
    baseUrl: baseuri +'/js/lib',
    paths: {
        jquery: 'jquery/jquery.min',
        jqueryNanoGallery: 'nanogallery/jquery.nanogallery'
    }
});

And here is how its loading:

require(['jqueryNanoGallery'],function() {
     $('#nanogallery').nanoGallery({
          ... 
     });
});

What I was doing wrong?

jefftmills commented 9 years ago

I hadn't made time to check out the 5.8.0 release until now. Unfortunately, I'm also getting the same error. It looks like perhaps the modifications I made to the embedded shifty in my fork did not get merged in. See line 58 at https://github.com/trustradius/nanoGALLERY/blob/master/jquery.nanogallery.js.

The relevant portion of that line is where the previous

module.exports=h:"function"==typeof define&&define.amdDISABLED?define(function

was changed to

module.exports=h:"function"==typeof define&&define.amd?define('ngTweenable', function

After applying this one change to 5.8.0 it works for me.

dragomireckijj commented 9 years ago

Thank you!

Kris-B commented 8 years ago

Sorry for the late reply... I made some corrections in v5.9.0 Hope it will now work as expected.

jefftmills commented 8 years ago

v5.9.0 works for me in both our development (requirejs loading individual script files) and production (using requirejs optimizer to bundle dependencies into a single script file) environments. Thanks.

Kris-B commented 8 years ago

@jefftmills thanks to you!