Skip to content
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

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

Open
malthejorgensen opened this issue Dec 23, 2016 · 5 comments

Comments

@malthejorgensen
Copy link

malthejorgensen commented Dec 23, 2016

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 95563fd (latest master as of 23rd Dec 2016)

@kottenator
Copy link

kottenator commented Jan 9, 2017

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

  • module.exports = smth provides default exported object for the whole module. It will be used as var smth = require('my-module')
  • exports.smth = smth is for multiple exports from the same module (e.g.): var smth = require('my-module').smth.

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
Copy link
Author

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
Copy link

kottenator commented Jan 13, 2017

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
Copy link

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
Copy link

All, I've run into an issue with this template in Select2 (see select2/select2@45a8773#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants