fix: submodule #272
fix: submodule #272
Conversation
WalkthroughThe changes involve updating the directory paths for the Changes
Sequence Diagram(s)(No sequence diagrams generated as the changes are straightforward and do not involve complex interactions.) Possibly related PRs
Poem
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/ci.yaml (1 hunks)
- .gitmodules (1 hunks)
Additional comments not posted (1)
.github/workflows/ci.yaml (1)
25-25: Verify the new directory path and update related references.The change from
examples/dojo/dojo-startertoworlds/dojo-starterlooks intentional and likely part of a project restructuring. However, to ensure smooth CI execution:
- Verify that the
worlds/dojo-starterdirectory exists in the repository.- Check if there are any other occurrences of the old path (
examples/dojo/dojo-starter) in the project that might need updating.To assist with verification, you can run the following script:
This script will help ensure that the new directory structure is in place and identify any other files that might need updating.
Verification successful
Directory path updated correctly and no lingering references found.
The new directory
worlds/dojo-starterexists as expected, and no other occurrences ofexamples/dojo/dojo-starterwere found in YAML or Markdown files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the new directory and check for other occurrences of the old path # Check if the new directory exists if [ -d "worlds/dojo-starter" ]; then echo "The new directory 'worlds/dojo-starter' exists." else echo "Warning: The directory 'worlds/dojo-starter' does not exist." fi # Search for other occurrences of the old path echo "Searching for other occurrences of 'examples/dojo/dojo-starter':" rg "examples/dojo/dojo-starter" --type yaml --type mdLength of output: 342
| @@ -1,3 +1,3 @@ | |||
| [submodule "dojo-starter"] | |||
| path = examples/dojo-starter | |||
| path = worlds/dojo-starter | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Update remaining references to the old submodule path.
The following files still reference examples/dojo-starter and should be updated to worlds/dojo-starter:
readme.md- Line with
cd examples/dojo-starter - Another line with
cd examples/dojo-starter
- Line with
examples/example-nodejs-bot/tsconfig.json"exclude": ["examples/dojo-starter/target/dev/manifest.json"]
Please ensure these paths are updated to reflect the new submodule location to maintain consistency across the project.
Analysis chain
LGTM! Verify consistency across the project.
The change to the submodule path from examples/dojo-starter to worlds/dojo-starter looks good. This appears to be a directory restructuring, which aligns with the PR title "fix: submodule".
To ensure consistency across the project, please run the following script to check for any references to the old path:
This script will help identify any potential inconsistencies or references that might need updating due to this path change.
LGTM! Verify no residual references to the old submodule path.
To ensure that there are no remaining references to examples/dojo-starter outside of .gitmodules, please run the following script:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for references to the old submodule path
# Search for references to the old path
echo "Searching for references to 'examples/dojo-starter':"
rg --type-not gitmodules "examples/dojo-starter"
# Check if the new directory structure exists
echo "Checking if the new directory structure exists:"
if [ -d "worlds/dojo-starter" ]; then
echo "Directory 'worlds/dojo-starter' exists."
else
echo "Warning: Directory 'worlds/dojo-starter' does not exist."
fi
# Verify submodule status
echo "Verifying submodule status:"
git submodule status
Length of output: 587
Script:
#!/bin/bash
# Description: Check for references to the old submodule path excluding .gitmodules
echo "Searching for references to 'examples/dojo-starter' excluding .gitmodules:"
rg "examples/dojo-starter" --glob '!**/.gitmodules'
Length of output: 394
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
scripts/build-packages.sh (1)
23-28: Selective testing implementation looks good, but consider some improvements.The changes introduce selective testing based on package characteristics, which can potentially speed up the build process. However, there are a few points to consider:
The condition for skipping tests is hardcoded. Consider using a configuration file or environment variables to define which packages should skip tests. This would make the script more flexible and easier to maintain.
It would be helpful to add comments explaining why WASM packages and the torii-client package are excluded from testing.
Consider adding a warning or log message when skipping tests for a package. This would make it clear which packages are not being tested during the build process.
Here's a suggested improvement to make the script more flexible and informative:
for package in "${packages[@]}"; do echo "Building $package..." pnpm --dir "$package" build - # Run tests only for non-wasm packages and non-torii-client packages - if [[ "$package" != *"-wasm" && "$package" != "packages/torii-client" ]]; then + # Check if package should be tested based on SKIP_TEST_PACKAGES environment variable + if [[ ! " ${SKIP_TEST_PACKAGES[*]} " =~ " ${package} " ]]; then pnpm --dir "$package" test + else + echo "Skipping tests for $package as per configuration" fi doneYou would then set the
SKIP_TEST_PACKAGESenvironment variable before running the script, like this:export SKIP_TEST_PACKAGES="packages/torii-wasm packages/utils-wasm packages/torii-client"This approach allows for more flexibility in deciding which packages to skip testing, and it provides clear feedback about skipped tests.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/state/src/tests/recs.test.ts (0 hunks)
- packages/state/src/tests/utils.test.ts (1 hunks)
- scripts/build-packages.sh (1 hunks)
Files not reviewed due to no reviewable changes (1)
- packages/state/src/tests/recs.test.ts
Additional comments not posted (1)
packages/state/src/__tests__/utils.test.ts (1)
107-108: Verify the intended behavior for nested schema conversionThe change in this test case suggests a significant modification in how the
convertValuesfunction handles nested objects:
- Previously, it converted the nested string value "42" to a number.
- Now, it preserves the original structure without type conversion.
This change could have far-reaching implications in how nested objects are processed throughout the application.
Please confirm if this change in behavior is intentional. If so:
- Consider updating the test name from "should correctly convert nested schema values" to something like "should preserve structure for nested schema values" to accurately reflect the new behavior.
- Verify that this change doesn't negatively impact other parts of the codebase that may expect nested values to be converted.
To help verify the impact, you can run the following script to find other occurrences of nested schema handling:
If this change is unintentional, please review the implementation of the
convertValuesfunction to ensure it correctly handles nested schemas as expected.Verification successful
Intended behavior for nested schema conversion confirmed
The search revealed that the only relevant changes are within the current test file. No other usages of
convertValueswith nested schemas were found.Therefore, the modification in the test case appears intentional and isolated.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other tests or usages of nested schemas rg --type typescript -e "nested.*schema" -e "convertValues.*nested"Length of output: 104
Script:
#!/bin/bash # Search for other tests or usages of nested schemas in TypeScript files rg -g '*.ts' -g '*.tsx' -e "nested.*schema" -e "convertValues.*nested"Length of output: 275
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
packages/create-burner/test/manager/burnerManager.test.ts (3)
74-79: LGTM! Consider extracting the chain ID to a constant.The addition of the mock for
provider.getChainId()and the updated expectation forStorage.getimprove the test's consistency and robustness. Good job!To further improve maintainability, consider extracting the chain ID "0x1" to a constant at the top of the file:
const TEST_CHAIN_ID = "0x1"; // Then use it in your tests burnerManager.provider.getChainId = vi.fn().mockResolvedValue(TEST_CHAIN_ID); expect(Storage.get).toHaveBeenCalledWith(`burners_${parseInt(TEST_CHAIN_ID)}`);This would make it easier to update the chain ID across all tests if needed in the future.
83-85: LGTM! Consider centralizing the mock setup.The addition of the mock for
provider.getChainId()in this test ensures consistency with the previous test. Good job on maintaining consistency!To reduce duplication and improve maintainability, consider centralizing the mock setup in the
beforeEachblock:beforeEach(() => { vi.resetAllMocks(); burnerManager = getBurnerManager(); burnerManager.provider.getChainId = vi.fn().mockResolvedValue(TEST_CHAIN_ID); });This way, you ensure that all tests use the same chain ID mock without having to repeat the setup in each test. It also makes it easier to update the mock behavior across all tests if needed in the future.
Suggested improvements have not been implemented.
- The
TEST_CHAIN_IDconstant is not defined inpackages/create-burner/test/manager/burnerManager.test.ts.- Mock setups for
burnerManager.provider.getChainIdare still present in individual test cases instead of being centralized in thebeforeEachblock.Analysis chain
Line range hint
1-185: Overall, good improvements to test consistency.The changes you've made improve the reliability and consistency of the tests by properly mocking the chain ID. This is a positive step towards more robust testing.
If you decide to implement the suggested improvements (extracting the chain ID to a constant and centralizing the mock setup), I'd be happy to verify those changes in a follow-up review.
If you implement the suggested improvements, you can run the following script to verify the changes:
This script will help ensure that the suggested improvements have been correctly implemented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the suggested improvements have been implemented. # Test 1: Check if TEST_CHAIN_ID constant is defined echo "Checking for TEST_CHAIN_ID constant:" rg 'const TEST_CHAIN_ID = "0x1";' packages/create-burner/test/manager/burnerManager.test.ts # Test 2: Check if mock setup is centralized in beforeEach echo "Checking for centralized mock setup in beforeEach:" rg 'beforeEach\(\) \{[\s\S]*burnerManager\.provider\.getChainId = vi\.fn\(\)\.mockResolvedValue\(TEST_CHAIN_ID\);[\s\S]*\}' packages/create-burner/test/manager/burnerManager.test.ts # Test 3: Verify that individual test cases no longer contain the mock setup echo "Verifying removal of individual mock setups:" rg 'burnerManager\.provider\.getChainId = vi\.fn\(\)\.mockResolvedValue\("0x1"\);' packages/create-burner/test/manager/burnerManager.test.tsLength of output: 869
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/create-burner/test/manager/burnerManager.test.ts (1 hunks)
- packages/state/package.json (1 hunks)
Additional comments not posted (1)
packages/state/package.json (1)
11-11: Approved: Test script updated for single run executionThe change from
"test": "vitest"to"test": "vitest run"is a good modification. Here's why:
- The new command explicitly runs tests once and exits, which is ideal for CI/CD pipelines.
- It provides a clear pass/fail status, making it easier to integrate with automated build processes.
- For local development, developers can still use
vitest --watchif they need the watch mode.This change doesn't affect the test code itself, only how it's executed. It's a common and recommended practice for configuring test scripts in package.json.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (8)
packages/utils-wasm/package.json (1)
Line range hint
9-13: Add a "prepare" script for package buildingTo ensure the package is always built before it's published, consider adding a "prepare" script.
Add a "prepare" script to your "scripts" section:
"scripts": { "build-wasm": "cd crate && ./build.sh", "build": "npm run build-wasm && tsc", - "test": "echo \"Error: no test specified\" && exit 1" + "test": "vitest run", + "prepare": "npm run build" },This will automatically run the build process before the package is published, ensuring that the latest changes are always included in the published package.
packages/create-dojo/package.json (2)
Line range hint
20-36: Consider reviewing all dependenciesWhile the addition of Vitest is the main change in this PR, it might be a good opportunity to review all dependencies and devDependencies to ensure they are necessary and up-to-date.
You may want to run
npm outdatedor use a tool likenpm-check-updatesto identify any packages that could be updated. This can help maintain the project's security and take advantage of the latest features and bug fixes.Here's a script to check for outdated packages:
#!/bin/bash # Check for outdated packages echo "Checking for outdated packages:" npm outdated || echo "No outdated packages found."
Issue Found: Vitest is referenced in the
testscript but is not listed as a dependency.
- Ensure that Vitest is added to
devDependenciesinpackages/create-dojo/package.jsonto align thetestscript configuration with the project's dependencies.Analysis chain
Line range hint
18-18: Verify test script configurationThe
testscript is correctly configured to use Vitest with coverage. This aligns well with the newly added Vitest dependency.Let's verify if there are any existing test files that might benefit from this new configuration:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing test files echo "Searching for existing test files that might use Vitest:" rg -t js -t ts "import.*from 'vitest'" || echo "No existing Vitest imports found."Length of output: 807
Script:
#!/bin/bash # Search for imports from other testing frameworks in existing test files echo "Searching for existing test framework imports (e.g., Jest, Mocha):" rg -t js -t ts "import.*from '(jest|mocha)'" || echo "No imports from Jest or Mocha found."Length of output: 278
Script:
#!/bin/bash # Search for testing framework dependencies in package.json echo "Checking for testing frameworks in package.json dependencies and devDependencies:" rg -i '"(jest|mocha|vitest)"'\s*:\s*'"'" version "},{"'"'" dependencies | jq '.dependencies, .devDependencies | keys[]' | grep -E 'jest|mocha|vitest' || echo "No Jest, Mocha, or Vitest dependencies found."Length of output: 266
Script:
#!/bin/bash # Search for testing framework dependencies in package.json using jq echo "Checking for testing frameworks in package.json dependencies and devDependencies:" jq -r '.dependencies, .devDependencies | keys[]' package.json | grep -E 'jest|mocha|vitest' || echo "No Jest, Mocha, or Vitest dependencies found."Length of output: 367
.github/workflows/release.yaml (5)
15-26: LGTM: Initial setup steps are well-configured.The checkout action with
fetch-depth: 0is correct for changelog generation and versioning. Using a specific version of pnpm (v8) ensures consistency. The Git configuration correctly attributes commits to the GitHub actor.Consider pinning the pnpm version to a specific minor version (e.g., 8.x) for even more consistency:
- version: 8 + version: 8.xThis ensures you're always using the latest patch version within the 8.x series.
28-40: LGTM: npm and dependency setup looks good.The npm configuration for npmjs.org is correct. Installing the Protobuf compiler suggests the project uses Protocol Buffers, which is fine. Using pnpm for both installation and building ensures consistency.
Consider caching the pnpm dependencies to speed up the workflow:
- uses: actions/cache@v3 with: path: ~/.pnpm-store key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm-lock.yaml') }} restore-keys: | ${{ runner.os }}-pnpm-Add this step before the "Install dependencies" step to potentially reduce build times in subsequent runs.
42-45: Package publishing steps are generally good, but could be improved.Using Lerna for versioning and publishing is appropriate for monorepos. The conditional dist-tag for preminor releases is a good practice.
Consider the following improvements:
- Remove the
--force-publishflag unless absolutely necessary. This flag bumps versions of all packages, even if they haven't changed:- npx lerna version ${{ github.event.inputs.release_type }} --conventional-commits --yes --no-private --force-publish + npx lerna version ${{ github.event.inputs.release_type }} --conventional-commits --yes --no-private
- Add error handling to the publish command:
- npx lerna publish from-git --yes --dist-tag ${{ github.event.inputs.release_type == 'preminor' && 'next' || 'latest' }} + npx lerna publish from-git --yes --dist-tag ${{ github.event.inputs.release_type == 'preminor' && 'next' || 'latest' }} || echo "No packages to publish"This will prevent the workflow from failing if there are no changes to publish.
47-57: LGTM: Changelog generation and commit steps are well-implemented.Using lerna-changelog for generating changelogs is appropriate for a Lerna monorepo. Automatically committing and pushing the changelog is a good practice. The [skip ci] tag in the commit message correctly prevents unnecessary CI runs.
Consider adding a step to verify the changelog was actually updated before committing:
- name: Check if changelog was updated run: | if [ -z "$(git status --porcelain CHANGELOG.md)" ]; then echo "No changes to CHANGELOG.md" exit 0 fiAdd this step before the "Commit and Push Changelog" step to avoid creating empty commits when there are no changes to the changelog.
1-57: Overall, the release workflow is well-structured but could benefit from some enhancements.The workflow covers all necessary steps for a comprehensive release process, from setup to publishing and changelog generation. The use of secrets for npm authentication and GitHub operations is correct and secure.
Consider the following general improvements to enhance the workflow's robustness and maintainability:
- Add error handling and status checks throughout the workflow to make it more resilient to failures.
- Consider splitting the workflow into multiple jobs (e.g., build, test, publish) to improve clarity and potentially enable parallel execution.
- Add a step to run tests before publishing to ensure only validated code is released.
- Consider adding environment-specific configurations if you need to support multiple environments (e.g., staging, production).
- Add comments to explain complex steps or decisions in the workflow for better maintainability.
Example of adding a test step:
- name: Run tests run: pnpm testAdd this step after the "Build packages" step and before the "Tag and Publish Packages" step.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .github/workflows/release-minor.yaml (0 hunks)
- .github/workflows/release-preminor.yaml (0 hunks)
- .github/workflows/release-prepatch.yaml (0 hunks)
- .github/workflows/release-prerelease.yaml (0 hunks)
- .github/workflows/release.yaml (1 hunks)
- .github/workflows/release_patch.yaml (0 hunks)
- lerna.json (1 hunks)
- packages/create-dojo/package.json (1 hunks)
- packages/utils-wasm/package.json (1 hunks)
Files not reviewed due to no reviewable changes (5)
- .github/workflows/release-minor.yaml
- .github/workflows/release-preminor.yaml
- .github/workflows/release-prepatch.yaml
- .github/workflows/release-prerelease.yaml
- .github/workflows/release_patch.yaml
Additional comments not posted (4)
packages/create-dojo/package.json (2)
Line range hint
1-37: Overall assessment: Changes look goodThe addition of Vitest as a testing framework is a positive change that should improve the project's testing capabilities. The changes are minimal and focused, which aligns well with the PR title "fix: submodule".
To fully leverage this change, consider:
- Updating any existing tests to use Vitest syntax if necessary.
- Adding a Vitest configuration file if specific settings are needed.
- Updating project documentation to reflect the use of Vitest for testing.
34-35: LGTM: Addition of Vitest as a testing frameworkThe addition of
vitestas a dev dependency is consistent with thetestscript in thescriptssection. This change introduces a modern and fast testing framework to the project, which is a positive improvement.A few points to consider:
- The version
^2.1.1is relatively recent, which is good for having the latest features and bug fixes.- Make sure to update the project documentation to reflect the use of Vitest for testing.
- Consider adding a
.vitest.config.jsorvitest.config.tsfile to the project root for any Vitest-specific configurations.Let's verify if there are any existing test files that might need updating:
#!/bin/bash # Search for existing test files echo "Searching for existing test files:" fd -e test.ts -e test.js -e spec.ts -e spec.js # Check if there's a Vitest config file echo "Checking for Vitest config file:" fd vitest.config.github/workflows/release.yaml (2)
1-10: LGTM: Workflow name and trigger are well-defined.The workflow name "Release" is clear and descriptive. The manual trigger (
workflow_dispatch) provides control over when releases are made, and therelease_typeinput offers flexibility for different types of releases.
11-13: LGTM: Job setup is appropriate.The job name "release" clearly describes its purpose, and using the latest Ubuntu environment (
ubuntu-latest) ensures access to up-to-date tools and security patches.
| "npmClient": "pnpm", | ||
| "command": { | ||
| "publish": { | ||
| "conventionalCommits": true, | ||
| "yes": true | ||
| } | ||
| } |
There was a problem hiding this comment.
LGTM on the conventionalCommits option, but consider the implications of yes: true.
The addition of conventionalCommits: true is a great improvement. This will enforce a standardized commit message format, which is beneficial for:
- Automated versioning
- Generating changelogs
- Making the project history more readable and organized
However, I have a concern about the yes: true option.
While yes: true can streamline the publishing process by automatically confirming prompts, it might lead to accidental publishes or skipping important confirmations. Consider the following alternatives:
- Remove the
yesoption to maintain manual confirmation for critical publish steps. - Use environment variables or CLI flags for automation in CI/CD pipelines instead of hardcoding it in the configuration.
Example:
"command": {
"publish": {
"conventionalCommits": true
}
}This way, you can use lerna publish --yes when needed, without bypassing confirmations by default.
packages/utils-wasm/package.json
Outdated
| "wasm-pack": "^0.12.1", | ||
| "vitest": "^2.1.1" |
There was a problem hiding this comment.
Update test script to use Vitest
The addition of Vitest as a devDependency is a good step towards implementing tests for this package. However, the current "test" script in your package.json doesn't utilize Vitest.
Consider updating the "test" script to use Vitest. Here's a suggested change:
"scripts": {
"build-wasm": "cd crate && ./build.sh",
"build": "npm run build-wasm && tsc",
- "test": "echo \"Error: no test specified\" && exit 1"
+ "test": "vitest run"
},Also, consider adding a "test:watch" script for development:
"scripts": {
"build-wasm": "cd crate && ./build.sh",
"build": "npm run build-wasm && tsc",
- "test": "echo \"Error: no test specified\" && exit 1"
+ "test": "vitest run",
+ "test:watch": "vitest"
},Don't forget to create test files and configure Vitest as needed for your project structure.
Committable suggestion was skipped due to low confidence.
packages/utils-wasm/package.json
Outdated
| @@ -26,6 +26,7 @@ | |||
| "devDependencies": { | |||
| "tsup": "^8.0.1", | |||
| "typescript": "^5.5.4", | |||
There was a problem hiding this comment.
Correct TypeScript version
The specified TypeScript version "^5.5.4" does not exist as of the current date. This could lead to installation issues or unexpected behavior.
Please update the TypeScript version to a valid, current version. For example:
"devDependencies": {
"tsup": "^8.0.1",
- "typescript": "^5.5.4",
+ "typescript": "^5.0.4",
"wasm-pack": "^0.12.1",
"vitest": "^2.1.1"
}Verify the latest stable version of TypeScript and use that.
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.
| "typescript": "^5.5.4", | |
| "typescript": "^5.0.4", |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
examples/example-vite-react-pwa-recs/package.json (2)
19-21: Approve workspace-linked @dojoengine packages, but consider production implications.The change to use "workspace:*" for @dojoengine packages is good for development, ensuring the latest local versions are used. However, for production builds, consider using fixed versions to ensure consistency and reproducibility.
For production builds, you might want to add a script that replaces "workspace:*" with fixed versions before publishing or deploying.
Line range hint
1-54: Overall assessment: Positive package updates with some cautions.This update brings the project's dependencies up to date, which is generally a good practice. However, given the scope of changes, especially in core packages like @latticexyz and development tools like ESLint, it's crucial to thoroughly test the project to ensure compatibility.
Recommendations:
- Review changelogs for @latticexyz packages for any breaking changes.
- Run linter, tests, and build process to catch any issues introduced by the updates.
- Consider using fixed versions for @dojoengine packages in production builds.
- Manually test the application, paying special attention to features that rely on updated packages.
These updates should improve your project's security, performance, and developer experience, but caution and thorough testing are advised before merging to production.
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 (22)
- examples/example-nodejs-bot/package.json (1 hunks)
- examples/example-vanillajs-phaser-recs/package.json (2 hunks)
- examples/example-vite-react-app-recs/package.json (1 hunks)
- examples/example-vite-react-phaser-recs/package.json (1 hunks)
- examples/example-vite-react-pwa-recs/package.json (1 hunks)
- examples/example-vite-react-sdk/package.json (1 hunks)
- examples/example-vite-react-sdk/src/App.tsx (1 hunks)
- examples/example-vite-react-threejs-recs/package.json (1 hunks)
- examples/example-vue-app-recs/package.json (1 hunks)
- packages/core/package.json (1 hunks)
- packages/create-burner/package.json (1 hunks)
- packages/create-dojo/package.json (2 hunks)
- packages/create-dojo/src/index.ts (1 hunks)
- packages/react/package.json (2 hunks)
- packages/sdk/package.json (1 hunks)
- packages/sdk/src/index.ts (1 hunks)
- packages/state/package.json (2 hunks)
- packages/torii-client/package.json (1 hunks)
- packages/torii-wasm/package.json (1 hunks)
- packages/utils-wasm/package.json (1 hunks)
- packages/utils/package.json (1 hunks)
- scripts/build-packages.sh (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/create-dojo/package.json
- packages/state/package.json
- packages/utils-wasm/package.json
- scripts/build-packages.sh
Additional comments not posted (50)
packages/torii-client/package.json (3)
24-24: Approve moving TypeScript to devDependencies with version update.Moving TypeScript from
dependenciestodevDependenciesis a good practice, as it's typically only needed during development and build time. The version has also been updated to^5.6.2, which is beneficial for staying current with the latest TypeScript features and improvements.
23-23: Approve tsup version update.The
tsuppackage has been updated from^8.0.1to^8.3.0. This update likely includes bug fixes and performance improvements for the build process.
20-24: Verify compatibility and build process after dependency changes.The changes to the development dependencies may have implications for the build process or compatibility with other packages in the workspace.
Let's run a script to check for any potential issues:
examples/example-vanillajs-phaser-recs/package.json (3)
12-14: LGTM: DevDependencies updated to recent versions.The updates to typescript, vite, and vite-plugin-top-level-await are minor version bumps, which should maintain backwards compatibility while potentially including bug fixes and performance improvements.
Line range hint
1-29: Summary: Package dependencies updated successfully.This update includes minor version bumps for several development dependencies (typescript, vite, vite-plugin-top-level-await) and one production dependency (@latticexyz/utils). These changes are consistent with good maintenance practices, keeping the project up-to-date with the latest compatible versions of its dependencies.
The updates should provide bug fixes, performance improvements, and potentially new features (especially for @latticexyz/utils) while maintaining backwards compatibility. However, it's always a good practice to run the project's test suite after updating dependencies to ensure everything still works as expected.
24-24: LGTM: @latticexyz/utils updated to a newer minor version.The update of @latticexyz/utils from ^2.0.12 to ^2.2.8 is a minor version bump, which should maintain backwards compatibility. However, given the jump from 2.0.x to 2.2.x, there might be new features or improvements introduced.
It would be beneficial to check the changelog or release notes for @latticexyz/utils to see if there are any new features or improvements that could be leveraged in this project. Here's a script to help with that:
examples/example-vue-app-recs/package.json (5)
21-21: LGTM: vite-plugin-top-level-await version updateThe update from ^1.4.1 to ^1.4.4 for vite-plugin-top-level-await is a patch version bump, which likely includes bug fixes and minor improvements. This update is beneficial for maintaining the stability of your build process.
26-29: LGTM: DevDependencies updates - Review Vite changelogThe updates to devDependencies are beneficial for keeping your development environment up-to-date:
- @vitejs/plugin-vue: ^5.0.4 to ^5.1.4
- typescript: ^5.2.2 to ^5.6.2
- vite: ^5.1.4 to ^5.4.7
- vue-tsc: ^2.0.4 to ^2.1.6
These updates likely include bug fixes, performance improvements, and new features for your development tools.
It's recommended to review the changelogs for these updates, especially for Vite, as it has a more significant version jump. You can check the Vite changelog with:
#!/bin/bash # Fetch the changelog for Vite gh release view --repo vitejs/vite v5.4.7Ensure that these updates don't introduce any conflicts or breaking changes in your development workflow.
19-29: Summary: Dependency updates - Ensure thorough testingThis PR updates several dependencies and devDependencies to their latest minor or patch versions. While these updates are generally beneficial for security, performance, and access to new features, it's crucial to ensure they don't introduce any unexpected issues.
Recommendations:
- Review changelogs for all updated packages, especially Vue and Vite.
- Conduct thorough testing of the application to verify that all functionality remains intact.
- Pay special attention to any custom configurations or plugins that might be affected by these updates.
Run the following command to check if there are any peer dependency warnings after these updates:
#!/bin/bash # Check for peer dependency warnings npm lsIf any warnings or errors are found, address them before merging this PR.
19-19: LGTM: @latticexyz/utils version updateThe update from ^2.1.1 to ^2.2.8 for @latticexyz/utils is a minor version bump, which is good for keeping the project up-to-date with the latest features and bug fixes.
It's recommended to check the changelog for any notable updates or potential impacts on your project. You can run the following command to view the changes between these versions:
23-23: LGTM: Vue version update - Verify compatibilityThe update from ^3.4.19 to ^3.5.8 for Vue is a minor version bump, which may introduce new features and improvements. This update is generally beneficial for keeping your project up-to-date with the latest Vue capabilities.
However, it's crucial to:
- Review the Vue changelog for any significant changes or potential breaking updates.
- Thoroughly test your application to ensure compatibility with this new Vue version.
You can check the Vue changelog with the following command:
packages/utils/package.json (2)
32-34: Review significant dependency updates and check compatibility.
The update of @latticexyz/utils from ^2.0.0-next.11 to ^2.2.8 is a significant change, moving from a pre-release to a stable version. This update may introduce new features or changes that could benefit the project. Please review the changelog for any breaking changes or new capabilities that could be leveraged.
The mathjs update to ^12.4.3 is a minor version bump and should be backwards compatible.
The micro-starknet dependency remains unchanged at ^0.2.3. Given the updates to other dependencies, it's worth verifying that this version is still compatible and up-to-date with the rest of the project.
To ensure compatibility and to check for any potential issues, you may want to run the following commands:
#!/bin/bash # Description: Verify compatibility of dependencies and check for any warnings or deprecation notices # Test: Install dependencies and check for any warnings or errors npm install # Check the exit status if [ $? -eq 0 ]; then echo "Dependencies installed successfully. Please review any warnings or notices in the install output." else echo "Error: Dependency installation failed. Please review the error messages and resolve any conflicts." exit 1 fi # Test: Check if there's a newer version of micro-starknet available latest_version=$(npm view micro-starknet version) current_version=$(npm list micro-starknet | grep micro-starknet | awk '{print $2}' | sed 's/[^0-9.]*//g') if [ "$latest_version" != "$current_version" ]; then echo "Warning: A newer version of micro-starknet is available. Current: $current_version, Latest: $latest_version" echo "Consider updating micro-starknet to ensure compatibility with other updated dependencies." else echo "micro-starknet is up to date." fi
21-25: LGTM: DevDependencies updated appropriately.The updates to the devDependencies look good. All changes are minor version bumps, which should maintain backwards compatibility. However, it's always a good practice to review the changelogs of these packages, especially for typescript and the testing libraries (vitest), to check for any breaking changes or new features that could benefit the project.
To ensure there are no unexpected breaking changes, you may want to run the following command after updating:
packages/core/package.json (1)
24-28: LGTM! Routine dependency updates.The changes to the package versions appear to be routine maintenance updates. This is a good practice to keep dependencies up-to-date and potentially benefit from bug fixes and performance improvements.
To ensure these updates don't introduce any breaking changes or compatibility issues, please run the following verification steps:
After running these commands, please review the output for any warnings, errors, or failed tests that might indicate compatibility issues with the updated packages.
Also applies to: 32-32
examples/example-vite-react-sdk/package.json (9)
22-22: LGTM: Minor version update for @eslint/jsThe update from ^9.9.0 to ^9.11.1 for @eslint/js is a minor version bump, which typically includes bug fixes and small improvements. This change is unlikely to introduce breaking changes and helps keep the project up-to-date with the latest ESLint features and fixes.
23-23: LGTM: Patch update for @types/reactThe update from ^18.3.3 to ^18.3.9 for @types/react is a patch version bump. This update likely includes more accurate type definitions and bug fixes for React, which can improve the development experience and catch potential type-related issues earlier.
26-26: LGTM: Minor version update for eslintThe update from ^9.9.0 to ^9.11.1 for eslint aligns with the @eslint/js update. This minor version bump likely includes new linting rules, bug fixes, and performance improvements. Keeping ESLint up-to-date ensures the project benefits from the latest code quality checks and best practices.
27-27: Please clarify the reason for using a specific commit versionThe eslint-plugin-react-hooks dependency has been changed from ^5.1.0-rc.0 to a specific commit version 5.1.0-rc-fb9a90fa48-20240614. While this ensures consistency, it may limit automatic updates and potentially cause the project to miss future improvements or bug fixes.
Could you please explain the rationale behind choosing this specific commit? Is there a particular fix or feature needed from this version? Consider whether it would be beneficial to use a more general version specifier once the required changes are incorporated into a stable release.
28-28: LGTM: Patch update for eslint-plugin-react-refreshThe update from ^0.4.9 to ^0.4.12 for eslint-plugin-react-refresh is a patch version bump. This update likely includes bug fixes and minor improvements for the ESLint plugin that works with React Refresh, enhancing the development experience when working with React components.
30-30: LGTM: Minor version update for TypeScriptThe update from ^5.5.3 to ^5.6.2 for typescript is a minor version bump. This update likely includes new language features, performance improvements, and bug fixes.
It's recommended to review the TypeScript changelog for version 5.6 to be aware of any new features or potential breaking changes that might affect your project. You can find the changelog at: https://github.com/microsoft/TypeScript/releases
31-31: LGTM: Minor version update for typescript-eslintThe update from ^8.0.1 to ^8.7.0 for typescript-eslint is a minor version bump. This update likely includes new TypeScript-specific linting rules, bug fixes, and improvements in TypeScript code analysis. Keeping this package up-to-date ensures better code quality and consistency in your TypeScript codebase.
32-32: LGTM: Patch update for ViteThe update from ^5.4.1 to ^5.4.7 for vite is a patch version bump. This update likely includes bug fixes and minor improvements to the Vite build tool. Keeping Vite up-to-date ensures you have the latest performance enhancements and fixes for your development and build processes.
22-32: Summary of dependency updatesThe changes in this file consist primarily of minor and patch version updates to various development dependencies. These updates are generally beneficial as they include bug fixes, performance improvements, and new features. Key points to note:
- Most updates are incremental and should not introduce breaking changes.
- The TypeScript update (5.5.3 to 5.6.2) may introduce new language features worth exploring.
- The eslint-plugin-react-hooks dependency has been pinned to a specific commit, which may require further explanation.
Overall, these updates help keep the project current with the latest tools and best practices. However, it's recommended to:
- Review the TypeScript 5.6 changelog for any relevant new features or changes.
- Clarify the reason for using a specific commit for eslint-plugin-react-hooks.
- Consider running the project's test suite to ensure these updates don't introduce any unexpected issues.
To ensure these updates don't introduce any breaking changes, please run the project's test suite and verify that all tests pass successfully.
examples/example-nodejs-bot/package.json (2)
20-30: Approved with caution: Runtime dependencies updated.The updates to runtime dependencies look good overall, but some major version bumps require attention:
- Express updated from 4.18.2 to 4.21.0
- OpenAI updated from 4.20.1 to 4.63.0
These major updates may include breaking changes that could affect the application's behavior.
Please take the following actions:
Review the changelogs for Express and OpenAI to understand any breaking changes:
Run the following script to check for any usage of deprecated or changed APIs:
#!/bin/bash # Description: Search for potential usage of Express and OpenAI APIs that might have changed # Test: Search for Express middleware and routing usage rg --type typescript 'app\.use|app\.get|app\.post|app\.put|app\.delete|express\.' -A 5 # Test: Search for OpenAI API calls rg --type typescript 'openai\..*\(' -A 5 # Review the output to identify any API usage that might need updates based on the changelog information.
- After making any necessary adjustments, thoroughly test the application to ensure it functions correctly with the updated dependencies.
7-15: LGTM: DevDependencies updated.The updates to devDependencies look good. These minor version bumps typically include bug fixes and improvements.
To ensure these updates don't introduce any breaking changes in the development workflow, please run the following commands:
packages/torii-wasm/package.json (1)
44-45: Dependency updates look good, but PR title needs clarification.The updates to
tsup(^8.1.0 to ^8.3.0) andtypescript(^5.5.4 to ^5.6.2) are appropriate minor version bumps. These updates likely include new features and bug fixes without breaking changes.However, the PR title "fix: submodule" doesn't seem to accurately reflect these dependency updates. Could you please clarify the relationship between the submodule fix mentioned in the title and these dependency updates? If there's no direct connection, consider updating the PR title and description to better reflect the nature of these changes.
To ensure these updates don't introduce any unexpected issues, please run the following verification script:
packages/react/package.json (2)
26-31: LGTM: DevDependencies updated.The updates to devDependencies look good. Keeping dependencies up-to-date is important for security and accessing new features.
To ensure compatibility with the updated versions, please run the following script:
#!/bin/bash # Description: Verify the project builds successfully with the updated dependencies. # Test: Attempt to build the project npm run build # Check the exit status if [ $? -eq 0 ]; then echo "Build successful. Updates appear to be compatible." else echo "Build failed. Please check compatibility with updated dependencies." fiAlso applies to: 33-34
41-41: LGTM: Dependencies updated.The updates to dependencies look good. Keeping dependencies up-to-date is important for security and accessing new features.
To ensure compatibility with the updated versions, especially for @latticexyz/utils which had a more significant version jump, please run the following script:
Also applies to: 44-44, 47-47
packages/create-burner/package.json (5)
29-29: LGTM: Testing library update is appropriate.The update to @testing-library/react (^16.0.1) is a patch version increment. This update likely includes bug fixes and minor improvements.
44-47: LGTM: Production dependency updates look good.The updates to production dependencies are as follows:
- @scure/bip32: ^1.5.0
- get-starknet-core: ^3.3.3
These minor version updates likely include improvements and bug fixes for cryptographic operations and StarkNet interactions.
To ensure these updates don't introduce any unexpected behavior, please run the following command to check for any breaking changes or new features:
#!/bin/bash # Description: Check for breaking changes or new features in updated production dependencies npm info @scure/bip32@^1.5.0 get-starknet-core@^3.3.3Additionally, thoroughly test any functionality that relies on these libraries to ensure everything works as expected after the update.
36-40: Development tool updates look good, but careful testing is advised.The updates to various development tools are as follows:
- @vitest/coverage-v8: ^1.6.0
- jsdom: ^24.1.3 (major version update)
- tsup: ^8.3.0
- typescript: ^5.6.2
- vitest: ^1.6.0
These updates should bring improvements and bug fixes. However, the major version update for jsdom (24.x.x) may introduce breaking changes.
Please run the following commands to ensure everything works as expected after these updates:
Pay special attention to any tests or functionality that relies on jsdom, as it has undergone a major version update.
31-33: LGTM: Type definition updates look good.The updates to @types/js-cookie (^3.0.6), @types/node (^18.19.50), and @types/react (^18.3.9) are minor version increments for type definitions. These updates should improve type checking and IDE support.
To ensure compatibility, please run the following command to check for any TypeScript compilation errors:
26-27: LGTM: Babel package updates look good.The updates to @babel/core (^7.25.2) and @babel/preset-env (^7.25.4) are minor version increments. These updates likely include bug fixes and improvements.
To ensure compatibility, please run the following command to check for any breaking changes or deprecations:
examples/example-vite-react-app-recs/package.json (2)
Line range hint
1-45: Overall assessment: Positive update with minor concerns.This update to the package.json file keeps the project dependencies current, which is generally a good practice. It should bring bug fixes, performance improvements, and potentially new features from the updated packages.
However, there are a few points to consider:
- The update to React 18.3.1 (a pre-release version) may introduce instability or incompatibilities. Thorough testing is crucial.
- The TypeScript update to 5.6.2 might require code adjustments to comply with new type checking rules.
- ESLint updates might introduce new linting rules that could affect your codebase.
These changes are approved, but please ensure comprehensive testing is performed, especially focusing on React component behavior and TypeScript compatibility. If any issues arise, consider rolling back React to the latest stable version.
20-21: Verify compatibility with updated dependencies, especially React.The dependencies have been updated to newer versions. Most updates appear to be minor or patch versions, which should not introduce breaking changes. However, please note:
React and React DOM have been updated to version 18.3.1, which is a pre-release version. Ensure that this version is stable enough for your use case and compatible with other dependencies.
Other notable updates include:
- @latticexyz/react and @latticexyz/utils: 2.0.12 -> 2.2.8
- mobx: 6.9.0 -> 6.13.2
- vite-plugin-top-level-await: 1.3.1 -> 1.4.4
- vite-plugin-wasm: 3.2.2 -> 3.3.0
To ensure compatibility, run the following script to check for any breaking changes or deprecation warnings:
Also applies to: 23-23, 25-26, 29-30
packages/sdk/package.json (3)
Line range hint
1-62: Summary of package.json updatesThe changes in this file consist of routine dependency updates in both
devDependenciesanddependenciessections. These updates are important for maintaining the project's security, stability, and access to the latest features. The most significant updates are to TypeScript and zustand, for which I've provided verification steps.Overall, these changes look good and follow best practices for package maintenance. Remember to run your test suite and perform any necessary manual testing to ensure these updates haven't introduced any unexpected behavior.
25-31: Approve devDependencies updates with a suggestion.The updates to the devDependencies look good. Keeping dependencies up-to-date is a good practice for security and stability.
Given the significant update to TypeScript (5.5.4 to 5.6.2), it's recommended to review the changelog for any new features or potential breaking changes that might affect your codebase. You can check the TypeScript changelog here: https://github.com/microsoft/TypeScript/releases
To verify the impact of the TypeScript update, you can run the following script:
This script will help identify any immediate issues caused by the TypeScript update and highlight any TODO comments that might be related to the previous version.
39-41: Approve dependencies update with a suggestion.The update to zustand (4.5.2 to 4.5.5) is a minor patch and should only include backwards-compatible bug fixes. This change looks good.
It's recommended to review the changelog for zustand to understand what bug fixes or improvements have been included in this update. You can check the zustand changelog here: https://github.com/pmndrs/zustand/releases
To verify the impact of the zustand update and check its usage in the project, you can run the following script:
This script will help you identify where zustand is being used in your project and if there are any outstanding TODOs or FIXMEs related to it that might need attention after this update.
Verification successful
Zustand update verified successfully.
No related
TODOorFIXMEcomments were found that might impact the update.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check zustand usage and potential issues after the update # Test: Check for zustand imports and usage echo "Checking zustand usage in the project..." rg -i "import.*from ['\"]zustand['\"]" --type typescript # Test: Look for any zustand-related TODO or FIXME comments echo "Checking for zustand-related TODO or FIXME comments..." rg -i "todo|fixme.*zustand" --type typescriptLength of output: 390
Script:
#!/bin/bash # Description: Check zustand usage and potential issues after the update # Test: Check for zustand imports and usage in .ts and .tsx files echo "Checking zustand usage in the project..." rg -i "import.*from ['\"]zustand['\"]" --glob "*.ts" --glob "*.tsx" # Test: Look for any zustand-related TODO or FIXME comments in .ts and .tsx files echo "Checking for zustand-related TODO or FIXME comments..." rg -i "todo|fixme.*zustand" --glob "*.ts" --glob "*.tsx"Length of output: 1375
Script:
#!/bin/bash # Description: Check zustand usage and potential issues after the update # Test: Check for zustand imports and usage in .ts and .tsx files echo "Checking zustand usage in the project..." rg -i "import.*from ['\"]zustand['\"]" --glob "*.ts" --glob "*.tsx" # Test: Look for any zustand-related TODO or FIXME comments in .ts and .tsx files echo "Checking for zustand-related TODO or FIXME comments..." rg -i "(todo|fixme).*zustand" --glob "*.ts" --glob "*.tsx"Length of output: 664
examples/example-vite-react-pwa-recs/package.json (2)
38-51: Approve development dependency updates, but verify project compatibility.The updates to GraphQL packages, React and TypeScript type definitions, ESLint, and TypeScript are good improvements. However, these changes, especially the significant ESLint update, might affect your project.
Please run the following checks to ensure compatibility:
Run your linter to check for any new warnings or errors:
npm run lintRun your test suite to ensure no regressions:
npm testBuild your project to check for any TypeScript errors:
npm run build
24-24: Approve minor updates to various dependencies.The minor version updates to @vite-pwa/assets-generator, graphql, mobx, react, react-dom, and Vite plugins are good improvements. These updates likely include bug fixes and performance enhancements while maintaining backwards compatibility.
Also applies to: 26-26, 28-28, 30-31, 34-35
examples/example-vite-react-phaser-recs/package.json (3)
22-24: Major dependency updates require thorough testingSeveral dependencies have been updated to newer versions, with some significant jumps:
- @latticexyz/phaserx: 2.0.0-next.14 -> 2.0.0-transaction-context-af4b168c
- @latticexyz/utils: ^2.0.0-next.14 -> ^2.2.8
- react & react-dom: ^18.2.0 -> ^18.3.1
- mobx: ^6.9.0 -> ^6.13.2
- styled-components: ^6.0.7 -> ^6.1.13
- zustand: ^4.4.1 -> ^4.5.5
These updates may introduce new features, performance improvements, or breaking changes. Please ensure that:
- The application still builds and runs correctly after these updates.
- All features dependent on these libraries are thoroughly tested.
- Any breaking changes are addressed, especially for the significant version jumps.
- The project's documentation is updated if any API changes affect its usage.
To verify the impact of these updates, run the following build and test commands:
#!/bin/bash # Description: Build the project and run tests echo "Building the project:" npm run build echo "Running tests:" npm testNote: Adjust the commands if your project uses a different build tool or test runner.
Also applies to: 31-31, 34-35, 37-37, 39-40, 42-44
47-60: Review development tool outputs after devDependencies updateAll devDependencies have been updated to newer versions, including important development tools:
- TypeScript: ^5.2.2 -> ^5.6.2
- ESLint and related plugins: Various updates
- Vite: ^4.3.9 -> ^4.5.5
- Tailwind CSS: ^3.3.5 -> ^3.4.13
To ensure these updates don't introduce any issues or new warnings, please:
- Run the linter and address any new warnings or errors.
- Check the TypeScript compilation output for any new type errors.
- Verify that the development server (Vite) starts without issues.
- Review the build output for any new warnings or optimizations.
Run the following commands to check the output of updated development tools:
Address any new warnings or errors that may appear after running these commands.
18-19: Verify the impact of new workspace dependenciesNew workspace dependencies have been added:
- @dojoengine/state
- @dojoengine/torii-client
- @dojoengine/utils
These additions might indicate new features or restructuring of the codebase. Please ensure that:
- These dependencies are correctly implemented in the project.
- Any necessary documentation has been updated to reflect these changes.
- The project builds and runs correctly with these new dependencies.
To verify the usage of these new dependencies, run the following script:
Also applies to: 21-21
examples/example-vite-react-threejs-recs/package.json (3)
21-27: Approve dependency updates with caution.The updates to various dependencies, including React, Three.js, and UI libraries, are generally beneficial for security and performance. However, some updates are significant and may introduce breaking changes or new features that need to be properly integrated.
Key updates to note:
- React and React DOM from 18.2.0 to 18.3.1
- @react-three/drei from 9.93.1 to 9.114.0
- @react-three/fiber from 8.15.14 to 8.17.8
Please ensure thorough testing of the application, especially focusing on Three.js related functionality and any custom hooks or components that might be affected by the React update. Run the following script to check for any deprecated API usage:
#!/bin/bash # Description: Check for potential breaking changes in React and Three.js usage echo "Checking for potential React 18.3 breaking changes:" rg --type typescript --type javascript 'useId|useTransition|useDeferredValue|useInsertionEffect' echo "Checking for @react-three/drei API changes:" rg --type typescript --type javascript '@react-three/drei' echo "Checking for @react-three/fiber API changes:" rg --type typescript --type javascript '@react-three/fiber'Also applies to: 29-32, 34-34, 36-36, 38-38, 43-44, 46-47, 50-51, 53-53, 55-57
60-80: Approve devDependency updates and suggest configuration review.The updates to devDependencies, including TypeScript, ESLint, and Storybook-related packages, are beneficial for maintaining a modern development environment. However, some updates may require configuration changes or introduce new features that could be leveraged.
Key updates to note:
- Storybook packages from 7.6.10 to 7.6.20
- TypeScript from 5.5.4 to 5.6.2
- ESLint and related packages to latest 5.x versions
Please review and update configurations if necessary, especially for Storybook and TypeScript. Run the following script to check for any outdated configurations:
#!/bin/bash # Description: Check for potential configuration updates needed echo "Checking Storybook configuration:" rg --type yaml --type javascript 'storybook' .storybook echo "Checking TypeScript configuration:" cat tsconfig.json echo "Checking ESLint configuration:" cat .eslintrc.js || cat .eslintrc.json || cat .eslintrc.yamlAdditionally, consider updating the
lintscript in yourpackage.jsonto use the latest ESLint features:"lint": "eslint . --ext ts,tsx --report-unused-disable-directives --max-warnings 0"
18-20: Approve the addition of Dojo engine dependencies.The reintroduction of
@dojoengine/state,@dojoengine/torii-client, and@dojoengine/utilssuggests an expansion of Dojo-related functionality in the project. This change appears to be intentional and aligns with the project's direction.To ensure proper integration, please run the following script to check for any usage of these new dependencies:
Verification successful
Dojo engine dependencies are properly integrated.
All added dependencies are actively used across the codebase, ensuring their proper integration.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new Dojo engine dependencies echo "Checking usage of @dojoengine/state:" rg --type typescript --type javascript '@dojoengine/state' echo "Checking usage of @dojoengine/torii-client:" rg --type typescript --type javascript '@dojoengine/torii-client' echo "Checking usage of @dojoengine/utils:" rg --type typescript --type javascript '@dojoengine/utils'Length of output: 2327
Script:
#!/bin/bash # Description: Check for usage of new Dojo engine dependencies in .js and .ts files echo "Checking usage of @dojoengine/state:" rg '@dojoengine/state' --glob '*.js' --glob '*.ts' echo "Checking usage of @dojoengine/torii-client:" rg '@dojoengine/torii-client' --glob '*.js' --glob '*.ts' echo "Checking usage of @dojoengine/utils:" rg '@dojoengine/utils' --glob '*.js' --glob '*.ts'Length of output: 4006
examples/example-vite-react-sdk/src/App.tsx (1)
12-24:⚠️ Potential issueImproved configuration structure, but attention needed on some values.
The restructuring of the
init<Schema>parameters intoclientanddomainobjects improves the organization and readability of the configuration. However, there are a couple of points that need attention:
The
worldAddressis now hardcoded. Consider making this configurable, possibly by moving it to thedojoConfigobject for consistency with other client parameters.The
chainIdis set to a placeholder value "your-chain-id". Ensure this is replaced with the actual chain ID before deployment.To address these issues, consider the following changes:
- For the
worldAddress:- worldAddress: - "0x5d475a9221f6cbf1a016b12400a01b9a89935069aecd57e9876fcb2a7bb29da", + worldAddress: dojoConfig.worldAddress,
- For the
chainId, replace "your-chain-id" with the actual chain ID or make it configurable:- chainId: "your-chain-id", + chainId: dojoConfig.chainId, // Add this to dojoConfigTo ensure the
dojoConfigobject contains all necessary properties, run the following script:packages/sdk/src/index.ts (2)
Line range hint
1-174: Overall impact: Minor import optimization with no functional changes.The removal of
WeierstrassSignatureTypefrom the import statement is the only change in this file. This modification doesn't affect the existing functionality of the SDK, as the removed type wasn't used in any of the exported functions or types. The rest of the file, including the SDK initialization, entity and event query functions, and message signing functionality, remains unchanged.
8-8: LGTM: Import statement updated correctly.The removal of
WeierstrassSignatureTypefrom the import list appears to be a valid change, as it's not used in the rest of the file. The remaining imports (Account,Signature,StarknetDomain, andTypedData) are still necessary for the SDK's functionality.To ensure that
WeierstrassSignatureTypeis not used elsewhere in the file or project, run the following script:Verification successful
LGTM: Import statement updated correctly.
The removal of
WeierstrassSignatureTypefrom the import list is verified to have no impact on the main SDK codebase. Its usage is confined to example setup files, which do not affect the core functionality of the SDK.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that WeierstrassSignatureType is not used in the project # Test: Search for WeierstrassSignatureType in all TypeScript files # Expect: No occurrences found rg --type typescript 'WeierstrassSignatureType'Length of output: 82
Script:
#!/bin/bash # Description: Verify that WeierstrassSignatureType is not used in the project # Test: Search for WeierstrassSignatureType in all TypeScript files (.ts and .tsx) # Expect: No occurrences found rg 'WeierstrassSignatureType' --glob '*.ts' --glob '*.tsx'Length of output: 438
packages/create-dojo/src/index.ts (1)
29-29: Proper invocation of themainfunctionThe addition of
main()ensures that the command-line interface (CLI) executes the defined commands. This is a necessary change to make the CLI operational.
| @@ -8,7 +8,7 @@ | |||
| "type": "module", | |||
| "scripts": { | |||
| "build": "tsup --dts-resolve", | |||
| "test": "echo \"Error: no test specified\" && exit 1" | |||
| "test": "echo \"Error: no test specified\"" | |||
There was a problem hiding this comment.
Implement actual tests instead of using a placeholder.
The current test script is just a placeholder and doesn't actually run any tests. This could lead to a false sense of security if integrated into a CI/CD pipeline.
Consider implementing actual tests for this package. If tests are not ready yet, you might want to use a more explicit placeholder:
"test": "echo \"Warning: No tests implemented yet\" && exit 1"This will ensure that CI pipelines fail until proper tests are implemented.
| "@types/node": "^20.16.6", | ||
| "@types/react": "^18.3.9", | ||
| "@types/react-dom": "^18.3.0", | ||
| "@typescript-eslint/eslint-plugin": "^5.62.0", | ||
| "@typescript-eslint/parser": "^5.62.0", | ||
| "@vitejs/plugin-react": "^4.3.1", | ||
| "eslint": "^8.57.1", | ||
| "eslint-plugin-react-hooks": "^4.6.2", | ||
| "eslint-plugin-react-refresh": "^0.3.5", | ||
| "typescript": "^5.6.2", | ||
| "vite": "^4.5.5" |
There was a problem hiding this comment.
💡 Codebase verification
Missing Essential devDependencies Detected
The current devDependencies in package.json are missing several essential packages that are critical for your development environment and build process:
@types/node@types/react@types/react-dom@typescript-eslint/eslint-plugin@typescript-eslint/parser@vitejs/plugin-reacteslinteslint-plugin-react-hookseslint-plugin-react-refreshtypescriptvite
Analysis chain
Verify development environment compatibility with updated devDependencies.
The devDependencies have been updated to newer versions. These updates may affect your development environment and build process. Key changes include:
- TypeScript updated to 5.6.2: This may introduce new language features or type checking improvements.
- ESLint and related plugins updated: This might introduce new linting rules or changes in existing ones.
- Vite updated to 4.5.5: This could affect your build process or development server behavior.
To ensure these updates don't negatively impact your development workflow, run the following script:
After running these checks, manually verify that your development workflow (hot reloading, build process, etc.) still functions as expected.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify compatibility of updated devDependencies
# Test: Check if TypeScript compilation works without errors
npx tsc --noEmit
# Test: Run ESLint to check for any new linting issues
npx eslint .
# Test: Verify Vite build process
npm run build
echo "Please review the output of these commands for any unexpected errors or warnings."
Length of output: 436
Script:
#!/bin/bash
# Description: Extract and display devDependencies from package.json
if command -v jq >/dev/null 2>&1; then
jq '.devDependencies' package.json
else
echo "jq is not installed. Please install jq to parse JSON files."
fi
Length of output: 241
| "@latticexyz/react": "^2.2.8", | ||
| "@latticexyz/utils": "^2.2.8", |
There was a problem hiding this comment.
💡 Codebase verification
@latticexyz/react is not used in the codebase.
Consider removing it from package.json if it's unnecessary.
Analysis chain
Approve @latticexyz package updates, but verify compatibility.
The update of @latticexyz packages from 2.0.0-next.11 to 2.2.8 is a significant change. While it's good to use stable versions, this update might introduce breaking changes.
Please review the changelog for these packages and ensure that your code is compatible with any breaking changes. You can use the following command to check for potential issues:
Re-running API usage check with corrected file types.
The previous command encountered an error due to unrecognized file types. Please use the following corrected command to search for @latticexyz usage in TypeScript and JavaScript files:
Proceeding with additional verification for @latticexyz/react.
To ensure that the update of @latticexyz/react does not introduce any issues, please run the following command to search for its usage in the codebase:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of potentially changed APIs in @latticexyz packages
# Test: Search for @latticexyz usage in the codebase
rg --type typescript --type javascript '@latticexyz' -C 5
Length of output: 92
Script:
#!/bin/bash
# Description: Check for usage of potentially changed APIs in @latticexyz packages
# Test: Search for @latticexyz usage in the codebase
rg --type ts --type js '@latticexyz' -C 5
Length of output: 8469
Script:
#!/bin/bash
# Description: Check for usage of @latticexyz/react in the codebase
# Test: Search for @latticexyz/react usage in TypeScript and JavaScript files
rg --type ts --type js '@latticexyz/react' -C 5
Length of output: 47
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
.prettierignore (1)
Issues Found with Removal of Entries in .prettierignore
- The
packages/create-dojodirectory still exists and has references within the codebase, indicating incomplete removal.- References to
examples/dojo-starterstill exist in the codebase despite the directory being removed or renamed.Analysis chain
Line range hint
1-14: Verify removal ofexamples/dojo-starterandpackages/create-dojoThe AI summary mentions the removal of two entries from .prettierignore:
examples/dojo-starterpackages/create-dojoThese removals are not visible in the provided code snippet. Let's verify these changes and their implications.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of entries from .prettierignore and check for related structural changes # Test 1: Check if the entries have been removed from .prettierignore echo "Test 1: Checking .prettierignore for removed entries" if grep -qE "examples/dojo-starter|packages/create-dojo" .prettierignore; then echo "Warning: One or both of the supposedly removed entries still exist in .prettierignore" else echo "Confirmed: Both entries have been removed from .prettierignore" fi # Test 2: Check if the directories still exist in the project echo "Test 2: Checking if the directories still exist" if [ -d "examples/dojo-starter" ]; then echo "Note: 'examples/dojo-starter' directory still exists" else echo "Confirmed: 'examples/dojo-starter' directory has been removed or renamed" fi if [ -d "packages/create-dojo" ]; then echo "Note: 'packages/create-dojo' directory still exists" else echo "Confirmed: 'packages/create-dojo' directory has been removed or renamed" fi # Test 3: Check for any references to the old paths in the codebase echo "Test 3: Searching for references to old paths in the codebase" if rg -g '!.prettierignore' "examples/dojo-starter"; then echo "Warning: Found references to 'examples/dojo-starter' in the codebase" else echo "No references to 'examples/dojo-starter' found in the codebase" fi if rg -g '!.prettierignore' "packages/create-dojo"; then echo "Warning: Found references to 'packages/create-dojo' in the codebase" else echo "No references to 'packages/create-dojo' found in the codebase" fiLength of output: 1724
packages/create-dojo/src/commands/start.ts (1)
100-100: Consider refactoring the assignment in the expressionThe static analysis tool flagged an assignment in an expression:
(data) => (body += data). While this is valid JavaScript, it can be less readable and potentially confusing.Consider refactoring this line to make the assignment more explicit:
- res.on("data", (data) => (body += data)); + res.on("data", (data) => { + body += data; + });This change makes the assignment more clear and aligns with best practices for code readability.
Tools
Biome
[error] 100-100: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .github/workflows/ci.yaml (2 hunks)
- .prettierignore (1 hunks)
- packages/create-dojo/bin/index.d.ts (1 hunks)
- packages/create-dojo/bin/index.js (2 hunks)
- packages/create-dojo/src/commands/start.ts (1 hunks)
- packages/create-dojo/src/index.ts (1 hunks)
- packages/create-dojo/src/utils/get-package-info.ts (1 hunks)
- packages/create-dojo/tsconfig.json (2 hunks)
- packages/create-dojo/tsup.config.ts (1 hunks)
Files skipped from review due to trivial changes (4)
- packages/create-dojo/bin/index.d.ts
- packages/create-dojo/src/utils/get-package-info.ts
- packages/create-dojo/tsconfig.json
- packages/create-dojo/tsup.config.ts
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yaml
Additional context used
Biome
packages/create-dojo/src/commands/start.ts
[error] 100-100: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Additional comments not posted (17)
.prettierignore (1)
8-8: LGTM: Addition ofworlds/dojo-starterto .prettierignoreThe addition of
worlds/dojo-starterto the .prettierignore file is consistent with the project restructuring mentioned in the AI summary. This change ensures that the new location for the dojo-starter is properly ignored by Prettier.packages/create-dojo/src/index.ts (4)
2-9: LGTM: Imports and process handlers are correctly set up.The necessary imports for setting up the CLI application are present, and the process handlers for SIGINT and SIGTERM ensure graceful exit of the application.
28-28: LGTM: Main function invocation is correct.The
mainfunction is properly invoked, which is necessary for the program to run.
1-28:⚠️ Potential issueSummary: Major functionality removed, clarification needed on package purpose
This review has identified a significant reduction in the functionality of the
create-dojopackage. While the basic CLI structure remains, the core project setup logic has been removed. This change fundamentally alters the package's utility and purpose.To move forward:
- Please provide context for these changes. Was this intentional or accidental?
- If intentional, what is the new intended purpose of this package?
- Are there plans to reimplement the removed functionality elsewhere?
- How does this change align with the overall project goals and user expectations?
Given the scope of these changes, it would be beneficial to update the PR description with this information to provide clarity for all reviewers and users of the package.
To ensure we haven't missed any relocated functionality, let's check for new files that might contain the moved logic:
#!/bin/bash # Search for new files that might contain the relocated functionality fd -e ts -e js --changed-within 1week packages/create-dojo/src
11-26:⚠️ Potential issueClarification needed: Significant functionality has been removed.
The
mainfunction now only sets up a basic CLI structure without the previous project setup logic. This is a major change that significantly affects the utility of this package.
- Was the removal of the project setup logic (template selection, project creation, etc.) intentional?
- If so, what is the new intended purpose of this package?
- Are there plans to reimplement this functionality elsewhere or in a different way?
Please provide context for these changes to ensure they align with the project's goals.
To confirm the extent of the changes, let's check for any remaining references to the removed functionality:
packages/create-dojo/bin/index.js (6)
8-9: LGTM: Improved formatting forreposarrayThe indentation changes enhance readability while maintaining the original content.
12-27: LGTM: Enhanced readability ofinitfunctionThe indentation adjustments improve the code's structure without altering its functionality.
62-63: LGTM: Improved indentation ingetPackageInfofunctionThe indentation adjustments enhance code readability without altering the function's behavior.
70-80: LGTM: Enhanced structure ofmainfunctionThe indentation adjustments and separation of
program.parse()improve code readability while maintaining the original functionality.
Line range hint
1-83: Overall: Improved code formatting and readabilityThe changes in this file are consistently focused on enhancing code readability through improved formatting and indentation. No functional changes were made, and the original behavior of the code remains intact. These improvements will make the code easier to maintain and understand for future developers.
29-52: LGTM: Improved formatting ofstartcommandThe reformatting enhances code readability while maintaining the original functionality. The error handling for missing project name is correctly preserved.
To ensure the error handling is working as expected, we can run the following script:
This script will attempt to run the start command without providing a project name, which should trigger the error handling code.
packages/create-dojo/src/commands/start.ts (6)
9-29: Improved readability of templates arrayThe formatting changes to the templates array enhance code readability. Each template object is now properly indented, making the structure clearer.
33-71: Enhanced readability of init functionThe init function has been reformatted with improved indentation and spacing. These changes make the code more readable without altering its functionality.
73-89: Improved formatting of rewritePackageJson functionThe rewritePackageJson function has been reformatted with better indentation and spacing. These changes enhance code readability without changing the function's logic.
93-115: Enhanced readability of getLatestVersion functionThe getLatestVersion function has been reformatted with improved indentation and spacing, making the code more readable without changing its functionality.
Tools
Biome
[error] 100-100: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
120-147: Improved formatting of start command definitionThe start command definition has been reformatted with better indentation and spacing. These changes enhance code readability without altering the command's functionality.
Line range hint
1-147: Overall improvement in code formatting and readabilityThe changes in this file consistently enhance code readability through improved formatting, indentation, and spacing. These modifications make the code more maintainable without altering its functionality. The structure and logic of the file remain intact, ensuring that the
startcommand and its supporting functions continue to work as intended.Tools
Biome
[error] 100-100: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Summary by CodeRabbit
dojo-startersubmodule fromexamples/dojo-startertoworlds/dojo-starter.dojo-starterdirectory and upgraded thedojoupcommand version.vitest runcommand for improved execution clarity.vitestas a new development dependency in multiple packages for enhanced testing capabilities.testscript from thecreate-dojopackage for streamlined configuration.