No buffer in browser

[fgnass]

This PR fixes #846 by using Feross' is-buffer module instead Buffer.isBuffer().

[coveralls]

Coverage increased (+1.5%) to 93.719% when pulling c849467 on fgnass:no-buffer-in-browser into f31317a on mzabriskie:master.

[albertogasparin]

An alternative to the is-buffer module is just changing the isBuffer util function to:

function isBuffer (obj) {
  return !!obj.constructor && typeof obj.constructor.isBuffer === 'function' && obj.constructor.isBuffer(obj)
}

[atri-dastidar-snkeos]

@fgnass: In lib/adapters/http.js you have removed Buffer, but you are still using it in the same file. Since Buffer is a global anyway, it is still being used.

Why not move the import of is-buffer from utils.js to http.js itself, and change this line to
if (isBuffer(data)) {

Or if you want to retain the function isBuffer in utils, you can do what @albertogasparin suggested.

BTW @albertogasparin: what you suggested is exactly what the is-buffer module does 😄

[fgnass]

I removed the require call because http.js is only used in Node.js and Buffer is a global there so there's no need for importing it. I'm fine with inlining the is-buffer code in utils.js as we don't really need the extra checks for node 0.10 and Safari 4/5 that are provided by Feross' module.

[fgnass]

... in http.js it's fine to use the native Buffer.isBuffer() check directly which, as a positive side effect, also covers the SlowBuffer case.

[albertogasparin]

@csvbox I know, as it's a one liner it looks silly to me importing a 3rd party dependency
@fgnass Are you sure that by using Buffer.isBuffer() in http.js Webpack is not including Buffer anyway? Is it not the reason why is-buffer was created (and you imported it) in first place?

[atri-dastidar-snkeos]

I removed the require call because http.js is only used in Node.js

Does it matter for the webpack build? In webpack.config.js I don't see separate targets defined for node or web, so even if you reference Buffer only in http.js, it would still be included, right? If yes, all other changes are rendered pointless.

[fgnass]

@csvbox Webpack respects the "browser" field in package.json (https://github.com/mzabriskie/axios/blob/master/package.json#L71) and therefore replaces http.js with xhr.js whenever it is required.

[albertogasparin]

@csvbox I've tested @fgnass code and it works
@fgnass May I ask you what it the reason to include is-buffer now? It looks like it's not used anywhere. I think Buffer.isBuffer in http.js is all we need (and it's supported by node since ever, so checking if it exists seems pointless).

[albertogasparin]

My bad, utils.isBuffer is actually used in lib/defaults.js (missed it when I re-checked the project...).
Only question left for me is whenever include is-buffer module or write the one liner.

[atri-dastidar-snkeos]

Webpack respects the "browser" field in package.json (https://github.com/mzabriskie/axios/blob/master/package.json#L71) and therefore replaces http.js with xhr.js whenever it is required.

@fgnass sorry I didn't see that before, thanks

[fgnass]

I would opt for keeping is-buffer as it already comes with good test coverage. We would otherwise have to copy the tests too. Small modules FTW.

[rubennorte]

We're usually avoiding adding new dependencies to the project but since the module is very small I think it's OK. @mzabriskie, @nickuraltsev WDYT?

[nickuraltsev]

I'm ok with this dependency. @fgnass Thank you for the PR!