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 wasm support #10

Merged
merged 1 commit into from
Sep 29, 2024
Merged

Add wasm support #10

merged 1 commit into from
Sep 29, 2024

Conversation

Specy
Copy link
Collaborator

@Specy Specy commented Sep 13, 2024

I'll open this PR for adding wasm support to the lib plus a TS wrapper over it to make a more javascripty idiomatic experience.

This PR is still ongoing, for now i added basic support for creating a grammar and a parser, still have to work out the rest, there will be a lot of serialization work going on!
It shouldn't be that hard to add the wasm layer, might take a few days

Implements #8

@Specy Specy marked this pull request as draft September 13, 2024 17:45
@Specy
Copy link
Collaborator Author

Specy commented Sep 13, 2024

I Added most of the typings and features, Grammar and Parser should mostly be completed, i only lack some typings for the various tables as i'm not sure how to type IndexMap and IndexSet, i'll have to see what serde spits out as json.
i've published the package on npm here

@umut-sahin
Copy link
Owner

IndexMap and IndexSet are for preserving insertion order. So that the output format is deterministic. Other than this they are pretty much the same as HashMap and HashSet, so you should be able to save it as JSON object.

Ordering of JavaScript objects are not guaranteed, so you can't just load the object while preserving order. However, there are global Map and Set classes, which have the exact same semantics as IndexMap and IndexSet, so I'd look into those!

@Specy
Copy link
Collaborator Author

Specy commented Sep 14, 2024

Seems like wasm_bindgen correctly converted IndexMap and IndexSet to a Map and an array, the set wouldn't make too much sense in javascript so i'm leaving it as an array.

The library should be pretty much complete by now, i've added the last typings and some documentation for the readme of the npm module. I've been trying it on my website and it seems to be working good! i'll link it here after i get to publish a beta version of it

@Specy Specy marked this pull request as ready for review September 14, 2024 08:31
Copy link
Owner

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

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

One thing to note, release process should be automated in the CI to publish to NPM. Also, the versions should match between crates.io and npm. This would allow us to have a unified version and compatibility between versions. #11 would do the same with pypi.

Another small thing is I'd prefer ts-lib directory to be moved to bindings/typescript as I'm thinking of having bindings/python as well.

I left a partial review, I need to head out for now, sorry. I'll do another review once you say it's ready for round 2.

Let me know if you have any questions, or need help with something!

Lastly, thanks for doing this, this is really valuable and I can't wait to see the website 🫶

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
examples/calculator.rs Outdated Show resolved Hide resolved
src/errors.rs Outdated Show resolved Hide resolved
Copy link
Owner

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, it's been a busy week.

There are some stuff to change, but overall, it looks preem!

Let me know if you have any question, or if you want help with anything.

Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
bindings/typescript/.gitignore Outdated Show resolved Hide resolved
bindings/typescript/package.json Outdated Show resolved Hide resolved
bindings/typescript/src/utils.ts Show resolved Hide resolved
src/errors.rs Outdated Show resolved Hide resolved
src/grammar.rs Outdated Show resolved Hide resolved
src/utils.rs Show resolved Hide resolved
Copy link
Owner

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

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

Could you change:

  publish:
    if: startsWith(github.ref, 'refs/tags/v')
    needs: [ conformance, test ]

    name: Publish

to

  publish-crates-io:
    if: startsWith(github.ref, 'refs/tags/v')
    needs: [ conformance, test ]

    name: Publish to Crates.io

This is probably the last review.

Thanks 🥳

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
bindings/typescript/src/index.ts Show resolved Hide resolved
bindings/typescript/src/types.ts Show resolved Hide resolved
bindings/typescript/src/utils.ts Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
Copy link
Owner

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

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

Last request and we can merge!

Could you squash all of the commits to a single one with the message:

feat: add typescript bindings using wasm

CI needs to be green as well, of course.

Copy link
Owner

@umut-sahin umut-sahin left a comment

Choose a reason for hiding this comment

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

Thanks for this amazing work, and sorry for the long review process. The time has come!

@umut-sahin umut-sahin merged commit 696d90c into umut-sahin:main Sep 29, 2024
6 checks passed
@umut-sahin umut-sahin mentioned this pull request Sep 29, 2024
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