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

Generate Typescript documentation from wit files #1157

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

Conversation

mjoerussell
Copy link

This PR uses the jco types command to generate Typescript definitions for the wasm crates instead of jco transpile. The reason is that jco transpile generates ts definitions by decoding WASM component binaries, whereas jco types parses .wit files directly and uses that information. This means that the Resolve instance created during types has access to the type documentation info, and the one created during transpile does not.

I've modified infra check npm with a new step which calls jco types and handles the resulting .d.ts files. When calling jco transpile we're now passing the option --no-typescript to avoid generating conflicting typescript outputs.

These changes depend on NomicFoundation/jco#1, which augments the types command to accept the same configuration file that we pass to transpile. Without this, the codegen in types will not be able to generate Typescript enums for Rust enums, or any other customization that we want.

@mjoerussell mjoerussell self-assigned this Nov 22, 2024
Copy link

changeset-bot bot commented Nov 22, 2024

⚠️ No Changeset found

Latest commit: d5c365c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mjoerussell
Copy link
Author

The core changes are done, but I still need to debug an issue with the npm tests. After these changes Jest is not able to resolve the module imports, even though infra check still succeeds. I'll keep looking and see if there's a deeper issue.

… .d.ts files using the `jco types` command.

This command is better for our use case than `jco transpile` because it generates the declarations directly from the source .wit files, whereas `jco transpile` uses the WASM binaries to generate them. This means that `jco types` has access to the documentation that's written in the wit interfaces, and `jco transpile` does not.

Since we're generating the interfaces in a separate step, I'm disabling typescript in the original `jco transpile` step. This is just a precaution to avoid confusion or possible conflicting declaration files interferring with each other.
…is correctly propagating through the build process.
…rpose of this is so that both processes use the same `CodegenFileSystem` instance. If they used separate instances, then the files generated in one function would get deleted by the next.
@mjoerussell mjoerussell marked this pull request as ready for review November 25, 2024 18:18
@mjoerussell mjoerussell requested a review from a team as a code owner November 25, 2024 18:18
Cargo.toml Outdated
@@ -151,7 +151,8 @@ thiserror = { version = "1.0.68" }
toml = { version = "0.8.19" }
trybuild = { version = "1.0.101" }
url = { version = "2.4.1", features = ["serde"] }
wasm-tools = { version = "1.216.0" }
wasm-tools = { version = "1.202.0" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this downgrade intended? not sure I understand the reason.
Also, nit blank lines.

Copy link
Author

Choose a reason for hiding this comment

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

No, this was leftover from a different change I was testing, which involved bringing in a local version wasm-tools in order to make some changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Cargo.lock is left still.

Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

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

Left a few questions.

Copy link
Collaborator

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

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

I think there is still one remaining issue, but otherwise, LGTM.

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.

2 participants