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

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.

About this issue

Original URL
State: closed
Created 8 years ago
Reactions: 1
Comments: 15 (6 by maintainers)

Most upvoted comments

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

A bad practice should not be kept forever.

ibc on Dec 31, 2016

@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.

aluanhaddad on Dec 30, 2016

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.

ibc on Sep 26, 2016

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 on Sep 26, 2016

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.

ibc on Sep 26, 2016

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

ibc on Sep 26, 2016