Hammer.js: Don't create a window.Hammer global when in CommonJS

[ibc]

In hammer.js:

// this prevents errors when Hammer is loaded in the presence of an AMD
//  style loader but by script tag, not by the loader.
var freeGlobal = (typeof window !== 'undefined' ? window : (typeof self !== 'undefined' ? self : {})); // jshint ignore:line
freeGlobal.Hammer = Hammer;

Not sure what such a code wants to prevent, but the fact is that it's polluting the global namespace (window) even when CommonJS or AMD is being used.

It shouldn't.

[runspired]

@ibc it should, for the reasons stated there.

[ibc]

According to such a rationale, every CommonJS/AMD capable library should also force a global. Is that what you mean?

[ibc]

To be perfectly clear: Your rationale is wrong. Otherwise, let's tell Reactjs, jquery and over 2000 NPM libraries that they should expose a window.Xxxxx global. So please, don't say that "it should" because that's not true. The author of a web application chooses whether he uses CommonJS/AMD loaders or legacy <script> tags.

[runspired]

@ibc No, this is a factor of usage. jQuery uses this same pattern, for example. Hammer would often fail in a Cordova, Electron or NWJS package were we to not use this pattern.

[ibc]

As Cordova user I've never found such a problem. The point (or one of the points) of CommonJS/AMD libraries is to not pollute the window namespace.

Anyhow I may understand your concerns. However, a better solution could be to provide a bundled file (generated with browserify) for those loading your lib via a <script> tag, and let all the others load it as a NPM library, by removing all the "loader detector" from the main source code.

To be clear: when I use browserify I don't need that a lib (such as hammerjs) comes within a single JS file, nor I need that the lib checks whether it's running in a CommonJS/AMD environment.

[ibc]

Also, jQuery does not follow such a pattern:

$ npm install jquery
$ node

> require('jquery');

> global.jQuery
undefined
> global.jquery
undefined
> global.$
undefined

No globals.

[runspired]

It does though :P https://github.com/jquery/jquery/blob/master/src/exports/global.js

[ibc]

Well, just reproduce my code above. There are no globals after loading require('jquery'). I think that the "no conflict" stuff of jQuery is not present in the NPM version of the library.

[arschmitz]

@ibc I also work on the jQuery team and the npm version is not any different and it does expose a global still see here in the distributable https://github.com/jquery/jquery/blob/3.1.1/dist/jquery.js#L10166

This is in fact necessary because there are others that have written plugins for hammer that depend on it being available in the global context this is the same situation for jQuery. If we did not expose the global on window most jQuery plugins would break. The reason your code above does not work is because your checking global.* not window.*

[ibc]

Well, I've just added jquery NPM version into a browserify based app, and loaded it as usual in may app main JS file (the entry point of the bundled file):

var foo = require('jquery');

There is not window.jquery nor window.jQuery at all.

[arschmitz]

@ibc well i dont know whats going on in your app or your browserify build process but you can see it clearly on this page http://view.jqueryui.com/master/demos/button/default.html that loads jQuery via amd if you go into the console you can clearly see the global exists.

Again there is no "npm version" of jQuery the file i linked _IS_ The latest version on npm

[ibc]

It just happens (checked right now) that this line is called with noGlobal === true so the global is not created.

That is because when browserify runs, there is no global.document so this code does not call the factory with true as second parameter.

I don't know if that is the desired behavior or not. Anyhow, I may understand that jQuery does a hack due the amount of 3rd party libraries relying on window.jQuery, but that does not mean that a new and modern library such as hammerjs should follow the same legacy pattern.

[arschmitz]

@ibc the fact is we face the same problem we have 3rd party plugins the depend on there being a global we would break 3rd party plugins if we change this so we won't be changing it like @runspired said

[aluanhaddad]

@arschmitz @runspired There needs to be a version that does not create a global. Even if I do not use any plugins, hammerjs still pollutes my namespace. Additionally more and more people continue to depend on the global because you continue to provide it.

It obviously is a chicken and egg problem but that is not a good reason to hold back the entire ecosystem.

moment did the right thing.
moment deprecated their global.
moment removed their global.
moment broke many people's apps.
moment broke apps I was responsible for maintaining (forgot to import and paid the price).
They did the right thing.

Please consider deprecating your global, giving plugin authors time to update and users time to decide, and then remove it completely.

[ibc]

we have 3rd party plugins the depend on there being a global

A bad practice should not be kept forever.