Feat/bindgen improve types #2816
Conversation
WalkthroughOhayo, sensei! This pull request focuses on enhancing the TypeScript bindings generation for Dojo, specifically addressing issues with Option types, custom enums, and field order handling. The changes span multiple files in the bindgen system, introducing new methods, constants, and type handling logic to improve the generation of TypeScript interfaces and type definitions. Dependency updates in the Cargo.toml file complement these code modifications. Changes
Assessment against linked issues
Possibly related PRs
Suggested Reviewers
Sensei, the changes look comprehensive and address the key issues outlined in the linked issue. The modifications enhance TypeScript binding generation with improved handling of Option types, custom enums, and field order management. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
crates/dojo/bindgen/src/plugins/typescript/generator/schema.rs (3)
15-19: Ohayo, sensei! Consider using a constant for the import statement.The import statement string
"import type { SchemaType as ISchemaType } from \"@dojoengine/sdk\";\n"is hardcoded multiple times. For better maintainability and to avoid potential errors if the import path changes, consider defining it as a constant or using a shared method to generate this import statement.
153-160: Ohayo, sensei! Use constants for expected strings in tests.In the test
test_it_adds_imports, the expected import statement is hardcoded. To enhance test maintainability and reduce duplication, consider defining expected strings as constants or using helper functions. This approach makes it easier to update tests if the import path changes.
286-288: Ohayo, sensei! Reduce code duplication in test token creation functions.The functions
create_test_struct_tokenandcreate_test_nested_struct_tokenshare similar logic. Refactoring common code into a helper function can minimize duplication and make the tests more maintainable.Also applies to: 316-318
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (1)
91-96: Ohayo sensei! Consider refactoring the prefix selection logic.The nested if-else structure could be simplified for better readability and maintainability.
Consider this alternative approach:
- if t.r#type == CompositeType::Enum { - "models." - } else if t.r#type == CompositeType::Struct - && !t.type_path.starts_with("core") - { - "models.Input" - } else { - "" - } + match t.r#type { + CompositeType::Enum => "models.", + CompositeType::Struct if !t.type_path.starts_with("core") => "models.Input", + _ => "" + }Also, consider extracting "core" as a constant:
const CORE_PREFIX: &str = "core";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(2 hunks)crates/dojo/bindgen/src/plugins/mod.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/constants.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/enum.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/function.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs(3 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs(6 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/schema.rs(7 hunks)crates/dojo/bindgen/src/plugins/typescript/writer.rs(1 hunks)
🔇 Additional comments (7)
crates/dojo/bindgen/src/plugins/typescript/generator/schema.rs (1)
Line range hint 1-274: Ohayo, sensei! Overall, the changes enhance the TypeScript schema generation.
The improvements to the TsSchemaGenerator and the associated tests enhance code clarity and maintainability. The handling of namespaces and type definitions is more robust, aligning well with the project's objectives.
crates/dojo/bindgen/src/plugins/typescript/generator/enum.rs (1)
40-74: Custom Enum Generation Implemented Correctly
Ohayo, sensei! The implementation of custom enum generation and the associated type definitions looks solid. Great work on enhancing the functionality for TypeScript enums.
crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs (1)
49-73: Import Handling and Input Type Addition Implemented Successfully
Ohayo, sensei! The enhancements made to the generate method, including import handling and input type addition, are well-implemented. Good job on ensuring that the necessary imports and type definitions are correctly managed.
crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (2)
60-71: Utility Functions for Token Type Checks Added Successfully
Ohayo, sensei! The token_is_option and token_is_custom_enum functions are a valuable addition for type checking within the generator. Nicely done!
111-130: Robust Handling of Composite Tokens in JsType::from
Ohayo, sensei! The updates to the JsType::from implementation enhance the handling of Option types and custom enums effectively. This ensures accurate TypeScript type generation.
crates/dojo/bindgen/src/plugins/mod.rs (1)
77-84: Ohayo sensei! The new Buffer method looks great!
The insert_at_index method is well-documented and provides a useful addition to the Buffer's insertion capabilities.
Cargo.toml (1)
74-75: Ohayo sensei! Good dependency management practices!
The switch from commit hashes to tagged versions for cainome dependencies improves reproducibility. However, let's verify compatibility with the updated versions.
Also applies to: 213-213
✅ Verification successful
Ohayo! The update to cainome v0.4.10 is safe to proceed!
The new version only adds a max recursion depth parameter to avoid hydration stack overflow, which is a non-breaking enhancement. The extensive usage of cainome throughout the codebase primarily involves serialization/deserialization features that are unaffected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in the updated dependencies
# Check cainome changelog or releases for breaking changes
gh api repos/cartridge-gg/cainome/releases/tags/v0.4.10 --jq '.body'
# Check serde_with changelog for breaking changes
curl -s https://raw.githubusercontent.com/jonasbb/serde_with/v3.11.0/CHANGELOG.md
Length of output: 416
Script:
#!/bin/bash
# Let's check the actual usage of cainome in the codebase
rg "use.*cainome" -A 3
# Check if there are any specific uses of features from cainome that might be affected
rg "cainome::" -A 3
# Let's also check the previous version used (if any) in git history
git show HEAD~1:Cargo.toml | rg "cainome"
Length of output: 84886
| // fastest way to add a field to the interface is to search for the n-1 | ||
| // `,` and add the | ||
| // field directly after it. | ||
| // to improve this logic, we would need to either have some kind of code parsing. | ||
| // we could otherwise have some intermediate representation that we pass to this generator | ||
| // function. | ||
| buffer.insert_after(gen, &schema_type, ",", 2); | ||
| buffer.insert_after(gen, &ns_def, ",", 2); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Refactor buffer manipulation logic for robustness.
Manipulating the buffer by searching for commas and inserting code based on string patterns can be error-prone and hard to maintain. Consider using a proper code parsing library or building an intermediate representation (IR) to manage code transformations more reliably.
| fn namespace_is_defined(&self, buffer: &mut Buffer, ns: &str) -> bool { | ||
| buffer.has(format!("\n\t{ns}: {{\n\t\t").as_str()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Improve namespace detection logic.
Using exact string matching in namespace_is_defined may be brittle and could fail if the formatting of the buffer changes. Consider maintaining a set of defined namespaces or parsing the buffer to reliably check if a namespace is already defined.
| let pos = buffer.pos(SN_IMPORT_SEARCH).unwrap(); | ||
| buffer.insert_at_index(CAIRO_ENUM_TYPE_IMPL.to_owned(), pos + 1); | ||
| } |
There was a problem hiding this comment.
Possible Panic when Calling unwrap() on buffer.pos(SN_IMPORT_SEARCH)
Ohayo, sensei! There's a potential issue with the use of unwrap() at line 28. If SN_IMPORT_SEARCH is not found in the buffer, pos() will return None, and calling unwrap() would cause a panic. It's recommended to handle the Option safely to avoid unexpected panics.
Apply this diff to handle the Option safely:
- let pos = buffer.pos(SN_IMPORT_SEARCH).unwrap();
+ if let Some(pos) = buffer.pos(SN_IMPORT_SEARCH) {
+ buffer.insert_at_index(CAIRO_ENUM_TYPE_IMPL.to_owned(), pos + 1);
+ } else {
+ // Handle the case where SN_IMPORT_SEARCH is not found
+ // For example, insert CAIRO_ENUM_TYPE_IMPL at a default position or log an error
+ }Committable suggestion skipped: line range outside the PR's diff.
| if !buffer.has(SN_IMPORT_SEARCH) { | ||
| buffer.push(CAIRO_OPTION_IMPORT.to_owned()); | ||
| } else if !buffer.has(CAIRO_OPTION_TOKEN) { | ||
| // If 'starknet' import is present, we add CairoOption to the imported types | ||
| buffer.insert_after(format!(" {CAIRO_OPTION_TOKEN}"), SN_IMPORT_SEARCH, "{", 1); |
There was a problem hiding this comment.
Potential Panic when Using buffer.insert_after without Checking Position
Ohayo, sensei! In the check_import method, when SN_IMPORT_SEARCH is present, the buffer.insert_after method is used without ensuring that SN_IMPORT_SEARCH exists at a valid position. This may lead to a panic if the expected string is not found. It's advisable to check the result of buffer.pos(SN_IMPORT_SEARCH) before inserting to prevent unexpected panics.
Apply this diff to handle the Option safely:
- buffer.insert_after(format!(" {CAIRO_OPTION_TOKEN}"), SN_IMPORT_SEARCH, "{", 1);
+ if let Some(_) = buffer.pos(SN_IMPORT_SEARCH) {
+ buffer.insert_after(format!(" {CAIRO_OPTION_TOKEN}"), SN_IMPORT_SEARCH, "{", 1);
+ } else {
+ // Handle the case where SN_IMPORT_SEARCH is not found
+ // For example, insert CAIRO_OPTION_IMPORT at a default position or log an error
+ }Committable suggestion skipped: line range outside the PR's diff.
| @@ -14,3 +14,20 @@ pub const CAIRO_BOOL: &str = "bool"; | |||
| pub const JS_BOOLEAN: &str = "boolean"; | |||
| pub const JS_STRING: &str = "string"; | |||
| pub const JS_BIGNUMBERISH: &str = "BigNumberish"; | |||
|
|
|||
| pub(crate) const BIGNUMNERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';\n"; | |||
There was a problem hiding this comment.
Typo in Constant Name BIGNUMNERISH_IMPORT
Ohayo, sensei! There's a typo in the constant name BIGNUMNERISH_IMPORT. It should be BIGNUMBERISH_IMPORT to match the correct spelling of 'BigNumberish'.
Apply this diff to fix the typo:
- pub(crate) const BIGNUMNERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';\n";
+ pub(crate) const BIGNUMBERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';\n";Be sure to update all references to BIGNUMNERISH_IMPORT throughout the codebase to prevent any compilation errors.
Committable suggestion skipped: line range outside the PR's diff.
| // println!("Generating code for model {}", c.type_path); | ||
| // println!("{:#?}", c); | ||
| // println!("====================="); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider using proper logging instead of commented-out print statements.
Replace the commented debug prints with proper logging using the log crate's macros for better debugging capabilities.
- // println!("Generating code for model {}", c.type_path);
- // println!("{:#?}", c);
- // println!("=====================");
+ log::debug!("Generating code for model {}", c.type_path);
+ log::trace!("Model details: {:#?}", c);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // println!("Generating code for model {}", c.type_path); | |
| // println!("{:#?}", c); | |
| // println!("====================="); | |
| log::debug!("Generating code for model {}", c.type_path); | |
| log::trace!("Model details: {:#?}", c); |
| if t.r#type == CompositeType::Enum { | ||
| "models." | ||
| } else if t.r#type == CompositeType::Struct | ||
| && !t.type_path.starts_with("core") | ||
| { | ||
| "models.Input" |
There was a problem hiding this comment.
💡 Codebase verification
Additional test coverage needed for composite type handling, sensei!
The current test suite lacks specific test cases for:
- Enum type prefix handling (
models.) - Struct type prefix handling (
models.Input) - Core-prefixed struct handling (no prefix)
Test improvements needed at:
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs: Add test cases with composite types increate_test_function
🔗 Analysis chain
Verify test coverage for new type handling logic.
While the test file includes comprehensive cases, we should verify coverage for the new prefix selection logic, especially for core-prefixed structs.
Let me gather more information about the test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for composite type handling
# Look for test cases covering different composite types
rg -A 10 "test_format_function_inputs" crates/dojo/bindgen/src/plugins/typescript/generator/
# Check for enum and struct test cases
ast-grep --pattern 'Token::Composite(t) { r#type: CompositeType::$_'
Length of output: 2871
Script:
#!/bin/bash
# Check test helper functions that create test data
rg -A 15 "create_.*_function" crates/dojo/bindgen/src/plugins/typescript/generator/function.rs
# Look for test cases with different composite types
rg -B 2 -A 5 "CompositeType::" crates/dojo/bindgen/src/plugins/typescript/generator/function.rs
# Check test utilities and mock data
rg -A 10 "fn create_" crates/dojo/bindgen/src/plugins/typescript/generator/function.rs
Length of output: 8619
|
@MartianGreed could you add #2781 too, please? |
Description
Several bindgen improvements, mostly over
CairoOptionandCairoCustomEnumRelated issue
Fixes #2784 #2782
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores