should we be sniffing for `window` and binding `window.fetch` instead of using just `global` when using npm package?

[niftylettuce]

updated title

[niftylettuce]

maybe you don't need this - because you defined browser in package.json, right?

[niftylettuce]

otherwise, this is how it'd need to be refactored I think?

-var realFetch = require('node-fetch');
+var realFetch = (typeof window === 'undefined')
+  ? require('node-fetch')
+  : require('whatwg-fetch');
+

-if (!global.fetch) {
-       global.fetch = module.exports;
-       global.Response = realFetch.Response;
-       global.Headers = realFetch.Headers;
-       global.Request = realFetch.Request;
+if (typeof window === 'undefined') {
+  if (!window.fetch) {
+    window.fetch = module.exports;
+    window.Response = realFetch.Response;
+    window.Headers = realFetch.Headers;
+    window.Request = realFetch.Request;
+  }
+} else {
+  if (!global.fetch) {
+    global.fetch = module.exports;
+    global.Response = realFetch.Response;
+    global.Headers = realFetch.Headers;
+    global.Request = realFetch.Request;
+  }
 }

[niftylettuce]

actually we need to - because if we use jsdomify then it injects window, I will submit PR

// create our DOM instance and expose `global.window`
// so that when we import Frisbee it will use `whatwg-fetch`
// instead of using `node-fetch` as the other test does
jsdomify.create();

import es6promise from 'es6-promise';
es6promise.polyfill();
import 'isomorphic-fetch';
import Frisbee from '../../src/frisbee';

console.log('window.fetch', window.fetch); // <-- returns undefined

[tracker1]

Don't do the else, just test for global and window separately...

if (typeof window !== 'undefined' && !window.fetch) {
  ...
}
if (typeof blobal !== 'undefined' && !window.fetch) {
  ...
}

[niftylettuce]

why are you doing this btw?

module.exports = function(url, options) {
    if (/^\/\//.test(url)) {
        url = 'https:' + url;
    }
    return realFetch.call(this, url, options);
};

why are you forcing https on // paths? the browser auto detects // by checking if https or http is available on them?

[niftylettuce]

leave it up to user to put in the proper URL, why are you doing this explicitly? seems like people would eventually file issues reg'd this

[gobengo]

I agree with @niftylettuce . Push the burden/power of URL building onto the caller.

IMO // is an antipattern anyway, because web documents are not-infrequently served over non-http/https protocols (e.g. file:// in localdev, Apache Cordova, and more). But that doesn't mean you should change it to https.

[niftylettuce]

I removed this in my PR referenced On Nov 29, 2015 2:28 PM, "Benjamin Goering" [email protected] wrote:

I agree with @niftylettuce https://github.com/niftylettuce . Push the burden of URL building onto the caller.

IMO // is an antipattern anyway, because web documents are not-infrequently served over non-http/https protocols (e.g. file:// in localdev, Apache Cordova, and more). But that doesn't mean you should change it to https.

— Reply to this email directly or view it on GitHub https://github.com/matthew-andrews/isomorphic-fetch/issues/53#issuecomment-160455110 .

[niftylettuce]

don't merge yet - I have to fix tests, indentation to use tabs (sigh), and also fix something else

[niftylettuce]

OK pull request is ready!