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

ArrayBuffer.isView() and buffer.buffer property #4420

Closed
dy opened this issue Dec 25, 2015 · 20 comments
Closed

ArrayBuffer.isView() and buffer.buffer property #4420

dy opened this issue Dec 25, 2015 · 20 comments
Labels
buffer Issues and PRs related to the buffer subsystem. question Issues that look for answers.

Comments

@dy
Copy link

dy commented Dec 25, 2015

Hello!
ArrayBuffer.isView thinks of node buffers as views to ArrayBuffers. Is this correct behaviour?

var buffer = Buffer(0);
ArrayBuffer.isView(buffer); //true

Because this is a bit non-intuitive, as ArrayBuffer.isView() is expected to detect TypedArrays, DataViews and stuff with ArrayBuffer as a .buffer parameter (according to MDN). Indeed, buffer.buffer instanceof ArrayBuffer — that is awesome (finding that out while writing this), but at least that needs to be covered by docs (?).
Also buffer.buffer, which is ArrayBuffer, has byteLength very different from the Buffer.length. That is because of shared memory for Buffers, etc, but makes that inner ArrayBuffer useless, unfortunately.

.isView in spec.

Thank you for reading, just close if the issue is insubstantial IYO :)

@GenuineRex
Copy link

I confirmed that v5.3.0 returns false as you are experiencing. In v0.13.0-pre this code returns false. What version are you running?

@dy
Copy link
Author

dy commented Dec 25, 2015

Eemmm... 5.3.0. Windows 7. ArrayBuffer.isView(Buffer(0)) returns true.

@GenuineRex
Copy link

Correction. Mac OS X El Capitan. v5.3.0 returns true.

@mscdex mscdex added question Issues that look for answers. buffer Issues and PRs related to the buffer subsystem. labels Dec 25, 2015
@mscdex
Copy link
Contributor

mscdex commented Dec 25, 2015

/cc @trevnorris

@GenuineRex
Copy link

Try this:
console.log( Object.prototype.toString.call(buffer) );

and read this issue #1810

this pretty much explains the behaviour you are seeing.

@dy
Copy link
Author

dy commented Dec 25, 2015

[object Uint8Array] O_o kind of not what expected.
All in all, I don’t have complaints, all cool, except for the docs, which could’ve explained the behavior of buffer.buffer :)

@trevnorris
Copy link
Contributor

I'm failing to see any issue here. Buffers are Uint8Array's with an altered proto. Everything here is to be expected.

That is because of shared memory for Buffers, etc, but makes that inner ArrayBuffer useless, unfortunately.

You're forgetting about buffer.byteOffset that gives you the start position in buffer.buffer. Which allows you to create different views, or copy out a chunk of memory.

@dy
Copy link
Author

dy commented Dec 27, 2015

buffer.byteOffset

Indeed. Is that API reliable?

@trevnorris
Copy link
Contributor

It's ES spec. Not one of our own, and a read only property. Technically a getter. So should be able to rely on the returned value.

@dy
Copy link
Author

dy commented Dec 27, 2015

Which spec? This one? There is no Buffer section.

@trevnorris
Copy link
Contributor

http://www.ecma-international.org/ecma-262/6.0/#sec-%typedarray%-typedarray item number 14.

You're getting the objects confused. Buffer() return a new Uint8Array with Buffer.prototype assigned to the proto so it has access to all the node specific APIs. The implementation (https://github.com/nodejs/node/blob/v5.3.0/lib/buffer.js#L70-L72) shows that the Buffer function is only a wrapper.

@dy
Copy link
Author

dy commented Dec 27, 2015

Yes, I see what there is now, and that is nice. But considering the history of Buffer’s implementations, and the fact that there seems to be no spec on how Buffer should exactly be done, I have concerns on how much we can rely on it’s current implementation in dependent libs.
Today it is Uint8Array, yesterday it was a separate class, before it used to have .buffer property, docs do not mention a word about that current behaviour, also it’s a bit unintuitive that Buffer instanceof Uint8Array (no other buffers seem to have such behavior), so who knows what to expect, that is the only thing.

Do I understand correctly that if there is no description of that in the docs, therefore it does not fall under the stability index?

@trevnorris
Copy link
Contributor

Unfortunately these changes have been forced on us by an ever changing v8 API.

how Buffer should exactly be done, I have concerns on how much we can rely on it’s current implementation in dependent libs.

Don't follow. Mind elaborating?

Are you mainly saying that more implementation details (e.g. the fact that we have .buffer) need to be more explicitly documented?

@dy
Copy link
Author

dy commented Jan 8, 2016

Are you mainly saying that more implementation details (e.g. the fact that we have .buffer) need to be more explicitly documented?

Yes, if the .buffer property, Buffer instanceof Uint8Array and ArrayBuffer.isView(buffer) === true, were covered in the docs, that would give answer to such questions as in this ticket ). Because these features are useful, but not clear how much they are stable.

@vkurchatkin
Copy link
Contributor

@trevnorris I'd say the question is: is instances of Buffer being just Uint8Arrays is considered an implementation detail or public contract?

@trevnorris
Copy link
Contributor

@vkurchatkin fair question. may want to take this to the @nodejs/ctc to get a decision.

@Fishrock123
Copy link
Member

I'm going with implementation detail (at least for now), but that is very unlikely to change, I think.

@rvagg
Copy link
Member

rvagg commented Jan 27, 2016

I'm not sure we got a final agreement on this in the CTC meeting but it was close to @trevnorris' apparent preference for documenting the UInt8Array methods we're inheriting. It's not just an implementation detail.

@trevnorris
Copy link
Contributor

@rvagg Reason for that proposal is because the current documentation states:

the Buffer class implements the Uint8Array API in a manner that is more optimized and suitable for Node.js' use cases.

Which I read as Buffer inherits from Uint8Array. Leading developers to believe they can use Uint8Array methods on Buffer instances. If we override any of these methods then the user should know about it.

Someone suggested it would be safest just to document them all, hence why I brought that up in the meeting. Didn't intend for it to imply it was my preference. I'd be fine documenting that Buffer supports all Uint8Array methods v8 has implemented in that version, except for those we specifically document as having been overridden. This would probably be easier in the long-run, if any new typed array methods are added in the future.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Closing as there does not appear to be anything further to do on this. Can reopen if necessary.

@jasnell jasnell closed this as completed Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

8 participants