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

Add export_type attribute to export string enums #4260

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RunDevelopment
Copy link
Contributor

Fixes #2153

This gives users control over whether the type of string enums is exported via a new export_type attribute exclusive for string enums.

I also added documentation on why the attribute exists and how it works. I intentionally kept the description of this PR short, because the docs should explain the behavior of the attribute. If any questions arise regarding the behavior/function of the attribute during the review process, I will see this as the docs being insufficient.

@RunDevelopment
Copy link
Contributor Author

wtf is going on here? CI fails with this message:

---- schema_hash_approval::schema_version stdout ----
thread 'schema_hash_approval::schema_version' panicked at crates/shared/src/schema_hash_approval.rs:15:5:
assertion `left == right` failed
  left: "11946883356319465460"
 right: "211103844299778814"

But the constant being assert!ed (APPROVED_SCHEMA_FILE_HASH) has the value "18290232323238297705". What is going on here?!

@RunDevelopment
Copy link
Contributor Author

One rebase later and the nature order is restored. My bad.

@daxpedda
Copy link
Collaborator

daxpedda commented Nov 11, 2024

So I had a different proposal a while ago. We might want to differentiate between imported and exported enums, which seems a bit strange considering JS doesn't actually have enums.

The idea is that we need to make it clear what types are actually exported into JS/TS by wasm-bindgen. Currently every struct and function marked with #[wasm_bindgen] is exported. Making special rules around enums, as is already the place with numeric enums, seems counter-productive and I would instead like to fix it at the root.

It seems to me that the case to have enums which we don't want to export is if we want to "import enums that already exist", e.g. in web-sys.

Here is my suggestion:

extern "C" {
    pub enum Foo {
        Bar = "Bar",
    }

This is an "imported" enum and will not be exported in JS/TS, but can still be correctly represented in JS/TSDoc and TS signatures as a type. This works quite well with web-sys as TS should recognize all these enums, but I don't know how this works exactly with #[wasm_bindgen(module = "...")] extern "C" { ... }.

Let me know what you think!

EDIT: I don't actually know if Rust or syn will allow us to parse something like this, which would also be important to figure out.

@RunDevelopment
Copy link
Contributor Author

Firstly, I would like to point out that your proposed solution would repeat #4163, so it's a breaking change.

I also don't like the idea of putting them into extern blocks, since enums aren't really externally defined. C-style and string enums essentially defined, in a sense, a primitive data type. So they are as externally defined as the concept of a 64-bit floating-pointer number is externally defined, IMO.

Currently every struct and function marked with #[wasm_bindgen] is exported. Making special rules around enums, as is already the place with numeric enums, seems counter-productive and I would instead like to fix it at the root.

What special rules? Right now, only string enums are inconsistent since they aren't exported. C-style enums being exported is consistent with structs being exported.


How about this idea: the main usability problem here is that WBG has to decide for the user whether a string enum (or any other thing for that matter) is exported or imported. WBG currently decides with via extern blocks, and you proposed to reuse that mechanism. However, what if we allow users to just explicitly tell us whether something is exported (= should be part of the public API of the generated JS)?

Maybe something like this:

#[wasm_bindgen]
pub enum Foo { A = "a" }
#[wasm_bindgen]
pub enum Bar { B = "b" }

#[wasm_bindgen(export)] { // export section
   Foo = true,
   Bar = false,
}

// short-hand for `Baz = true` in the export section
#[wasm_bindgen(export)]
pub enum Baz { C = "c" }

This would give users very direct control over the API of their JS package.

But the main reason I want something like this is that it gives users control over third-party types. Even if a user wants to export a string enum defined by a third-party library, they currently just can't.

As I see it, the goal is to give users control over the public API of their JS packages, so why not be explicit about it instead of going through the notion of imported/exported types?

This would also allow us to make guesses. E.g. if a string enum is used by a publicly exported function, the string enum should probably be publicly exported too. If we guessed wrong and the user wanted to keep it internal, they can then make it so.

What do you think about this approach?

@daxpedda
Copy link
Collaborator

Firstly, I would like to point out that your proposed solution would repeat #4163, so it's a breaking change.

The idea is to let web-sys move to this new pattern as well, avoiding this issue.

I also don't like the idea of putting them into extern blocks, since enums aren't really externally defined. C-style and string enums essentially defined, in a sense, a primitive data type. So they are as externally defined as the concept of a 64-bit floating-pointer number is externally defined, IMO.

I agree, this is a compromise in lack of a better solution.

Currently every struct and function marked with #[wasm_bindgen] is exported. Making special rules around enums, as is already the place with numeric enums, seems counter-productive and I would instead like to fix it at the root.

What special rules? Right now, only string enums are inconsistent since they aren't exported. C-style enums being exported is consistent with structs being exported.

I meant that the proposed change of this PR would not make it consistent with the rest. As you point out, numeric enums are already consistent.

However, what if we allow users to just explicitly tell us whether something is exported (= should be part of the public API of the generated JS)?

Something like this would be indeed great. However while we can do this just fine with string enums, because they aren't exported, we can't do it for everything else. So I would rather move to something like this consistently in the next breaking change.

Just dropping some thoughts about this topic: Rust itself has no mechanism for native libraries on how to handle what is exported and what is not from dependencies. So I see this more as a Rust problem. In the future, the component model would address this quite well. So I would argue instead of coming up with more out-of-the-ordinary solutions for this problem, moving to the component model would be more appropriate.

@RunDevelopment
Copy link
Contributor Author

So I would rather move to something like this consistently in the next breaking change.

Oh, then I think I miscommunicated a bit. My goal for this PR was to have a solution without a breaking change.

But you are correct that the export_type attribute would only serve as a temporary solution and be removed when we implement a proper solution in the next major-ish version.

I also thought of a different solution to this problem based on a heuristic and without an attribute. We could just assume that if a string enum is used in an exported function, the string enum is probably exported as well. This wouldn't remove the weirdness of string enums, but should correctly export (and not export) string enums in the majority of use cases.

Rust itself has no mechanism for native libraries on how to handle what is exported and what is not from dependencies.

Can't you just pub use dep; to re-export a dependency? Not that this would help us, since WBG exports are determined by which code is compiled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript definition not generated for string enums
2 participants