dont patch globals
Closed this issue · 23 comments
var setImmediate = require("setImmediate")
It should be a clean function without global side effects.
Have side effects be opt in.
Although I sympathize, this is a shim for a global, not a CommonJS module. The CommonJS module is just a transport format for those who don't have the ability to load scripts directly into their environment.
In other words, you opt-in by requireing it.
@domenic why would you want a global side effect in a commonJS environment.
Just fork the code path to not mutate global scope in a commonJS environment and only mutate it in a non-commonJS environment.
Because you're shimming a global scope that doesn't exist correctly, as in e.g. Node < 0.9. setImmediate is by spec and by contract a global variable; what you're looking for is something different, viz. a CommonJS module with semantics similar to setImmediate but without the specified property of being attached to the global object.
Anyway the entire argument doesn't even matter anymore. require("timers").setImmediate is a thing.
However "shims" that patch global scope are silly.
Not a fan of es5-shim anymore? Sad times.
@domenic not a fan of touching globals ever anymore, i don't care what the reason is. I don't need es5 shim because I don't support safari or ie8
I personally am a fan of shims that optionally fix the global scope. It's easy to build a shim for setImediate on top of a CommonJS module that has the functionality of setImediate but creating a CommonJS module that doesn't touch globals out of a shim for setImediate requires forking the project.
Nah.
require("setimmediate");
module.exports = global.setImmediate;
delete global.setImmediate;done!
Super edge casey. You need to check for existence of setImediate first for example.
if (typeof global.setImmediate === 'undefined') {
require("setimmediate");
module.exports = global.setImmediate;
delete global.setImmediate;
} else {
module.exports = global.setImmediate;
}comes close. It still has edge cases though, such as if global.setImediate is not compliant, or is explicitly set to undefined rather than just never having been set.
Just ran in the same issue here. Are there any forks which does this without polluting the global namespace?
I just use next-tick as a process.nextTick polyfill. #worksforme
@Raynos That was already something that I'm doing: https://github.com/unshiftio/tick-tock/blob/master/index.js#L5-L20 but I need something that works in browsers as well.
@3rd-Eden https://github.com/medikoo/next-tick works in browsers.
@domenic for those of us coding "widgets" to be plugged in 3rd party websites, including code into the widget that pollutes the window namespace is a MUST NOT since it may create conflicts with the hosting website (not in our control).
The CommonJS module is just a transport format for those who don't have the ability to load scripts directly into their environment.
Well, do you really think that developers using CommonJS use it because "they don't have the ability to load scripts directly into their environment"?
It sounds like this library isn't a good fit for your requirements then.
Developers coding with CommonJS/AMD in mind produce code/libraries suitable for CommonJS and AMD, and also for all those devs still loading scripts via <script> tags. That's easy by simply using browserify (or similar) which produces CommonJS/AMD/global suitable exports, or by just adding these few lines of code:
// AMD
if ( typeof define == 'function' && define.amd )
{
define([], function()
{
return
{
setImmediate : setImmediate,
clearImmediate : clearImmediate,
toGlobal : toGlobal
};
});
}
// CommonJS
else if ( typeof module != 'undefined' && module.exports )
{
exports.setImmediate = setImmediate;
exports.clearImmediate = clearImmediate;
}
// Legacy <script> loading
else
{
global.setImmediate = setImmediate;
global.clearImmediate = clearImmediate;
}The problem is in the other side: in all those libraries forcing global pollution yes or yes and arguing that it's the desired design. Just the wrong way.
It sounds like this library isn't a good fit for your requirements then.
Yes, the feature provided by this library fits my requirements. The problem is the way in which you force me to consume the library. Hope you see the difference.
I was referring to the library as a whole, not just its features.
IMHO you should not force a single way of consuming your library (or the good features it provides).
I understand, respect, and disagree with your opinion.
If you don't need global pollution - you can use library version of core-js
var setImmediate = require('core-js/library/fn/set-immediate');IIRC the same approaches for task implementation.
For most use cases, you'd be better off with https://www.npmjs.com/package/asap than a setImmediate polyfill anyway.