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

Generate TypeScript return types for async functions #2665

Merged
merged 9 commits into from
Aug 26, 2021

Conversation

trevyn
Copy link
Contributor

@trevyn trevyn commented Aug 22, 2021

Threads through an asyncness bool and an "inner" return value type descriptor for functions, then generates Promise<...> return types for async functions.

Fixes #2394

@trevyn
Copy link
Contributor Author

trevyn commented Aug 22, 2021

Per the docs, returning Result only allows Result<T, JsValue>:

The Result type can be returned from functions exported to JS as well as closures in Rust. Only Result<T, JsValue> is supported where T can be converted to JS.

These tests are now failing, whereas before the non-JsValue error types were being hidden by the return type elision for async functions:

#[wasm_bindgen]
pub async fn async_return_7() -> Result<AsyncCustomReturn, u32> {
    Ok(AsyncCustomReturn { val: 7 })
}

#[wasm_bindgen]
pub async fn async_return_8() -> Result<AsyncCustomReturn, AsyncCustomReturn> {
    Ok(AsyncCustomReturn { val: 8 })
}

#[wasm_bindgen]
pub async fn async_throw() -> Result<(), js_sys::Error> {
    Err(js_sys::Error::new("async message"))
}
the trait `wasm_bindgen::describe::WasmDescribe` is not implemented for
`Result<AsyncCustomReturn, u32>`

help: the following implementations were found:
<Result<T, wasm_bindgen::JsValue> as wasm_bindgen::describe::WasmDescribe>

And indeed, they don't work when removing the async.

@alexcrichton
Copy link
Contributor

Thanks for the PR! The implementation here looks pretty reasonable to me. I think it's fine to update those tests as well, yeah.

Can you add some specific tests for this new feature though? Ideally there'd be reference tests to see the output, tests in the main tests suite to exercise that things work, and tests in the typescript area to ensure that everything typechecks as expected.

@trevyn
Copy link
Contributor Author

trevyn commented Aug 25, 2021

@alexcrichton Thanks for the review!

I added a test in the typescript-tests area.

I don't see any existing reference tests at all for .d.ts output; am I missing something, or shall I add reference tests for .d.ts to the cli/tests/reference section? (Right now they just test the .js and .wat output)

Also, the only intended output change is to the .d.ts files, so there shouldn't be any new behavior outside of TypeScript, and existing async fn tests should still apply. Not sure if there was something specific you wanted me to add in the main tests suite?

@alexcrichton alexcrichton merged commit 965b88c into rustwasm:master Aug 26, 2021
@alexcrichton
Copy link
Contributor

Nah this looks good to me, thanks!

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.

async functions don't have proper type signatures in Typescript
2 participants