feat: Torii upgrade #303
feat: Torii upgrade #303
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on updates to the Continuous Integration (CI) workflow and package dependencies. The CI configuration in Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/state/package.json (1)
Line range hint
3-3: Consider updating the package versionGiven that a significant dependency has been updated, it might be appropriate to increment the version number of the
@dojoengine/statepackage itself. This helps in tracking changes and maintaining semantic versioning.Consider updating the version from "1.0.0-alpha.20" to the next appropriate version (e.g., "1.0.0-alpha.21" or "1.0.0-beta.1", depending on the nature of the changes).
.github/workflows/ci.yaml (2)
Line range hint
16-17: Good addition of submodule update step.Adding a step to update submodules is a good practice. It ensures that all submodules are properly initialized and up-to-date before the build process, preventing potential issues with missing or outdated submodule content.
Consider optimizing this step by adding the
--depth 1flag to the git submodule update command, which can speed up the process by fetching only the latest commit for each submodule:- run: git submodule update --init --recursive + run: git submodule update --init --recursive --depth 1This can significantly reduce the checkout time, especially for repositories with large submodules or a long history.
Line range hint
44-47: Consider addressing the commented-out Codecov section.There's a commented-out section for uploading coverage reports to Codecov. If code coverage is important for your project, consider either:
- Uncommenting and configuring this step if you want to start using Codecov.
- Removing this commented-out section if there are no plans to use Codecov in the near future.
This will help maintain a cleaner and more intentional CI configuration.
packages/state/src/recs/index.ts (4)
125-130: LGTM. Consider adding a comment explaining the purpose ofdont_include_hashed_keys.The addition of
dont_include_hashed_keys: trueto theclient.getEntitiescall is consistent with the Torii upgrade. This change likely optimizes performance or addresses data privacy concerns by excluding hashed keys from the returned entities.Consider adding a brief comment explaining the purpose and impact of setting
dont_include_hashed_keysto true, to improve code maintainability and clarity for future developers.
168-168: LGTM. Ensure consistent documentation across similar changes.The addition of
dont_include_hashed_keys: trueto theclient.getEventMessagescall is consistent with the earlier change in thegetEntitiesfunction and aligns with the Torii upgrade.For consistency, if you add a comment explaining
dont_include_hashed_keysin thegetEntitiesfunction as suggested earlier, consider adding a similar comment here or referencing the explanation ingetEntities.
235-235: LGTM. Consider adding global documentation fordont_include_hashed_keys.The addition of
dont_include_hashed_keys: trueto thisclient.getEntitiescall ingetEntitiesQueryis consistent with the previous changes and aligns with the Torii upgrade.Given that this parameter has been added to multiple functions, consider adding a global comment or updating the documentation to explain the purpose and impact of
dont_include_hashed_keysacross all affected API calls. This would improve overall code clarity and maintainability.
Line range hint
125-235: Overall LGTM. Consider improving documentation for the Torii upgrade changes.The consistent addition of
dont_include_hashed_keys: trueacross multiple API calls (getEntities,getEvents,getEntitiesQuery) aligns well with the Torii upgrade objective. These changes likely represent a significant update in how the Torii library handles hashed keys.To improve code maintainability and clarity:
- Add a global comment or update the module documentation to explain the purpose and impact of
dont_include_hashed_keys.- Consider updating the function documentation for
getEntities,getEvents, andgetEntitiesQueryto mention this new behavior.- If there are any performance implications or use cases where developers might want to include hashed keys, document these considerations.
These documentation improvements will help future developers understand the rationale behind these changes and use the updated API effectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
- .github/workflows/ci.yaml (1 hunks)
- packages/state/package.json (1 hunks)
- packages/state/src/recs/index.ts (3 hunks)
- packages/torii-client/package.json (1 hunks)
- packages/torii-wasm/build.sh (1 hunks)
- worlds/dojo-starter (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- worlds/dojo-starter
🧰 Additional context used
🔇 Additional comments (4)
packages/torii-wasm/build.sh (2)
Line range hint
1-24: LGTM for the rest of the scriptThe remainder of the script follows good practices:
- It uses
set -exfor improved error handling and debugging.- It builds for both web and Node.js targets.
- It cleans up after the build process by removing the cloned repository.
These aspects contribute to a robust and maintainable build process.
7-7: 🛠️ Refactor suggestionConsider the implications of specifying a feature branch for cloning
The change to clone from the
grpc-updatebranch instead of the default branch has several implications:
- It ensures consistency by targeting a specific branch, which can be beneficial for reproducible builds.
- However, feature branches like
grpc-updateare often less stable than main branches, potentially introducing instability into the build process.- The build now depends on the state of this specific branch, which may not always be up-to-date or could be deleted in the future.
- This change might impact CI/CD pipelines that rely on this script, especially if the specified branch is not always available.
Consider the following improvements:
- Use an environment variable or script parameter to specify the branch, allowing for easier switching between branches:
-git clone --depth 1 --branch grpc-update https://github.com/dojoengine/dojo.c dojo.c +BRANCH=${DOJO_BRANCH:-main} +git clone --depth 1 --branch $BRANCH https://github.com/dojoengine/dojo.c dojo.c
- Add error handling to deal with cases where the specified branch doesn't exist:
if ! git clone --depth 1 --branch $BRANCH https://github.com/dojoengine/dojo.c dojo.c; then echo "Failed to clone branch $BRANCH, falling back to main" git clone --depth 1 https://github.com/dojoengine/dojo.c dojo.c fi
- Consider using a specific commit hash or tag instead of a branch name for even more reproducible builds:
COMMIT_HASH=abc123... # Replace with the desired commit hash git clone https://github.com/dojoengine/dojo.c dojo.c cd dojo.c git checkout $COMMIT_HASHThese changes would make the script more flexible and robust while maintaining the ability to target specific versions when needed.
To ensure that the
grpc-updatebranch exists and is up-to-date, you can run the following check:This will help verify the current state of the specified branch.
packages/state/package.json (1)
28-28: Dependency update to workspace referenceThe dependency for
@dojoengine/torii-clienthas been changed from a fixed version"1.0.0-alpha.20"to a workspace reference"workspace:*". This change aligns with the PR objective of upgrading the Torii library.Pros:
- Allows for easier local development and testing of interdependent packages.
- Ensures that the latest compatible version of the torii-client is always used within the workspace.
Cons:
- May introduce potential instability if the torii-client package undergoes breaking changes.
- Could make the build less reproducible across different environments.
To ensure consistency and identify potential issues, let's check for other workspace references and the overall dependency structure:
✅ Verification successful
Workspace reference for
@dojoengine/torii-clientis consistent across the monorepo.No issues found with the dependency change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other workspace references and overall dependency structure jq '.dependencies + .devDependencies + .peerDependencies' packages/state/package.json # Check if other packages in the monorepo use workspace references fd package.json | xargs jq -c 'select(.dependencies."@dojoengine/torii-client" | type == "string" and startswith("workspace:")) | {file: input_filename, dep: .dependencies."@dojoengine/torii-client"}'Length of output: 1280
.github/workflows/ci.yaml (1)
23-23: Verify compatibility with the updated dojoup version.The dojoup command has been updated to v1.0.0-alpha.16, which aligns with the PR objective of upgrading the Torii library. This change likely brings improvements or fixes to the Dojo toolchain.
To ensure compatibility, please run the following script to check if there are any version-specific dependencies or configurations in the project:
packages/torii-client/package.json
Outdated
| @@ -17,7 +17,7 @@ | |||
| } | |||
| }, | |||
| "dependencies": { | |||
| "@dojoengine/torii-wasm": "workspace:*" | |||
| "@dojoengine/torii-wasm": "1.0.0-alpha.20" | |||
There was a problem hiding this comment.
💡 Codebase verification
Issues Found:
- The specified version
@dojoengine/[email protected]is not published. This will prevent thetorii-clientpackage from resolving its dependency correctly. - There are remaining workspace dependencies for
@dojoengine/torii-wasmin other packages:examples/example-vite-react-phaser-recs/package.jsonexamples/example-vite-kitchen-sink/package.jsonexamples/example-vite-react-sdk/package.json
Please address these issues to ensure consistent dependency management and a successful build process.
🔗 Analysis chain
Verify the impact of switching from workspace dependency to a specific version.
The dependency @dojoengine/torii-wasm has been updated from a workspace dependency ("workspace:*") to a specific version ("1.0.0-alpha.20"). This change has several implications:
- It suggests that
torii-wasmis now being published as a separate package rather than being used as a local workspace dependency. - The version matches the current version of
torii-client, which is good for consistency but may require manual updates in the future to keep them in sync. - This change might affect the build process and development workflow, as it's no longer automatically using the latest local version.
To ensure this change doesn't introduce any issues, please run the following checks:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the specified version of torii-wasm is published
npm view @dojoengine/[email protected]
# Verify that the build process still works with the new dependency
npm run build
# Check for any scripts or documentation that might need updating due to this change
grep -R "workspace:\*" .
Length of output: 8214
Script:
#!/bin/bash
# Check if the specific version of torii-wasm is published using GitHub CLI
gh api repos/CodeRabbitInc/torii-wasm/releases/tags/v1.0.0-alpha.20
# Search for any remaining workspace dependencies for @dojoengine/torii-wasm in the codebase
rg '"@dojoengine/torii-wasm":\s*"workspace:\*"' .
Length of output: 578
Summary by CodeRabbit
New Features
Dependency Updates
@dojoengine/torii-clientto a dynamic workspace reference.@dojoengine/torii-wasmdependency to a specific version.Bug Fixes
Subproject Update