Skip to content

Use ArrayBufferLike instead of ArrayBuffer for reading/writing bytes #199

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

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

hassankhan
Copy link
Contributor

Prior to using ArrayBufferLike, TypeScript compilation errors in the generated output would occur:

src/generated/foobar.ts:1133:25 - error TS2345: Argument of type '(value: ArrayBuffer) => string' is not assignable to parameter of type 'StringLifter'.
  Types of parameters 'value' and 'bytes' are incompatible.
    Type 'Uint8Array<ArrayBufferLike>' is missing the following properties from type 'ArrayBuffer': maxByteLength, resizable, resize, detached, and 2 more.

Unfortunately, due to merging #197, these legitimate errors were also hidden. Ideally, we should investigate why the generated output has compilation issues and resolve them, and then revert #197.

@jhugman
Copy link
Owner

jhugman commented Dec 27, 2024

Ideally, we should investigate why the generated output has compilation issues and resolve them, and then revert #197.

I am in favour of reverting #197. I don't think the library is yet mature enough to turn off typescript checking for generated code. If we would absolutely want to support turning it off, then we should have the user opt-out by adding something to the uniffi.toml config file.

@hassankhan
Copy link
Contributor Author

@jhugman I'm in favour of reverting it too, for now (or as you said, make it configurable)

With regards to this specific PR though, is there anything else you'd like to see?

@hassankhan hassankhan force-pushed the @hassankhan/fix-arraybuffer branch from 23e9c44 to 856d575 Compare December 27, 2024 19:36
@jhugman
Copy link
Owner

jhugman commented Dec 27, 2024

Sorry, my mistake.

I think switching type-checking back on in this PR, and fixing the resulting errors would be useful. If I understand your comment #0 correctly, this issue is fixing a tsc compile error.

I'm really sorry, I'm afk until 6 January, so can't really check out your PR and poke at it.

I'm going to ask @Johennes or @zzorba to have a look with you.

@hassankhan
Copy link
Contributor Author

If its all the same to you, I'd prefer to make a separate PR reverting #197, keep this PR as-is to fix this one specific class of TypeScript compilation errors, and make separate PRs for the other errors as I can get to them.

Prior to using `ArrayBufferLike`, TypeScript compilation errors in the generated output would occur:

```
src/generated/foobar.ts:1133:25 - error TS2345: Argument of type '(value: ArrayBuffer) => string' is not assignable to parameter of type 'StringLifter'.
  Types of parameters 'value' and 'bytes' are incompatible.
    Type 'Uint8Array<ArrayBufferLike>' is missing the following properties from type 'ArrayBuffer': maxByteLength, resizable, resize, detached, and 2 more.
```

Unfortunately, due to merging jhugman#197, these legitimate errors were also hidden. Ideally, we should investigate
why the generated output has compilation issues and resolve them, and then revert jhugman#197.
@hassankhan hassankhan force-pushed the @hassankhan/fix-arraybuffer branch from 856d575 to 2d24240 Compare January 1, 2025 02:07
@hassankhan hassankhan marked this pull request as ready for review January 1, 2025 02:07
@zzorba zzorba self-requested a review January 2, 2025 19:00
@Johennes
Copy link
Collaborator

Johennes commented Jan 3, 2025

I haven't observed these error myself so far. I suspect they require the consumer to use a certain minimum TS version?

Would it be possible to set up a test case that exposes the failure and verifies the fix?

@jhugman
Copy link
Owner

jhugman commented Jan 6, 2025

I'm sorry to step on your toes @Johennes .

Looking at this PR, it seems harmless, and only part of @hassankhan 's larger #198.

readArrayBuffer and writeArrayBuffer are only used internally, and not exposed to user space (and so definitely non-trivial to test).

The writeArrayBuffer is only used in FfiConverterString and we can guarantee that this only gets called with an ArrayBuffer.

readArrayBuffer is used in the FfiConverterArrayBuffer, and we can definitely say an ArrayBuffer is returned. We test this in test_coverall2.ts. I am confused why FfiConverterArrayBuffer.read is still compiling, but that is more down to my lack of typescript understanding than anything.

@Johennes What say we approve this, so @hassankhan can regain some of the excellent momentum he had before the break?

@jhugman jhugman requested a review from Johennes January 6, 2025 14:24
@Johennes
Copy link
Collaborator

Johennes commented Jan 6, 2025

Ah, ok. This sounds totally fine then. Sorry, I didn't dig into the usages myself. I was just being overly cautious because of the prior TS breakage we had. 😅

@Johennes Johennes merged commit 8cc0509 into jhugman:main Jan 6, 2025
18 checks passed
@hassankhan hassankhan deleted the @hassankhan/fix-arraybuffer branch January 6, 2025 15:41
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.

3 participants