umdjs / umd

UMD (Universal Module Definition) patterns for JavaScript modules that work everywhere.
MIT License
7.42k stars 423 forks source link

templates/jqueryPlugin.js doesn't seem to conform to CommonJS spec #87

Open malthejorgensen opened 7 years ago

malthejorgensen commented 7 years ago

Not sure if I'm looking at the right CommonJS spec (the commonjs.org-site is a bit confusing), but in the current jQuery plugin template (jqueryPlugin.js, line 7) module.exports is used instead of the CommonJS standard exports.

You can compare this with commonjsStrict, line 23, which does check and use the exports-object – not module.exports.

I think jqueryPlugin.js should be changed to use the exports-object.

Note: Both files were checked at commit 95563fd6b46f06bda0af143ff67292e7f6ede6b7 (latest master as of 23rd Dec 2016)

kottenator commented 7 years ago

@malthejorgensen - no, it's fine, there are both module.exports and exports exist in CommonJS. The difference is:

Real problem with UMD for jQuery plugin and CommonJS

There is different problem with CommonJS and current jqueryPlugin.js implementation:

if (typeof module === 'object' && module.exports) {
  // Node/CommonJS
  module.exports = function(root, jQuery) {
    // ...
  };
}

So when you do var plugin = require('my-plugin') - it's actually a factory function which you need to call to initialize the plugin:

plugin(); // or ...
plugin(window); // or ...
plugin(window, $); // or ...
plugin(null, $);

Looks complicated. I don't understand - why not to do simply:

if (typeof module === 'object' && module.exports) {
  // Node/CommonJS
  var jQuery = require('jquery');
  factory(jQuery);
  module.exports = jQuery;
}

But the problem described above is not a bug - just do require('my-plugin')() instead of require('my-plugin').

malthejorgensen commented 7 years ago

Hi @kottenator

By that logic CommonJS and Node.js-modules are the same – I don't think that is true – have you read the CommonJS spec – it doesn't mention module.exports?

I think the reality is that Node.js-modules added module.exports for convenience, and uses that as the "single source of truth", and then has the exports variable reference module.exports. This makes any CommonJS module work in Node.js – but not the other way around. A Node.js module could use only module.exports but that's not compatible with the CommonJS spec, which only takes into account the exports variable.

Maybe the "CommonJS" label should be removed (and replaced with "Node.js-compatible") from the UMD templates?

kottenator commented 7 years ago

Agree, module.exports is not by CommonJS spec, you're right. But that's how it works in Node.js and I don't know other way to do default export in ES5.

I don't think it's a problem to use module.exports in UMD. If they'd use exports instead - it'll complicate the plugin import: require('my-plugin').init() (something like that).

Currently it is:

// my-plugin.js
module.exports = function(root, jQuery) {
  // ...
  factory(jQuery);
  return jQuery;
}

// your ES5 code
require('my-plugin')();

// your ES6 code
import plugin from 'my-plugin';
plugin();

With your idea it'd become:

// my-plugin.js
exports.init = function(root, jQuery) {
  // ...
  factory(jQuery);
  return jQuery;
}

// your ES5 code
require('my-plugin').init();

// your ES6 code
import {init} from 'my-plugin';
init();

What I suggest is:

// my-plugin.js
var jQuery = require('jquery');
factory(jQuery);
module.exports = jQuery; // actually, it's not necessary to export anything

// your ES5 code
require('my-plugin');

// your ES6 code
import 'my-plugin'; // or import $ from 'my-plugin';
shelldandy commented 7 years ago

Hey guys I faced that problem yesterday what I did is this:

;(function (factory) {
  if (typeof define === 'function' && define.amd) {
    define(['jquery'], factory)
  } else if (typeof exports !== 'undefined') {
    module.exports = factory(require('jquery'))
  } else {
    factory(jQuery)
  }
}(function ($) {
  // All the normal plugin here
}))

Insert your normal ; if you'd like I removed them since I use standard as my lint tool

alexweissman commented 7 years ago

All, I've run into an issue with this template in Select2 (see https://github.com/select2/select2/commit/45a877345482956021161203ac789c25f40a7d5e#commitcomment-24593031).

Any guidance would be appreciated, as Select2 is a fairly large community and this issue could potentially be affecting a lot of users.