Unnecessary Typescript definitions are created for web_sys string enums

[davids-work]

Describe the Bug

After updating to wasm_bindgen 0.2.94 / web_sys 0.3.71, I noticed that the generated Typescript definition file created by wasm-pack contained definitions for a bunch of string enums stemming from web_sys, e.g.

/**
 *The `MediaSourceReadyState` enum.
 *
 **This API requires the following crate features to be activated: `MediaSourceReadyState`*
 */
export type MediaSourceReadyState = "closed" | "open" | "ended";

These were not there with wasm_bindgen 0.2.93.

Steps to Reproduce

Cargo.toml:

[package]
name = "wasm-bindgen-unnecessary-bindings"
version = "0.1.0"
edition = "2021"

[lib]
crate-type = ["cdylib"]

[dependencies]
wasm-bindgen = { version = "0.2.94" }

[dependencies.web-sys]
version = "0.3.71"
features = [
    "console",
    "MediaSourceReadyState",
]

src/lib.rs

use wasm_bindgen::prelude::wasm_bindgen;
use web_sys::{console::info, js_sys::Array};

// Note how MediaSourceReadyState is not used anywhere!

#[wasm_bindgen]
pub fn foo() {
    // Just to create some reference to web_sys so that it's actually pulled in
    info(&Array::new());
}

Build: wasm-pack build --release --target web

Expected Behavior

Definitions are only generated for types that are part of my crate's interface

Actual Behavior

Definitions are generated for the web_sys::MediaSourceReadyState enum despite it not being part of my library at all.

More string enum definitions are generated if other web_sys features are enabled

[daxpedda]

Stemming from #4147.

I didn't check yet, but this might justify a yank.
Cc @RunDevelopment.

[RunDevelopment]

I reproduced the issue and technically it is working as intended.

(Sorry in advance for the long comment. I tried to make it as short as possible, but it still ended up like this.)

How web-sys defines string enums

web-sys main purpose is define Rust bindings for JS web APIs. Since JS APIs are imported into Rust, nothing is exported on the JS/TS side, so .d.ts don't include these types. Example:

// web-sys code
#[wasm_bindgen]
extern "C" {
    # [wasm_bindgen (extends = :: js_sys :: Object , js_name = URL , typescript_type = "URL")]
    pub type Url;
    # [wasm_bindgen (structural , method , getter , js_class = "URL" , js_name = href)]
    pub fn href(this: &Url) -> String;
    # [wasm_bindgen (structural , method , setter , js_class = "URL" , js_name = href)]
    pub fn set_href(this: &Url, value: &str);
    // ...
}

// user code
#[wasm_bindgen]
pub fn get_url() -> web_sys::Url {
    web_sys::Url::new("https://example.com").unwrap()
}

// no type definition for `URL`
export function get_url(): URL;

However, this is not true for string enums. Example:

// web-sys code
#[wasm_bindgen]
pub enum MediaSourceReadyState {
    Closed = "closed",
    Open = "open",
    Ended = "ended",
}

