Moddable-OpenSource / moddable

Tools for developers to create truly open IoT products using standard JavaScript on low cost microcontrollers.
http://www.moddable.com
1.32k stars 236 forks source link

TypeScript type adjustments for Socket and Worker #1414

Closed cmidgley closed 3 days ago

cmidgley commented 4 days ago

Three typescript .d.ts file changes:

(Edited to remove the xs.d.ts changes, per discussion below)

cmidgley commented 4 days ago

I don't recall all the reasons why I needed this, but here is one.

function doSomething(buffer: ArrayBuffer): void { }

const myArray = new Uint8Array(10);
doSomething(myArray);

This works perfectly on Node typings/execution, but fails without this change on XS:

error TS2345: Argument of type 'Uint8Array' is not assignable to parameter of type 'ArrayBuffer'.
      Property 'concat' is missing in type 'Uint8Array' but required in type 'ArrayBuffer'.

I've not done a comparative study between Node and XS, so this may be a naive solution. Perhaps the correct solution is adjusting Uint8Array (and associated) types to have concat?

phoddie commented 4 days ago

A Uint8Array is not a subclass of ArrayBuffer. The call to doSomething should be doSomething(myArray.buffer).

cmidgley commented 4 days ago

I understand that a view is logically not a buffer, but it does work in Node. Worse, third party modules that I depend upon consume it that way. You can even ask ArrayBuffer if it is a view or not with ArrayBuffer.isView(xxx). For example, in Node:

function doSomething(buffer: ArrayBuffer): void { 
    // it fully works as an ArrayBuffer...
    console.log(buffer.byteLength);
    console.log(new Uint8Array(buffer)[0]);
    console.log(new Uint8Array(buffer.slice(0, 3))[0]);
    console.log(ArrayBuffer.isView(buffer));
    // console.log(buffer[0]); // this is a type error as expected
}

const myArray = new Uint8Array(10);
myArray.fill(65);
doSomething(myArray);

Will output:

10
65
65
true

Also, ArrayBufferLike is challenging to work with in XS as the following (basically your example) works in Node:

function doSomething(buffer: ArrayBuffer) {}
const myArray = new Uint8Array(10);
myArray.fill(65);
doSomething(myArray.buffer);

But on XS it fails to compile:

error TS2345: Argument of type 'ArrayBufferLike' is not assignable to parameter of type 'ArrayBuffer'.
  Property 'concat' is missing in type 'SharedArrayBuffer' but required in type 'ArrayBuffer'.

29 doSomething(myArray.buffer);
               ~~~~~~~~~~~~~~

  ../../../../../../../../../../../moddable/xs/includes/xs.d.ts:65:2
    65  concat(...buffers: ArrayBufferLike[]): ArrayBuffer;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    'concat' is declared here.

This ArrayBufferLike problem doesn't get solved by making concat optional, as the error then morphs to:

error TS2345: Argument of type 'ArrayBufferLike' is not assignable to parameter of type 'ArrayBuffer'.
  Type 'HostBuffer' is missing the following properties from type 'ArrayBuffer': slice, [Symbol.toStringTag]

29 doSomething(myArray.buffer);
               ~~~~~~~~~~~~~~

I do agree in hindsight that concat? is a hack and wrong (and doesn't address the whole problem). But I'm unclear how to get compatibility with Node modules without doing something like adding concat, slice, etc. to HostBuffer and adjust ArrayBufferLike so it works across the types.

As always I'm glad to help with a PR, but I need guidance on this one. Thanks!

phoddie commented 4 days ago

I understand that a view is logically not a buffer, but it does work in Node.

The example above generates the same result running under XS as Node.

You can even ask ArrayBuffer if it is a view or not with ArrayBuffer.isView(xxx).

You can ask ArrayBuffer.isView about anything. It returns true for TypedArrays and DataView.

ArrayBuffer.isView("123");
ArrayBuffer.isView({});
ArrayBuffer.isView(1);
ArrayBuffer.isView();

Also, ArrayBufferLike is challenging to work with in XS as the following (basically your example) works in Node

That generates the same result in XS and Node too: both agree that an ArrayBuffer instance isn't a view.

ArrayBuffer and Uint8Array are not generally interchangeable. It is a mistake to pass a Uint8Array to a call that expects an ArrayBuffer though it might work in some cases - as you found. For example. new Uint8Array(buffer) works for Uint8Array because the TypedArray constructor will iterate the Uint8Array to get the byte values (it will do the same for any other TypedArray). But that's making a copy into a new buffer, which is very different from what happens when passing the Uint8Array constructor an ArrayBuffer (the ArrayBuffer is used as-is without making a copy).

I think the question here is why does TypeScript let you pass a Uint8Array to a call declared as expecting an ArrayBuffer since they really aren't interchangeable. I think that's because when the types don't match, TypeScript falls back to "duck typing". A strictly conforming ArrayBuffer instance (prior to resizable support in ES2024, at least) has the following properties (beyond Object.prototype): byteLength, slice, and detached. Maybe TypeScript decides that matches and the addition of concat by XS breaks the duck typing check. If you enable ES2024 in TypeScript, does your example still pass type checks under Node?

cmidgley commented 4 days ago

If you enable ES2024 in TypeScript, does your example still pass type checks under Node?

I have es2022, strict: true, strickNullChecks: true, and noImplicitAny: true. I did go to the TypeScript playground, where I can select es2023 (it does not have es2024) and it works (see this playground).

Maybe TypeScript decides that matches and the addition of concat by XS breaks the duck typing check.

I do think you are correct that concat causes duck typing to not match, and that's even worse with ArrayBufferLike due to HostBuffer.

Node's typings allow duck typing to work for things like UInt8Array ->ArrayBuffer and ArrayBufferLike -> ArrayBuffer. I don't know if that's intentional (and long-term supported) or just how it works today. However, third-party NPM modules depend on this, and while technically wrong (they should use ArrayBufferLike), it makes it difficult to port them over. For example, @nats-io has many cases where they pass a Uint8Array to an ArrayBuffer method. I can't fix this without forking and owning merging forever, and that would make it impractical with mcpack. I may be able to suggest they change it with a PR, and I can look into that if necessary, but I'll bet they aren't the only ones doing this.

You can ask ArrayBuffer.isView about anything.

My thinking about ArrayBuffer.isView is that by having the typeguard on ArrayBuffer it implies they intend for an ArrayBuffer to be a buffer or a view of a buffer. By checking if it's a view you get extended properties on the buffer (via ArrayBufferView) and can make smart decisions about how to consume it ... but maybe that's tea leaf reading.

So given all this, if the goal is type compatibility with Node, we might consider something like this:

1) Extend HostBuffer to be duck type compatible with ArrayBuffer (by adding the missing properties), as existing node code knows nothing about HostBuffer

