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 JSDoc annotations to ensure enums type check even without .d.ts file #4157

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

RunDevelopment
Copy link
Contributor

This is a partial fix for #3864.

I added additional JSDoc annotations to ensure that enums type check even without explicit type definition (.d.ts file).

C-style enums get an @enum {1|2|3|4} annotation which (1) marks the frozen enum object as an enum and (2) ensures that type annotations properly resolve. A comment in #3864 suggested using @enum {number}, but I defined the enum as the union of all possible numeric values instead to make the type more specific.

String enums get @typedef {"a" | "b" | "c"} StringEnum which is exactly the same as TS type StringEnum = "a" | "b" | "c". This means that string enums have exact type definitions with and without a .d.ts file. I also added @type {StringEnum[]} to arrays of string enum variants to ensure that the internal uses of those arrays don't cause type errors.

With those 2 changes, all of enum.js type checks without its .d.ts file.

One remaining problem is that I don't know how to test for the lack of type errors. While making this PR, I just added a // @ts-check at the top of enum.js so I could see type errors in my IDE. Any ideas how I could add tests for this?

@daxpedda daxpedda added needs review Needs a review by a maintainer. waiting for author Waiting for author to respond labels Oct 9, 2024
@daxpedda
Copy link
Collaborator

@RunDevelopment if you rebase this PR, I'm happy to give it a review.

One remaining problem is that I don't know how to test for the lack of type errors. While making this PR, I just added a // @ts-check at the top of enum.js so I could see type errors in my IDE. Any ideas how I could add tests for this?

Unfortunately not.
I have almost no experience with JS/TS linting, but ideally all our output testing should be covered by them.

@daxpedda daxpedda removed the needs review Needs a review by a maintainer. label Oct 13, 2024
@RunDevelopment
Copy link
Contributor Author

Rebased. Thank you for reviewing!

crates/cli/tests/reference/enums.js Outdated Show resolved Hide resolved
crates/cli/tests/reference/enums.js Outdated Show resolved Hide resolved
crates/cli/tests/reference/enums.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Contributor Author

I've been think about this some more, and I think I'm going to remove the string enum JSDoc comments from this PR. This goal of this PR was to make sure that users of the generated JS don't have type errors, but the string enum changes are all about internal type errors (which don't affect users).

So I agree with the comment that the string enum stuff belongs in #4189, although I'll make a separate PR for it after #4189.

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.

LGTM!
Missing a changelog entry.

@RunDevelopment
Copy link
Contributor Author

Done, although I'm not sure whether the explanation I picked for this change is good...

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.

Would appreciate if you could do the rebase!

@RunDevelopment
Copy link
Contributor Author

Sure thing!

@daxpedda daxpedda merged commit 76776ef into rustwasm:main Oct 16, 2024
41 checks passed
@RunDevelopment RunDevelopment deleted the enum-type-check-without-dts branch October 16, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants