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

Update TS bindings output for variants #1180

Merged
merged 16 commits into from
Dec 12, 2024

Conversation

mjoerussell
Copy link
Contributor

@mjoerussell mjoerussell commented Dec 4, 2024

Updates our JCO submodule to include the changes in NomicFoundation/jco#5. Includes one other change to fully integrate these changes, which is to change NodeVariant to NodeType in codegen/../cst/index.mts so that the expected imports align with the new generated exports.

Closes #1166
Closes #1171

@mjoerussell mjoerussell requested review from a team as code owners December 4, 2024 22:21
Copy link

changeset-bot bot commented Dec 4, 2024

⚠️ No Changeset found

Latest commit: 021604c

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
Contributor Author

This is ready to review, but there are a couple small things that need to be fixed before it'll be ready to merge:

  1. The dependent PRs need to be merged
  2. Once Update bindings for variants and output documentation for resources jco#5 is merged, the submodule version in this branch should be updated again
  3. There is a broken npm test that I think is caused by a bug in ts-jest. The issue is that in the tests NodeType is always undefined. I'm working on diagnosing this now.

@mjoerussell
Copy link
Contributor Author

I've created a PR to fix the bug that's blocking this issue: NomicFoundation/jco#7. Once this is merged (& this PR has the updated version) then everything should be complete.

Copy link
Contributor

@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.

Based on #1169.

… .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.
…the variant ID as well as the EBNF form of the kind to use as documentation. Getting the EBNF form involves creating an instance of `SpecModel` and serializing the ID using that.
…guage` field to be a `&Language` instead of an `Rc<Language>` because of some weird issue getting `KindBuilder` to instantiate properly given that `KindsModel::from_language` received a `&Rc<Language>` param. However, I realized afterwards that I could just use `to_owned` when creating the SpecModel instead of changing SpecModel everywhere.
… for the clippy lints `doc_markdown` and `doc_link_with_quotes`. These are "pedantic" lints that we run into issues with now that we're outputting the EBNF docs for these kinds, but the errors aren't relevant to us.
…now-unused module import `codegen_spec`.
…odeKind` to `NodeType`.

* Update base cst/index.mts to expect & export `NodeType`
@mjoerussell
Copy link
Contributor Author

@OmarTawfik Rebase is complete, ready for your review whenever.

Copy link
Contributor

@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 couple of comments about unintended changes. Otherwise, LGTM.
Thank you!

@mjoerussell
Copy link
Contributor Author

Just some artifacts from the rebase, even though they were fixed in #1169 the rebase brought them back here.

@mjoerussell mjoerussell added this pull request to the merge queue Dec 12, 2024
Merged via the queue into NomicFoundation:main with commit 6a31d1b Dec 12, 2024
1 check passed
@mjoerussell mjoerussell deleted the feature/ts-api-docs branch December 12, 2024 19:43
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.

add doc comments for classes update NodeVariant API
2 participants