stop global pollution and clean up methods/exports #12
Conversation
i would fix that resolvable antipattern but gh web editor
index.js
Outdated
| @@ -1,8 +1,8 @@ | |||
| /* globals fetch, Headers */ | |||
| /* istanbul ignore next */ | |||
| if (!process.browser) { | |||
| global.fetch = require('node-fetch') | |||
| global.Headers = global.fetch.Headers | |||
| var fetch = require('node-fetch') | |||
There was a problem hiding this comment.
This will shadow the global fetch in browsers with undefined.
There was a problem hiding this comment.
the var fetch will in fact be hoisted, unconditionally, to the top of the scope, which since this is a module, won't be the global scope - it will be a function wrapper.
There was a problem hiding this comment.
...That code won't run in browsers, and in node it will hoist to the bootstrap wrapper function in module compile
There was a problem hiding this comment.
var fetch runs unconditionally, even if it's in the if block - it's hoisted.
Which means that it's the same as:
var fetch;
if (!process.browser) {
fetch = require('node-fetch');
}| module.exports.delete = (...args) => new R2().delete(...args) | ||
| const methods = ['get', 'put', 'post', 'head', 'patch', 'delete'] | ||
| for (const method of methods) { | ||
| R2.prototype[method] = function(...args) { |
There was a problem hiding this comment.
This is actually much slower, and also not equivalent (enumerability, for one) - these should stay as explicit prototype methods.
There was a problem hiding this comment.
I'm not sure that enumerability matters in this context, and the speed of attaching things to exports isn't really an issue in any situation.
There was a problem hiding this comment.
I'm referring to the speed of using the class in the first place, at runtime.
|
Hey I think this PR is obsolete as per #38? |
No description provided.