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 new TypeScript bindgen #1783

Merged
merged 21 commits into from
Apr 15, 2024
Merged

Add new TypeScript bindgen #1783

merged 21 commits into from
Apr 15, 2024

Conversation

RareSecond
Copy link
Contributor

First pass at generating a new .ts file that can be used as the main entrypoint for the updated TypeScript SDK.

This current iteration works perfectly fine for fire-and-forget actions. However, once I start incorporating statefulness into the setup, this file may need some changing. But wanted to get this in already, so that future changes can be more focused on specific features!

Can be tested by running sozo build --typescript-v2

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 99.45554% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 69.14%. Comparing base (fc5ad79) to head (3494fde).
Report is 27 commits behind head on main.

Files Patch % Lines
...ates/dojo-bindgen/src/plugins/typescript_v2/mod.rs 99.62% 2 Missing ⚠️
bin/sozo/src/commands/build.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1783      +/-   ##
==========================================
+ Coverage   67.43%   69.14%   +1.71%     
==========================================
  Files         305      308       +3     
  Lines       33521    34612    +1091     
==========================================
+ Hits        22604    23934    +1330     
+ Misses      10917    10678     -239     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

@RareSecond great first pass here!
Some minor comment on the go for now.

I'm wondering how we can test that effectively. Is the generated JS file something we can run easily somehow to at least verify there's no errors?
Because doing string comparisons can be a solution, but we may think of something better to ensure more robust testing on the code generation.

For the test failing for the format, you have a script into the repo scripts/rust_fmt.sh. You can run it to verify locally. And if you want auto formatting, run the same command that's inside the script and remove the --check.

@RareSecond
Copy link
Contributor Author

I'm wondering how we can test that effectively. Is the generated JS file something we can run easily somehow to at least verify there's no errors? Because doing string comparisons can be a solution, but we may think of something better to ensure more robust testing on the code generation.

Hmm what about saving a snapshot of the fully generated file and testing against that? It should always output the exact same content for the same input.

@glihm glihm added the sozo label Apr 8, 2024
@glihm
Copy link
Collaborator

glihm commented Apr 8, 2024

I'm wondering how we can test that effectively. Is the generated JS file something we can run easily somehow to at least verify there's no errors? Because doing string comparisons can be a solution, but we may think of something better to ensure more robust testing on the code generation.

Hmm what about saving a snapshot of the fully generated file and testing against that? It should always output the exact same content for the same input.

Yep, sounds a good approach. This is what we have for dojo-lang, we run a command like:

CAIRO_FIX=1 cargo test ...

and it counts as a generation run where the output will be considered as the truth for the other test runs without with CAIRO_FIX.

However, I was thinking about something more functional, ensuring at least the JS code generated is correct and "compiles"/"can be interpreted correctly".
But let's do a first pass as you mentioned, testing that the output is as expected, and for now let's have a file that you've tested on your end and you know it works in JS.

WDYT @RareSecond?

@RareSecond
Copy link
Contributor Author

Sounds good! However, I'll need some guidelines on how to best approach this testing. Because I don't know how stuff is currently mocked in the repo, i.e. how do I ensure the function is called with the correct testing parameters.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

@RareSecond I've let some comments about testing and answering some questions. Let me know if you have doubt on this or if it's unclear, we can catch up anytime. 👍

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

@RareSecond great work on this!
As we've mentioned, as this plugin will be enhanced soon we will apply more tests on top of these modifications. 👍

As dojo-js is widely used, we should have at some functional test to ensure we're generating valid javascript. Let's discuss that at the next iteration.

The examples folder will soon contain more examples with some edge cases like several contracts with same name etc.. this will also be a good test to see how we handle such ones.

@glihm glihm merged commit 017c0d1 into main Apr 15, 2024
12 checks passed
@glihm glihm deleted the feature/add-new-typescript-bindgen branch April 15, 2024 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants