UMD output does not work with require.js optimizer

[xi]

When using the UMD module output, the require.js optimizer is not able to trace dependencies. My first impulse was to fix it there (see requirejs/r.js#857), but their feedback was that it would probably be easier to change the output to something like this:

(function (factory) {
    if (typeof module === 'object' && typeof module.exports === 'object') {
        var v = factory(require, exports); if (v !== undefined) module.exports = v;
    }
    else if (typeof define === 'function' && define.amd) {
        define(['require', 'exports', 'lib'], factory);
    }
})(function (require, exports) {
    var lib = require('lib');
});

[danquirk]

cc @vladima

[vladima]

I think we can change our emit to requested form to be more aligned with standard idioms, i.e. this one. However I'd like to get a confirmation that this new pattern is correctly handled by require.js optimizer. @xi did you have chance to try it?

[xi]

I did not, but it was provided by the require.js optimizer author himself. So it should be fine.

The important aspect is really that the dependencies are define inside of the define() call because require.js optimizer operates on the AST.

[kitsonk]

Usually @jrburke will comment if you ping him. I also thought there were reasons why it wasn't exactly aligned, right @csnover because there was the debate about calling it UMD when it wasn't strictly UMD.

[jrburke]

I added this test to the r.js optimizer, it passes in the existing optimizer code, so the pattern suggested in the first message on this issue will work with the optimizer, and now that there is a test in there should continue to work in future releases:

requirejs/r.js@69f0651

If a different UMD variant is desired that you want tested with the optimizer, I can test it once you achieve a consensus on the style you want to use.

[csnover]

@vladima The signature where define comes first can’t be used unless you want the next ticket to be from a Browserify user complaining that Browserify doesn’t work. Moving the dependencies into the wrapper per the OP is a fine idea.

[csnover]

I’ll write a patch unless someone else is already on top of it.

[vladima]

@csnover sure, PR will definitely be appreciated!

[mhegazy]

thanks @csnover!