-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix primitive slice js #722
Fix primitive slice js #722
Conversation
}); | ||
|
||
test("Float64Vec", (t) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails on my machine:
slices-ts › Float64Vec
test/slices-ts.mjs:14
13: let data = Float64Vec.newFromOwned(input);
14: t.is(data.borrow(), input);
15: });
Difference (- actual, + expected):
[
- 2.26666497653787e-308,
+ 1,
2,
3,
4,
5,
]
› file://test/slices-ts.mjs:14:7
I'm going to open this up as ready for review despite the failing test, in the hope that a reviewer can point me out to where the byte interpretation goes wrong. I got as far as seeing that, the value |
Okay I think I realised the issue. The method ...
} finally {
functionCleanupArena.free();
} I didn't actually check if the js backend supports owned slices in parameters. I've got a better test now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me!
Thanks for adding new tests as well, very much appreciated.
Co-authored-by: Tyler K <ambiguousemails@proton.me>
This PR attempts to address #699. According to the generated
ts
files primitive slices are returned as anArray<number>
on the js side, but the generated js code looks like it's trying to return types ofFloat64Array
. I personally think it makes more sense to return typed arrays which have a perfectly ergonomic API and allow the caller to decide what to do with, but don't really want to challenge the existing implementation. Just checking that I understand it correctly. I had to add anArray.from
wrap to get that to work. However, it appears there is a bug in the actual interpretation of the raw bytes that causes the first byte to not be interpreted correctly, I tried to get to the bottom of it, but couldn't quickly figure it out, so I thought I'd open the PR, and solicit help. Does anyone know what might be going on, and why the wasm backed array seems to be incorrect?