Fix optional arguments in TypeScript

[dbrgn]

For a function like this:

#[wasm_bindgen]
pub fn opt_fn(_a: Option<i32>) -> Option<i32> {
    None
}

...the currently generated typescript looks like this:

/**
* @param {number | undefined} _a
* @returns {number | undefined}
*/
export function opt_fn(_a: number | undefined): number | undefined;

Support for undefined in the parameter type was added in #1201. However, while type number | undefined is technically optional at runtime, typechecking tools will still consider the parameter to be required. (CC @rhysd, please correct me if this information is actually incorrect.)

Instead, the TypeScript should look like this, with a ? appended to the parameter name:

/**
* @param {number | undefined} _a
* @returns {number | undefined}
*/
export function opt_fn(_a?: number): number | undefined;

Reference: https://www.typescriptlang.org/docs/handbook/functions.html#optional-and-default-parameters

This PR should actually fix #1129.

Some notes: I also adjusted crates/typescript-tests/src/opt_args_and_ret.ts, but I could not find out whether and where this file is actually used. I think it's dead code, but I might be wrong.

[dbrgn]

It is used for local and CI tests.

I assumed so, but when I run those tests (with run.sh) with bad TS type declarations, nothing fails...

[c410-f3r]

Nice work!

Some notes: I also adjusted crates/typescript-tests/src/opt_args_and_ret.ts, but I could not find out whether and where this file is actually used. I think it's dead code, but I might be wrong.

It is used for local and CI tests.

[c410-f3r]

Should the names of these methods be changed to new_(required|optional)?

[dbrgn]

Are there any guidelines / recommendations on this topic? I couldn't find anything in https://github.com/rust-dev-tools/fmt-rfcs/.

I personally prefer the shorter version, it's clear from the signature that it's a constructor.

[c410-f3r]

I assumed so, but when I run those tests (with run.sh) with bad TS type declarations, nothing fails...

Maybe because optional arguments are automatically coerced into undefined? For example, this and this have the same output.

[alexcrichton]

Thanks for this @dbrgn! In addition to what @rhysd is mentioning, could the test in in crates/typescript-tests be updated with various flavors of optional arguments to make sure this works? (e.g. some methods, some cases about mixed optional arguments, etc)

[dbrgn]

PR updated!

@alexcrichton I added some tests, but note that the tests are a bit flaky with regards to ensuring correct output for optional and/or omittable parameters. I found several combinations that pass the test but are not actually correct. For example, this works:

// typescript_tests.d.ts
export function opt_fn_mixed(_a: number | undefined, _b: number, _c?: number): number | undefined;

// opt_args_and_ret.ts
const opt_fn_mixed: (a: number, b: number, c: number) => number | undefined = wbg.opt_fn_mixed;

The reason is that number is a type subset of number | undefined, and narrowing the set of allowed types is always possible.

I think a test function or macro that asserts the correctly generated TS output string/file would be the better approach than using TypeScript's type system for verification. But that's out of scope for this PR.

[alexcrichton]

Ok no worries, this looks good to me! Sorry for the delay in reviewing :(

I'm fine testing more in further PRs as well, this certainly looks good enough for now! @dbrgn want to open a follow-up issue with your thoughts on testing?