mroderick / PubSubJS

Dependency free publish/subscribe for JavaScript
MIT License
4.77k stars 457 forks source link

Problem use in requirejs #35

Open Troland opened 10 years ago

Troland commented 10 years ago

It's weired that when i run rake jquery to get a jquery pubsub version.After that, i use it in requriejs version 2.1.8+ where the config is like

requirejs.config({
        'paths': {
            'jquery': 'jquery-1.10.2.min',
            'pubsub': 'jquery.pubsub'
        },
        'shim': {
            'pubsub': ['jquery']
        }
});

it produced a error said

PubSub is not defined` in line `pubsub.version = PubSub.name + ' ' + PubSub.version;

It seem that the the line 38 `` if (typeof exports === 'object' && module){ module.exports = factory();

// AMD
} else if (typeof define === 'function' && define.amd){
    define(factory);
// Browser
} else {
    root.PubSub = factory();
}
mikkotikkanen commented 10 years ago

+1

mroderick commented 10 years ago

That looks like a RequireJS configuration issue to me.

Did you get it working in the end?

iborisov commented 10 years ago

+1 PubSub object is available only in factory function.

aron commented 10 years ago

@mroderick the jQuery shim expects PubSub to be a global, but since introducing the new UMD wrapper it isn't so you need to check for and require jQuery in AMD and Node environments. e.g.:

  function createPlugin(jQuery, PubSub) {
    // Create the pub sub methods.
  }

  if (typeof define === 'function' && define.amd){
        require(['jquery', 'pubsub'], createPlugin);
    } else if (typeof exports === 'object'){
        // CommonJS
        createPlugin(require('jquery'), require('pubsub'));
    } else {
        // Browser globals
        createPlugin(window.jQuery, window.PubSub);
    }
mroderick commented 10 years ago

I've removed the line that calculates the version property for jQuery with https://github.com/mroderick/PubSubJS/commit/2d171aece492fe51a525491a768d3980713b4a24 as the core property was removed a long time ago.

mroderick commented 10 years ago

@aron I've also created a pull request to show that it is possible to load the jQuery built version in an AMD context: https://github.com/mroderick/PubSubJS/pull/65.

Does this cover the scenario you described?

iborisov commented 10 years ago

You define it as a module for require.js, but the point is to use it as "regular" jQuery plugin. In my project I load all jQuery plugins at once at application startup like:

define(['jquery', 'jquery.plugin1', 'jquery.plugin2'], function($) {
  // then use plugin1 and plugin2 from jquery:
  $.plugin1(...);
  $.plugin2(...);
}

All plugins I'm using except of jquery.pubsub work this way.

amclin commented 9 years ago

Here's a simple test:

require(['jquery','pubsub'],function($,pubsub) {
  console.log(typeof(pubsub)); // Returns object
  console.log(typeof(PubSub)); // Returns undefined
  console.log(typeof($.pubsub)); // Returns function
  $.pubsub('subscribe',"foo", bar); // PubSub is undefined
});

When using AMD and the JQuery version, the pubsub module binds a useless method to the JQuery object because the globally scoped variables are never created.

I don't think is a realistic expectation that every time someone includes PubSub via require.js defines they need to also manually attach the object and define the globals.

This can be solved by removing the else wrapper around

// Browser globals
var PubSub = {};
root.PubSub = PubSub;
factory(PubSub);
bjrn commented 8 years ago

This change means that it's no longer possible to use PubSubJS without modification if one would like to avoid leaking global vars. Even though it's a small task to remove the global assign from the library when it's not desired – is unconditionally assigning window.PubSub the most common use-case?

amclin commented 8 years ago

I would argue that yes, like JQuery, a globally namespaced object is the most common use case considering the events themselves can be namespaced. Although I do think there's a valid argument for a local object, I believe that should be handled by argument/configuration as it would be the exception, not the norm.

bjrn commented 8 years ago

Just seems like always exposing a global object goes against the purpose of using AMD in the first place. If I want to expose window.PubSub I would just require the library and assign it in the beginning of the app.

Also, this "feature" was added in a patch release and had some unfortunate side-effects. I can agree however that our usage is probably less common and have no issue with manually patching the lib for our purpose.