This is the same code as users would write to export a string enum type (see #2153), so the string enum is included in the generated .d.ts file. This is behavior is consistent with exporting C-style enums and structs, which is why I say it is technically correct.

Intended behavior

However, it's clearly not intended for web-sys, so how do we fix this? I think we should first clarify what the intended behavior is.

As I see it, there are basically 4 cases:

1. Internal usage of external type. I.e. in this issue, MediaSourceReadyState was likely used by some bindings the user used.

   // web-sys code
   #[wasm_bindgen]
   pub enum MediaSourceReadyState { Closed = "closed", ... }
   
   // user code
   #[wasm_bindgen]
   pub fn foo() {
       so_something(web_sys::MediaSourceReadyState::Closed);
   }

2. Public usage of external type.

   // web-sys code
   #[wasm_bindgen]
   pub enum MediaSourceReadyState { Closed = "closed", ... }
   
   // user code
   #[wasm_bindgen]
   pub fn foo(arg: web_sys::MediaSourceReadyState) { ... }

3. Internal usage of user-defined type.

   // all user code
   #[wasm_bindgen]
   pub enum UserState { Closed = "closed", ... }
   
   #[wasm_bindgen]
   pub fn foo() {
       so_something(UserState::Closed);
   }

4. Public usage of user-defined type.

   // all user code
   #[wasm_bindgen]
   pub enum UserState { Closed = "closed", ... }
   
   #[wasm_bindgen]
   pub fn foo(arg: UserState) { ... }

I think the intended behavior for cases 1 and 4 is clear:

// case 1
// no MediaSourceReadyState
export function foo();

// case 4
export type UserState = "closed" | ...;
export function foo(arg: UserState);

But case 2 is not so clear. The MediaSourceReadyState type has to be defined, but should it be exported? I mean, the user never said that MediaSourceReadyState should be part of the public TypeScript API of their npm package.

// case 2 (no export)
type MediaSourceReadyState = "closed" | ...
export function foo(arg: MediaSourceReadyState);

// case 2 (export)
export type MediaSourceReadyState = "closed" | ...
export function foo(arg: MediaSourceReadyState);

Similarly, in case 3, the string enum is explicitly defined and marked as pub by the user, so should we not export it because it's only used internally?

// case 3 (no export or definition)
// no UserState
export function foo();

// case 3 (export)
export type UserState = "closed" | ...;
export function foo();

How should it behave?

Private string enums

User code also has some additional complexity, because we allow non-public string enums. E.g. this is the current behavior for a non-public string enum that is publicly used:

// all user code
#[wasm_bindgen]
/* no pub */ enum UserState { Closed = "closed", ... }
#[wasm_bindgen]
pub fn foo(arg: UserState) { ... }

type UserState = "closed" | ...;
export function foo(arg: UserState);

I think the behavior should be:

• private string enum is used publicly -> define the type but don't export it (current behavior).
• private string enum is used internally -> don't define the type.

[daxpedda]

Is there any use-case for defining string enums in TS that aren't exported?

---

While its interesting to hash all this out, I'm unsure how to proceed right now.
I don't believe the TS output has any stability guarantees (which is states/documented nowhere), but there are many users already relying on it.
Until we come to a conclusion on all these questions and implement them we should not expose types to users which will be removed later on.
That said, even fixing it is already a breaking change, so maybe we could ignore all this and just slap skip_typescript on web-sys enums until we resolve the finer points.

My plan right now is:

• Revert the TS part of Improved support for string enums #4147.
• Release a new version (which I have to do anyway today).
• Maybe yank the current one.

WDYT?

[RunDevelopment]

Is there any use-case for defining string enums in TS that aren't exported?

Yes, for typing. This is the private string enum used publicly case. Or did I misunderstand your question?

maybe we could ignore all this and just slap skip_typescript on web-sys enums until we resolve the finer points.

I would prefer this as a temporary solution, because it means that #2153 remains fixed. Not having TS types for web-sys enums was also the behavior before #4147 and people didn't report that specifically.

Thoughts?

[daxpedda]

Is there any use-case for defining string enums in TS that aren't exported?

Yes, for typing. This is the private string enum used publicly case. Or did I misunderstand your question?

My understanding is that the generated TS file is only used to import it, so I don't understand what not exported types would be good for.

Unfortunately I'm clueless about TS and the ecosystem, so I might be just missing something obvious.

maybe we could ignore all this and just slap skip_typescript on web-sys enums until we resolve the finer points.

I would prefer this as a temporary solution, because it means that #2153 remains fixed. Not having TS types for web-sys enums was also the behavior before #4147 and people didn't report that specifically.

Thoughts?

My concern is that there other types that are exposed now that should not. E.g. private enum types are exported, maybe we don't want that.

But agreed on the latter part, if everything else would be in order using skip_typescript would solve the issue.

[RunDevelopment]

My understanding is that the generated TS file is only used to import it, so I don't understand what not exported types would be good for.

Sorry for the confusion. My code example for "private string enum used publicly" had an error. It should have been:

type UserState = "closed" | ...;
export function foo(arg: UserState);

If UserState wasn't defined, there would be a type error, because arg: UserState would reference a non-existing type.

[daxpedda]

Sorry for the confusion.

No, I'm actually running on fumes here.

---

Unfortunately, after thinking about this more, I think #4173 won't work out after all.
Many crates actually have to specify custom bindings missing in web-sys or vendoring them to avoid --cfg=web_sys_unstable_apis, which won't have the skip_typescript on them.
This is among the other issues I pointed out above.

So I will have to go ahead with the original plan and revert #4147.

But please let this not discourage you from continuing to work on #2153 if you want to, I'm still happy to continue finding a solution and reviewing an implementation!

[RunDevelopment]

Many crates actually have to specify custom bindings

Ah, true.

So I will have to go ahead with the original plan and revert #4147.

No worries, please do.

I think just I'm going to make a version of #4147 that only exports the type if referenced elsewhere (in the next couple of days). This should be compatible (fixing this issue) while still resolving #2153, right?

[RunDevelopment]

No, I'm actually running on fumes here.

Sorry for causing some of our headaches and please take care of yourself.

[daxpedda]

No, I'm actually running on fumes here.

Sorry for causing some of our headaches and please take care of yourself.

All good, thank you for all the contributions and all the input you have given so far!