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 enum types for function parameters and return types #3647

Merged

Conversation

ChristianIvicevic
Copy link
Contributor

@ChristianIvicevic ChristianIvicevic commented Oct 7, 2023

The adjusted typescript-tests unfortunately aren't great to assert the correctness of the generated types because a generated (a: number) => void could still be assigned to a (a: Foo) => void as the enum values in Foo are merely numbers. Therefore the tests currently only asserts that something structurally compatible is generated, but there is no check whether the expected enum is referenced. Classic TypeScript...

Fixes #2154

@ChristianIvicevic ChristianIvicevic force-pushed the 2154-enums-in-function-signatures branch from 5199ea0 to 5a3cfa4 Compare October 7, 2023 03:34
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell our testing suite actually doesn't verify the TS output against something we write.

In any case, LGTM!

Could you add a changelog entry?

crates/backend/src/codegen.rs Outdated Show resolved Hide resolved
crates/typescript-tests/src/enums.ts Outdated Show resolved Hide resolved
@daxpedda daxpedda merged commit 5b42ffc into rustwasm:main Oct 7, 2023
25 checks passed
@ChristianIvicevic
Copy link
Contributor Author

@daxpedda Please allow me some questions regarding your release process and testing/using pre-release versions of the crates.

  1. What is your typical release cadence and is there any plans on when to release next?
  2. I wanted to give my new changes a try and replaced my local dependency in my project with a path to my fork
wasm-bindgen = { git = "https://github.com/ChristianIvicevic/wasm-bindgen.git", branch = "2154-enums-in-function-signatures" }

Running wasm-pack yielded an error

thread 'main' panicked at 'unknown descriptor: 66', /Users/christian.ivicevic/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasm-bindgen-cli-support-0.2.87/src/descriptor.rs:164:22

This made me realize that there might be some mismatch with my globally installed version of wasm-pack. Is this expected? How can I properly use my fork until a proper release is available?

@daxpedda
Copy link
Collaborator

daxpedda commented Oct 7, 2023

  1. What is your typical release cadence and is there any plans on when to release next?

See #3530.

  1. I wanted to give my new changes a try and replaced my local dependency in my project with a path to my fork

You can't without also installing wasm-bindgen-cli from the same custom fork.
cargo install wasm-bindgen-cli --git https://github.com/ChristianIvicevic/wasm-bindgen --branch 2154-enums-in-function-signatures -f (not sure if that -f is needed)

I'm not sure exactly how wasm-pack functions, but if it directly depends on a wasm-bindgen-* crate, it might need to be forked as well.

@ChristianIvicevic
Copy link
Contributor Author

I'll give that manual override a try and if it doesn't work I'll wait until either the user in the linked issue has opened a release PR; otherwise, I'll look into it myself 😅 Thanks for pointing me to that issue.

@ChristianIvicevic
Copy link
Contributor Author

Alright, your proposal to override the globally installed CLI (even I didn't had it installed previously) has worked and the output is correct. I'll stick with that until an official release has been pushed.

Thanks again for the support.

@Liamolucko
Copy link
Collaborator

Running wasm-pack yielded an error


thread 'main' panicked at 'unknown descriptor: 66', /Users/christian.ivicevic/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasm-bindgen-cli-support-0.2.87/src/descriptor.rs:164:22

Oh - this PR should've bumped the schema version (in wasm-bindgen-shared), since adding the name field to Descriptor::Enum means that wasm-bindgen's now incompatible with the previous version.

This is why you got this cryptic error message rather than a proper 'version mismatch' error.

@daxpedda
Copy link
Collaborator

daxpedda commented Oct 9, 2023

Oh - this PR should've bumped the schema version (in wasm-bindgen-shared), [..]

I'm not totally familiar with how this works. I know that when we bump the version for a new release this will get a new schema version anyway.

As far as I understand it though, this PR didn't update the shared/lib.rs file, which is why the schema version was not bumped. Is there some other way to update the schema version or how is this supposed to be done?

@Liamolucko
Copy link
Collaborator

Liamolucko commented Oct 11, 2023

I'm not totally familiar with how this works. I know that when we bump the version for a new release this will get a new schema version anyway.

No, it's done manually. In principle, it's possible for the interface between the macro and the CLI to stay the same between two versions of wasm-bindgen, in which case the schema version shouldn't be bumped: it should be left as the previous version of wasm-bindgen.

Then you would be able to freely use, for example, version 0.2.87 of the CLI with version 0.2.88 of the macro. However since changes to the interface between the macro and CLI are very common, this hasn't happened in a long time - not since I've been involved, at the very least.

EDIT: never mind, this actually happened in 0.2.86! It still had had a schema version of 0.2.85, and so was compatible with the previous CLI/macro. There weren't any changes to the interface between 0.2.86 and 0.2.87 either, so it could have theoretically continued using 0.2.85 as well, but it got bumped anyway.

As far as I understand it though, this PR didn't update the shared/lib.rs file, which is why the schema version was not bumped. Is there some other way to update the schema version or how is this supposed to be done?

Yeah, in principle the schema version is supposed to just keep track of changes to the schema in that file (hence the name). But since the version mismatch system is built on top of it, it's morphed into a general way of keeping track of changes in the interface between the macro and the CLI.

@daxpedda
Copy link
Collaborator

daxpedda commented Oct 11, 2023

Thanks, this makes sense to me now!

So how exactly do we bump the schema version if nothing changes in shared/lib.rs? Do we just bump this:

pub const SCHEMA_VERSION: &str = "0.2.87";

It's a bit weird to me to bump the version there without bumping the version in wasm-bindgen in general.

@Liamolucko
Copy link
Collaborator

So how exactly to we bump the schema version if nothing changes in shared/lib.rs? Do we just bump this:

pub const SCHEMA_VERSION: &str = "0.2.87";

Yes, and then update the hash.

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.

Typescript enum definitions are not used in function signatures
3 participants