New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider val != null
here if the goal is to be equivalent to val !== null && val !== undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just do this
return val && val.constructor && typeof val.constructor.isBuffer === 'function' && val.constructor.isBuffer(val)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-buffer
can 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.js
with said inline function and removes the dependency onis-buffer
frompackage.json
.