jonschlinkert / kind-of

Get the native JavaScript type of a value, fast. Used by superstruct, micromatch and many others!
https://github.com/jonschlnkert
MIT License
351 stars 40 forks source link

Don't treat buffer any specially #40

Open jimmywarting opened 3 years ago

jimmywarting commented 3 years ago

https://github.com/jonschlinkert/kind-of/blob/abab085d65f7ee978011da8f135291892fcd97db/index.js#L124

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

mindplay-dk commented 2 years ago

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:

https://github.com/jonschlinkert/kind-of/blob/abab085d65f7ee978011da8f135291892fcd97db/index.js#L119-L122

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 commented 2 years ago

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

mindplay-dk commented 2 years ago

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

https://github.com/jonschlinkert/kind-of/blob/abab085d65f7ee978011da8f135291892fcd97db/index.js#L12-L14

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:

https://github.com/jonschlinkert/kind-of/blob/abab085d65f7ee978011da8f135291892fcd97db/index.js#L96-L98

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 commented 2 years ago

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