-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Quote names containing colons in generated .d.ts #4488
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
Conversation
| readonly __wbindgen_export_0: WebAssembly.Table; | ||
| readonly "__wbgt__reference_test::example_test": (a: number) => void; | ||
| readonly __wbgtest_cov_dump: () => [number, number]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is visible here :) __wbgt__reference_test::example_test is quoted now. 🎉
guybedford
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a changelog entry.
|
This still seems to be failing CI unfortunately. |
|
Finally just looks like the fixtures still just need to be rerun with a bless flag to update the expected output. |
Sorry, I'm not totally sure what that means or (if it is something I'm supposed to do) how to achieve it. Is there a command I can try running? |
|
Yes, you can run: |
daxpedda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT we still need a test for this. Otherwise this is LGTM.
(if you can't get the reference tests to align dw about it, we can fix that before merging)
|
|
||
| export interface InitOutput { | ||
| readonly memory: WebAssembly.Memory; | ||
| readonly "__wbgt__wasm_export_colon_reftest::colon_test": (a: number) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this sufficient test of the feature? Previously, this .d.ts file would have caused a syntax error because there would be no quotes on this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! Could you take another look, @daxpedda?
|
changelog needs an update, it's currently in the release log for version 0.2.104. |
Fixes #4449.
The change to crates/cli/tests/reference/wasm-export-types.rs reproduces the issue in a test. The change to wasm2es6js.rs fixes it.