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

[Task] Provide proper Typescript typing to WASM bindings #309

Closed
10 tasks
JelleMillenaar opened this issue Jul 13, 2021 · 4 comments
Closed
10 tasks

[Task] Provide proper Typescript typing to WASM bindings #309

JelleMillenaar opened this issue Jul 13, 2021 · 4 comments
Labels
Wasm Related to Wasm bindings. Becomes part of the Wasm changelog

Comments

@JelleMillenaar
Copy link
Collaborator

JelleMillenaar commented Jul 13, 2021

Description

Provide proper Typescript typing for the WASM bindings. At the moment the typing mostly are just any. This is preferably implemented in an automated fashion that is easy to maintain.

Motivation

Better Typescript support

To-do list

Create a task-specific to-do list. Please link PRs that match the To-do list item behind the item after it has been submitted.

Change checklist

Add an x to the boxes that are relevant to your changes, and delete any items that are not.

  • The feature or fix is implemented in Rust and across all bindings whereas possible.
  • The feature or fix has sufficient testing coverage
  • All tests and examples build and run locally as expected
  • Every piece of code has been document according to the documentation guidelines.
  • If conceptual documentation (mdbook) and examples highlighting the feature exist, they are properly updated.
  • If the feature is not currently documented, a documentation task Issue has been opened to address this.
@JelleMillenaar JelleMillenaar added the Wasm Related to Wasm bindings. Becomes part of the Wasm changelog label Jul 13, 2021
@m-renaud
Copy link
Contributor

m-renaud commented Sep 22, 2021

@JelleMillenaar Are you sure these aren't already properly typed? The following works locally for me, and changing any of the declared types to an invalid type results in a TypeScript type error:

import express from 'express';
import identity from '@iota/identity-wasm/node';
import { Document, KeyPair } from '@iota/identity-wasm/node';

const app = express();
const port = 3000;
app.get('/', (req, res) => {
  const key: KeyPair = new identity.KeyPair(identity.KeyType.Ed25519);

  const doc: Document = identity.Document.fromKeyPair(key);

  res.send(`Created ${doc}`);
});
app.listen(port, () => {
  console.log(`server is listening on ${port}`);
});

Eg. changing the key assignment to:

const key: Document = new identity.KeyPair(identity.KeyType.Ed25519);

results in the following error:

src/app.ts:8:9 - error TS2740: Type 'KeyPair' is missing the following properties from type 'Document': authentication, insertMethod, removeMethod, insertService, and 18 more.

8   const key: Document = new identity.KeyPair(identity.KeyType.Ed25519);
          ~~~

If there are specific bindings you know of that aren't typed I can add them.

@m-renaud
Copy link
Contributor

Taking a closer look it appears that it's the async functions that have the incorrect type (Promise<any>) instead of the more specific type. It looks like rustwasm/wasm-bindgen#2665 fixes this though, so we may just need to wait for the next wasm-bindgen version.

@cycraig
Copy link
Contributor

cycraig commented Sep 27, 2021

There are a few objects that are still converted to a generic JsValue/any in the bindings. Notably the Signature struct for the proof in WasmDocument and WasmDiffDocument.

With regards to async function types: unfortunately that PR does not fix the issue our side, as we are using future_to_promise to get around lifetime issues, which returns Promise<any>.

E.g.
Currently we use the following code for exporting publish_document:

#[wasm_bindgen(js_name = publishDocument)]
pub fn publish_document(&self, document: &JsValue) -> Result<Promise> {
  let document: IotaDocument = document.into_serde().wasm_result()?;
  let client: Rc<IotaClient> = self.client.clone();

  let promise: Promise = future_to_promise(async move {
    client
      .publish_document(&document)
      .await
      .wasm_result()
      .and_then(|receipt| JsValue::from_serde(&receipt).wasm_result())
  });

  Ok(promise)
}

This would need to be changed to take all parameters by value to get the correct type (Promise<Receipt>):

#[wasm_bindgen(js_name = publishDocument)]
pub async fn publish_document(self, document: WasmDocument) -> Result<WasmReceipt> {
  self.client
    .publish_document(&document.0)
    .await
    .map(WasmReceipt::from)
    .wasm_result()
}

However, I believe that would also change the ergonomics of its usage in Javascript, which is undesirable:

CURRENT: const receipt = client.publishDocument(doc);
  AFTER: const receipt = Client.publishDocument(client.clone(), doc.clone());

We are still exploring other options such as typescript_type annotations to get around these limitations but we're open to suggestions.

@cycraig
Copy link
Contributor

cycraig commented Nov 22, 2021

I ended up manually annotating the Typescript types which allows us to keep the same function signatures. Not ideal but seems like the simplest and cleanest solution right now.

E.g.

// Workaround for Typescript type annotations on async function returns.
#[wasm_bindgen]
extern "C" {
  #[wasm_bindgen(typescript_type = "Promise<Receipt>")]
  pub type PromiseReceipt;
}

#[wasm_bindgen(js_name = publishDocument)]
pub fn publish_document(&self, document: &WasmDocument) -> Result<PromiseReceipt> {
  let document: IotaDocument = document.0.clone();
  let client: Rc<IotaClient> = self.client.clone();

  let promise: Promise = future_to_promise(async move {
    client
      .publish_document(&document)
      .await
      .map(WasmReceipt)
      .map(Into::into)
      .wasm_result()
  });

  // WARNING: this does not validate the return type. Check carefully.
  Ok(promise.unchecked_into::<PromiseReceipt>())
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Development

No branches or pull requests

3 participants