Skip to content
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

Don't treat buffer any specially #40

Open
jimmywarting opened this issue Aug 8, 2021 · 4 comments
Open

Don't treat buffer any specially #40

jimmywarting opened this issue Aug 8, 2021 · 4 comments

Comments

@jimmywarting
Copy link

function isBuffer(val) {

Uint8Array works better cross Deno & browsers, really think you should make a instanceof check instead...

@mindplay-dk
Copy link

mindplay-dk commented Mar 29, 2022

This would be a breaking change.

Going by the implementation, I think the buffer type reported by this library intentionally designates the NodeJS Buffer type.

Although the comment is confusing:

kind-of/index.js

Lines 119 to 122 in abab085

/**
* If you need to support Safari 5-7 (8-10 yr-old browser),
* take a look at https://github.com/feross/is-buffer
*/

In modern browsers, is-buffer can not be used to check for browser's buffer types:

isBuffer(new Uint8Array()) // => false

isBuffer(new Uint8Array().buffer) // => false

I'm not familiar with the history of Safari - I can't say if it actually had a Buffer type at some point, or whether that was compatible with the NodeJS Buffer type.

What I can say, is that buffers in the browser are represented by the ArrayBuffer type, which is unrelated to the NodeJS Buffer type - they are both "buffers" in the logical sense, but they are not compatible types; they don't even exist in the same JS environments, as far as I'm aware, and so I don't think this library should designate these as buffer.

Futhermore, types such as uInt8Array etc. in the browser aren't technically "buffers" in the first place - they're what's known as "views", which are a kind of proxy to a buffer.

To illustrate that point:

new Uint8Array() instanceof ArrayBuffer // => false
new Uint8Array().buffer instanceof ArrayBuffer // => true

These view-types aren't compatible either - describing them as broadly as "buffers" would likely be confusing more than useful.

Note that the browser provides a standard API for type-checking those:

ArrayBuffer.isView(new Uint8Array()) // => true

I would vote to close this issue.

@mindplay-dk
Copy link

Unless there's a good explanation for it, I would also suggest that the mentioned comment be removed.

@mindplay-dk
Copy link

On that same note, I think a mistake was made here:

kind-of/index.js

Lines 12 to 14 in abab085

if (type === 'function') {
return isGeneratorFn(val) ? 'generatorfunction' : 'function';
}

This has all the same problems I described above - if someone doesn't know this implementation detail, and wants to check if something is a function, they might write a condition like kindOf(val) === "function", which would fail for generator functions.

If someone did want to check specifically for generator functions, you do of course have this function:

kind-of/index.js

Lines 96 to 98 in abab085

function isGeneratorFn(name, val) {
return ctorName(name) === 'GeneratorFunction';
}

However, this is also unreliable, for the same reasons described above - if you needed to check if something is a generator (or more likely an iterable/iterator) the only reliable way to implement that, is to check the returned value after calling, so it's probably not even a feature that should exist.

Of course, this would be a breaking change, which is unfortunate - on the other hand, if someone had to upgrade, they would be made aware of a likely source of bugs in their programs, so it might be for the better.

@jimmywarting
Copy link
Author

jimmywarting commented Mar 29, 2022

My plot was that it's bad to encourage user to rely on node:buffer for cross compatible reasons.
I may end up using either a node:buffer or Uint8array depending on which enviorment i'm in.

If I where to treat Uint8array and node:buffer equally as the same type (cuz it walks like a duck and quacks like a duck - and both are instances of uint8array) and then use this lib to check what type of x is, then x would be either 'buffer' || 'uint8array' where as i would like to only get uint8array for the type node:buffer as well.

i don't want to give node:buffer any special treatment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants