v8 module: allow TypedArray and DataView #23953
Conversation
nodejs-github-bot
added
the
c++
jasnell
added
the
semver-minor
test/parallel/test-v8-serdes.js
Outdated
| const length = des.readUint32(); | ||
| const buf = des.readRawBytes(length); | ||
|
|
||
| let length = des.readUint32(); |
test/parallel/test-v8-serdes.js
Outdated
| assert.strictEqual(buf.toString(), 'hostObjectTag'); | ||
| for (let i = 0; i < arrayBufferViewsLength; i += 1) { | ||
| length = des.readUint32(); |
There was a problem hiding this comment.
Could these be const length2 and const buf2 please?
Recycling variables should be avoided.
test/parallel/test-v8-serdes.js
Outdated
| // `v8.serializer.writeRawBytes()` should support both `TypedArray` | ||
| // and `DataView`. | ||
| const arrayBufferViews = common.getArrayBufferViews(buf); | ||
| arrayBufferViewsLength = arrayBufferViews.length; |
There was a problem hiding this comment.
Could this be const arrayBufferViewsLength instead.
.
.
.
Which made be think isn't there a race here?
Is the callbacks guaranteed to be in order?
At minimum this should be documented in a comment
There was a problem hiding this comment.
Is the callbacks guaranteed to be in order?
Yes. We have only one HostObject so that the callback will be called only once.
I think v8.Serializer.writeRawBytes() being expected to be called inside of the callback of _writeHostObject makes the test a bit confusing. I'm going to refactor this.
|
@oyyd as alway thank you for the contribution. While reviewing the code and looking at cases of variable recycling I found that I don't understand the operation order guarantee. |
test/parallel/test-v8-serdes.js
Outdated
| @@ -66,6 +66,7 @@ const deserializerTypeError = | |||
| } | |||
|
|
|||
| { | |||
| let arrayBufferViewsLength; | |||
There was a problem hiding this comment.
Maybe simplest solution is to add a sentinel:
let has_writeHostObjectCalled = false;the in _writeHostObject
has_writeHostObjectCalled = true;the first line of _readHostObject
assert.ok(has_writeHostObjectCalled);There was a problem hiding this comment.
or dual use arrayBufferViewsLength - initialise it to false, and then assert
des._readHostObject = common.mustCall(() => {
assert.notStrictEqual(false, arrayBufferViewsLength);
...
|
@refack Thanks for your advice! Comments addressed. I have separated the test for |
Looks 💯. Thank you for following up! |
refack
added
the
author ready
This commit allow passing `TypedArray` and `DataView` to: - v8.deserialize() - new v8.Deserializer() - v8.serializer.writeRawBytes() PR-URL: nodejs#23953 Refs: nodejs#1826 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
|
Landed in 56881b0 🎉 |
refack
removed
the
author ready
This commit allow passing `TypedArray` and `DataView` to: - v8.deserialize() - new v8.Deserializer() - v8.serializer.writeRawBytes() PR-URL: #23953 Refs: #1826 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
This commit allow passing `TypedArray` and `DataView` to: - v8.deserialize() - new v8.Deserializer() - v8.serializer.writeRawBytes() PR-URL: #23953 Refs: #1826 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
This commit allow passing
TypedArrayandDataViewto:v8.deserialize()new v8.Deserializer()v8.serializer.writeRawBytes()Refs: #1826
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes