Remove dependency on is-buffer #1816
Conversation
| * @returns {boolean} True if value is a Buffer, otherwise false | ||
| */ | ||
| function isBuffer(val) { | ||
| return ![undefined, null].includes(val) && val.constructor === Buffer; |
There was a problem hiding this comment.
consider val != null here if the goal is to be equivalent to val !== null && val !== undefined
There was a problem hiding this comment.
also consider checking typeof Buffer !== "undefined" so that we don't get strict mode errors related to the Buffer variable not being defined (browser tests should catch this?)
There was a problem hiding this comment.
just do this
return val && val.constructor && typeof val.constructor.isBuffer === 'function' && val.constructor.isBuffer(val)
There was a problem hiding this comment.
Why not simplify with Buffer.isBuffer ?
If the package is installed via npm, we can assume it's running in Node and has Buffer defined.
Otherwise, it gets polyfilled during Webpack builds for browsers.
There was a problem hiding this comment.
It's even used in other places of the code, like here: https://github.com/axios/axios/blob/master/lib/adapters/http.js#L41
There was a problem hiding this comment.
Nevermind- I tried it and it injects the polyfill for the whole Buffer module. Your solution works best.
There was a problem hiding this comment.
So, are you going to merge it? Do you want me to do anything else?
|
Please let us know the minified size difference of this change |
|
@RikkiGibson I think the main advantage would be reducing the dependency tree and not reducing the package size, which would be minimal anyways. |
|
Can someone approve this PR? |
|
@flavior121 I agree, no idea why this has taken so long. |
|
can you add some tests? happy to merge this as soon as we add tests to this, since is-buffer had tests before, also thank you for being so patient this has taken a pretty long time I agree, hopefully this can be quick this time around |
|
@yasuf I just added some tests, I think they should suffice provided the rest of the utilities are tested in a similar manner. Let me know if you need anything else. |
The dependency on
is-buffercan be easily inlined with a function that has the same functionality, reducing download size and increasing maintainability for the package. This PR adds a function inlib/util.jswith said inline function and removes the dependency onis-bufferfrompackage.json.