2) Add the ArrayBuffer.isView type guard so it's Node compatible, and we can be efficient about reusing a view rather than cloning it in methods that receive an ArrayBufferLike.

3) Adding the XS extension concat to the various view objects, so they remain duck type compatible with ArrayBuffer (and therefore, with ArrayBufferLike)

An approach similar to this would extend the functionality a bit on the core XS buffer services, but hopefully make it type cross-compatible with Node. The devil is in the details, so some experiments would need to be done to be sure the end result works (mock the types and verify compatibility before investing in the implementations).

p.s. I mentioned earlier @nats-io. It's an impressive scalable messaging, object store, and distributed service execution engine with a well-abstracted TypeScript client library (@nats-io/nats.js). It's quite large and complex, and I have gotten it working on XS simply with polyfills (and myconcat?` hack) and not a single line change to their library. All on cheap little ESP32 with 4MB PS-RAM. I'm totally blown away at the power of XS!

phoddie commented 4 days ago

I found an ES2024 TypeScript PR. It contains an update to ArrayBuffer with resizable support. Replacing the ArrayBuffer support in xs.d.ts and building your example gives:

main.ts:16:13 - error TS2345: Argument of type 'Uint8Array' is not assignable to parameter of type 'ArrayBuffer'.
  Type 'Uint8Array' is missing the following properties from type 'ArrayBuffer': maxByteLength, resizable, resize, detached, and 2 more.

16 doSomething(myArray);
               ~~~~~~~

So.... the problem isn't concat but that you got lucky, thanks to duck typing and the multitude of options in the TypedArray constructors, in ever being able to pass a Uint8Array to a function expecting an ArrayBuffer. I'm not sure what the right solution is, but am reasonably confident it isn't changing concat (unless ES2024 TypeScript is going to make all the additions to ArrayBuffer for resizable support optional, which seems unlikely). My feeling is that this should be fixed in the script (doSomething, etc):

function doSomething(buffer: ArrayBufferLike): void {}
...
doSomething(myArray.buffer);

Note that ArrayBufferLike is what doSomething intends to accept, since it would also work with a SharedArrayBuffer (in the standard) and with a HostBuffer under XS.


Changing HostBuffer isn't an option -- host buffers are created in many places and are deliberately minimal. Any... anyway, you would also have to deal with SharedArrayBuffer being different (it is growable in ES2024, not resizable!).


Your @nats-io experience is great. I've had good luck, as have others, pulling in npm packages with minimal dependencies and more-or-less having them "just work" under XS. We should collect those together somehow... it would benefit more folks. But that's definitely a separate conversation from this thread.

cmidgley commented 4 days ago

Thanks for all your time on this. I really appreciate it.

I'll work with the @nats-io and try to get this changed (they've been very responsive so far). Having es2024 as a reference point will help the argument.

I'll update this PR to remove the xs.d.ts change, and see what other feedback you may have at that time.

phoddie commented 4 days ago

Thanks for sticking with this. It is a small point, but I don't want to make strange changes to core typings without understanding them well first.

The rest of the PR looks reasonable, but for one small detail. The in WorkerOptions everything is optional (see the implementation of getIntegerProperty() in modWorker.c) and number and symbol are also numbers; so it should be more like this:

    export interface WorkerOptions {
        static?: number;
        chunk?: {
            initial?: number;
            incremental?: number;
        };
        heap?: {
            initial?: number;
            incremental?: number;
        };
        stack?: number;
        keys?: {
            initial?: number;
            incremental?: number;
            name?: number;
            symbol?: number;
        };
    }
cmidgley commented 3 days ago

Excellent - not sure how I ended up with string (goes to show I don't use those...), but I didn't realize the optionality of the others. Changes made, and open to any other suggestions. Thx.

mkellner commented 3 days ago

This has been committed and will be available in the next update.