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

Warning about wrapShim with RequireJS #1574

Closed
philfreo opened this Issue Apr 1, 2014 · 6 comments

Comments

Projects
None yet
3 participants
@philfreo

philfreo commented Apr 1, 2014

I've got a pretty large project using many plugins for jquery, bootstrap, backbone, etc. Upon upgrading to the latest moment.js some non-AMD-compatible plugins that depended on moment (e.g. bootstrap-daterangepicker) stopped working -- only in the r.js optimized single-file build of the project.

The issue is due to how (later versions of) Moment don't also set a global window.moment if RequireJS is present. What moment.js is doing is pretty unique among browser RequireJS libraries (first I've encountered). jQuery still exposes the global even when RequireJS is used.

Even when you define the RequireJS shim properly, such as:
shim: { 'bootstrap-daterangepicker': ['jquery', 'moment'] } it still fails.

There is a RequireJS option wrapShim that fixes this behavior.

I bring this up only because it took me a long time to debug the issue (for what should have been a simple Moment.js version bump), so I'd recommend putting a warning in http://momentjs.com/docs/#/use-it/require-js/ like:

If you have other plugins that rely on Moment but aren't AMD-compatible, you may need to add wrapShim: true to your r.js config.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 2, 2014

I'm a bit confused. We just added the noGlobal option, but in theory a global object should still be exported.

define("moment", function (require, exports, module) {
    if (module.config && module.config() && module.config().noGlobal !== true) {
        // If user provided noGlobal, he is aware of global
        makeGlobal(module.config().noGlobal === undefined);
    }

    return moment;
});

If that is not the case then we have a bug.

@philfreo

This comment has been minimized.

philfreo commented Apr 2, 2014

That's why I was confused at first also! The global is exported... but too late!

The issue is that the RequireJS optimizer r.js (by design) concatenates all your modules into 1 file, in the order required based on dependencies. However, the actual execution of the code is a little different. The raw JS file will be parsed and the non-AMD plugins that rely on AMD plugins will throw an error.

define('moment', function() { ....... }); // the code inside here is invoked -- but not until the file has been parsed
(function() { /* bootstrap-daterangerpicker code here inside an IIFE gets run before any define modules */ }());
define('bootstrap-daterangepicker', function() { }); // this shim gets added after the actual code itself, letting other modules know this plugin is ready

with wrapShim it's something like:

define('moment', function() { ....... });
define('bootstrap-daterangepicker', function() { /* bootstrap-daterangepicker IIFE inside here */ });

The wrapShim option describes this situation perfectly:

Only use other "shim" modules as dependencies for shimmed scripts, or AMD libraries that have no dependencies and call define() after they also create a global (like jQuery or lodash). Otherwise, if you use an AMD module as a dependency for a shim config module, after a build, that AMD module may not be evaluated until after the shimmed code in the build executes, and an error will occur. The ultimate fix is to upgrade all the shimmed code to have optional AMD define() calls.
If it is not possible to upgrade the shimmed code to use AMD define() calls, as of RequireJS 2.1.11, the optimizer has a wrapShim build option that will try to automatically wrap the shimmed code in a define() for a build. This changes the scope of shimmed dependencies, so it is not guaranteed to always work, but, for example, for shimmed dependencies that depend on an AMD version of Backbone, it can be helpful.

So in summary, the design decision you've made is fine, but I suggest pointing out the wrapShim option in your RequireJS instructions. Or just do what jQuery does and leak the global (immediately, rather than upon module load) regardless. /cc @jrburke

@icambron

This comment has been minimized.

Member

icambron commented Apr 2, 2014

Leaking the global right away would prevent us from using the noGlobal flag and then a lot of purists will yell at us that we're polluting their global scope. So yeah, I think the doc fix is the best thing to do here.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 2, 2014

Such a mess... Lets fix the docs.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 4, 2014

moment/momentjs.com@914b39c

Lets also export the global in the beginning.

@icambron

This comment has been minimized.

Member

icambron commented Apr 4, 2014

Fixed by #1590.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment