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

Expose traits and its methods via #[napi] directly when supported upstream #987

Closed
Xanewok opened this issue May 27, 2024 · 5 comments · Fixed by #1120
Closed

Expose traits and its methods via #[napi] directly when supported upstream #987

Xanewok opened this issue May 27, 2024 · 5 comments · Fixed by #1120
Assignees

Comments

@Xanewok
Copy link
Contributor

Xanewok commented May 27, 2024

Context

While implementing #963 we found that it's impossible to define in terms of #[napi] something along the lines of

export interface MyInterface {
  some(): string;
  methods(): string;

or use a better suited way to expose Rust trait implementations to the TypeScript side.

Alternatives

One idea explored to provide a shim of

#[napi]
struct MyTraitInterface(Box<dyn MyTrait>);

but this forces us to provide explicit conversion methods for the N-API-exposed types that wrap a type implementing a Rust trait into this shim, making it less ergonomic and potentially more costly due to cloning involved whenever we needed to access the trait methods.

Solution

This is probably blocked on napi-rs/napi-rs#1164, which seems to be planned. The solution would be to:

  • expose the trait methods via #[napi] directly rather than having to introduce our own custom interface in the .ts file;
  • being able to conveniently expose select methods for a given #[napi] decorated trait interface or
  • use planned class inheritance feature to implement and expose that for us.
@OmarTawfik
Copy link
Contributor

OmarTawfik commented May 30, 2024

@Xanewok I was going to investigate into having separate TypeScript wrappers that wrap the NAPI objects, instead of defining the public API entirely via NAPI, as it is currently the most ergonomic way to leverage all TypeScript language features, allowing us to use things like iterators, symbols, discriminated unions, and also interfaces.

I will look into having a more concrete proposal for us to review/discuss. Please let me know if you have any suggestions/concerns.

@Xanewok
Copy link
Contributor Author

Xanewok commented Jun 5, 2024

It's hard to suggest or voice concerns because the proposed solution seems to be a bit on the vague side for now; I'm sure there might be some unforeseen issues there as well.

Maybe one thing to keep in mind is that it seems like a topic that would require a bit of research and following a rabbit hole and it seems to be overlapping with what @AntonyBlakey's doing for the applied research and WASM instead of NAPI, if I understand correctly.

As such, I'd warn against spending more time here as the underlying problem might be obsolete soon (assuming we can manage it for the v1 release).

@OmarTawfik
Copy link
Contributor

I think even with WASI/WIT, which will change the underlying FFI objects, we will still need to have our own wrappers if we are going to add convenient/native APIs. One example is adding JS iterators to query results to enable writing for loops naturally. But there are many others.

But I agree that we need to make sure that doesn't conflict with the WASM research.

@Xanewok
Copy link
Contributor Author

Xanewok commented Jun 5, 2024

I did some initial investigation into implementing the JS iterators via #[napi(iterator)] and #[napi] impl Generator for ... but it seems that this will not be possible without capturing the env ourselves - napi's Generator::next does not allow injecting the env variable as it's done for the methods according to: https://github.com/napi-rs/napi-rs/blob/80d9d87ef9e473ae51f841dbd5ee9e9800a0fcd9/crates/napi/src/bindgen_runtime/iterator.rs#L283C9-L283C13 (interestingly enough, it is propagated to Generator::throw).

One approach would be to control where the types are constructed, copy the and store the environment on the premise that the wrapper will not move between JS contexts/environments and re-inject it in the Generator implementations.

@AntonyBlakey
Copy link
Contributor

AntonyBlakey commented Jun 5, 2024

If it's at all possible we should hold off on this. WIT is going to change all this, and soon I think. We will definitely want to do some wrapping to provide e.g. iterator types in each client language.

@OmarTawfik OmarTawfik moved this to Todo in Slang - Backlog Jul 26, 2024
@OmarTawfik OmarTawfik self-assigned this Oct 8, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 8, 2024
See the added changesets for the specific API changes/additions.

Additionally, this PR:

- deprecates/removes NAPI build/pipeline.
- adds `jco` as a sub-module to the repo.
- deletes the platform-specific packages, as we now just need one.

Closes #410
Closes #570
Closes #573
Closes #816
Closes #987
@github-project-automation github-project-automation bot moved this from Todo to Done in Slang - Backlog Oct 8, 2024
@OmarTawfik OmarTawfik moved this from ⏳ Todo to ✅ Done in Slang - 2024 H2 Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants