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

Bytes as ArrayBuffer #1590

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Bytes as ArrayBuffer #1590

wants to merge 2 commits into from

Conversation

Scooletz
Copy link

@Scooletz Scooletz commented Jul 28, 2023

This PR introduces a capability to convert raw byte chunks like byte[] as an ArrayBuffer. It does it by implementing an object converter directly in Jint core project. This is done to not make the ArrayBufferInstance public and keep the API change as minimal.

Open questions:

  1. Why the test is failing? I tried to debug it and I fail to understand why an ArrayBuffer is not an instance of ArrayBuffer. I think it's not realm connected. I observed that Object.prototype.toString returns something different than expected as well. I'd appreciate some help.
  2. Are we ok with introduction of a custom object converter in the core?
  3. Are we ok to implement a similar approach for Memory<byte> which would require offset bookkeeping in the ArrayBuffer (I can implement it).

@lahma
Copy link
Collaborator

lahma commented Jul 28, 2023

This PR introduces a capability to convert raw byte chunks like byte[] as an ArrayBuffer. It does it by implementing an object converter directly in Jint core project. This is done to not make the ArrayBufferInstance public and keep the API change as minimal.

Nice! Here's some feedback from the top of my head.

I think it would also be fine if we rename ArrayBufferInstance to more idiomatic JsArrayBuffer and expose it as public, generally things are internal if they haven't been API-checked for general consumption. We've already changed DateInstance to JsDate etc to get clean API. This would be for the case if you want to return from your c# code something like engine.Realm.Intrinsics.ArrayBuffer.Construct(bytes); (see below).

  1. Why the test is failing? I tried to debug it and I fail to understand why an ArrayBuffer is not an instance of ArrayBuffer. I think it's not realm connected. I observed that Object.prototype.toString returns something different than expected as well. I'd appreciate some help.

The constructed object is not properly initialized, I'd suggest adding new Construct method to ArrayBufferConstructor:

public ArrayBufferInstance Construct(byte[] data)
{
    var obj = OrdinaryCreateFromConstructor(
        this,
        static intrinsics => intrinsics.ArrayBuffer.PrototypeObject,
        static (engine, _, state) => new ArrayBufferInstance(engine, state!),
        data);

    return obj;
}

And changing your converter logic to:

if (value is byte[] bytes)
{
    result = engine.Realm.Intrinsics.ArrayBuffer.Construct(bytes);
    return true;
}

Note, now ArrayBufferConstructor could also be made public if there's use case for such construction and everything "unnecessary" is hidden as internal/private.

  1. Are we ok with introduction of a custom object converter in the core?

I think there might be small problem with ambiguity here. You could also want to have your byte[] to be converted to Uint8Array or Uint8ClampedArray - so automatic conversion during runtime might not be obvious, or one could want to have both possibilities (converting to both ArrayBuffer or Uint8Array).

  1. Are we ok to implement a similar approach for Memory<byte> which would require offset bookkeeping in the ArrayBuffer (I can implement it).

Everything what makes Jint more usable in interop scenarios sounds good to me.

@Scooletz
Copy link
Author

Thank you for a prompt and in-depth reply @lahma . This is my first more serious work with Jint codebase so that I'm stepping into unfamiliar territory. Let me dive deeper with your comments in mind and get this PR altered over the weekend.

@lahma
Copy link
Collaborator

lahma commented Jul 28, 2023

With #1591 I did some preliminary work and did the rename and expose of type. If you find it useful for future, could also unseal it if there's some benefit with different backing store implementations.

So if you rebase, you should have things readily available.

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

Successfully merging this pull request may close these issues.

None yet

2 participants