Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: create-dojo + update manifest path #338

Merged
merged 1 commit into from
Nov 26, 2024
Merged

fix: create-dojo + update manifest path #338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/example-vite-kitchen-sink/dojoConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { createDojoConfig } from "@dojoengine/core";

import manifest from "../../worlds/dojo-starter/manifest_dev.json";
import manifest from "../../worlds/onchain-dash/manifests/release/deployment/manifest.json";
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Manifest path is incorrect and inconsistent with other examples

The verification reveals critical issues:

  • The specified manifest path worlds/onchain-dash/manifests/release/deployment/manifest.json does not exist
  • All other example projects consistently use worlds/dojo-starter/manifest_dev.json
  • Only the kitchen-sink example deviates with two files referencing the non-existent path:
    • examples/example-vite-kitchen-sink/dojoConfig.ts
    • examples/example-vite-kitchen-sink/src/dojo.config.ts

The change should be reverted to use the same development manifest as other examples (worlds/dojo-starter/manifest_dev.json) to maintain consistency and ensure the example works out of the box.

🔗 Analysis chain

Verify the manifest path exists and is accessible

The manifest path has been changed to point to a release manifest in the onchain-dash project. Let's verify this path exists and is consistent across the repository.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the manifest path exists and check for other manifest references

# Check if the manifest file exists
if [ -f "worlds/onchain-dash/manifests/release/deployment/manifest.json" ]; then
    echo "✓ Manifest file exists"
else
    echo "✗ Manifest file not found"
fi

# Search for other manifest references to ensure consistency
echo "Checking other manifest references:"
rg -g '!*.json' "manifest.*\.json"

Length of output: 1925

🛠️ Refactor suggestion

Consider making the manifest path configurable

The hardcoded relative path to the manifest file might be problematic:

  1. It assumes a specific directory structure
  2. It's using a release manifest for what appears to be a development/example setup
  3. The relative path (../../) could break if the file structure changes

Consider making the manifest path configurable through environment variables:

-import manifest from "../../worlds/onchain-dash/manifests/release/deployment/manifest.json";
+import manifest from process.env.DOJO_MANIFEST_PATH || "../../worlds/onchain-dash/manifests/release/deployment/manifest.json";

This would allow developers to:

  • Override the manifest path for local development
  • Use different manifests for different environments
  • Avoid breaking changes if the directory structure changes

Committable suggestion skipped: line range outside the PR's diff.


export const dojoConfig = createDojoConfig({
manifest,
Expand Down
2 changes: 1 addition & 1 deletion examples/example-vite-kitchen-sink/src/main.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import "./app/globals.css";
import { init } from "@dojoengine/sdk";
import { OnchainDashSchemaType, schema } from "@/dojo/models";
import { env, getRpcUrl } from "@/env";
import { dojoConfig } from "@/dojo.config";
import { dojoConfig } from "../dojoConfig";
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Path alias change is inconsistent with project configuration

The change from @/dojo.config to ../dojoConfig should be reverted as the project explicitly configures the @ path alias in both:

  • tsconfig.json with "@/*": ["./src/*"]
  • vite.config.ts with a corresponding Vite alias configuration

Additionally, there are 42 other imports using the @ alias pattern throughout the codebase, making this change inconsistent with the project's established path resolution strategy.

🔗 Analysis chain

Review build configuration for path alias changes

The change from @/dojo.config to ../dojoConfig removes usage of the @ path alias. Ensure that this change is intentional and consistent with the project's build configuration (tsconfig.json/vite.config.ts).

Let me check the build configuration files to complete the verification.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check build configurations for path alias definitions
# Expected: Confirm if @ alias should be removed or maintained

# Check TypeScript config
fd -e json "tsconfig" examples/example-vite-kitchen-sink/

# Check Vite config
fd -e ts -e js "vite.config" examples/example-vite-kitchen-sink/

# Look for other @ alias usages that might need similar updates
rg "from\s+['\"]@/" examples/example-vite-kitchen-sink/src/

Length of output: 5355

Script:

#!/bin/bash
# Check the content of TypeScript and Vite configs
cat examples/example-vite-kitchen-sink/tsconfig.json
cat examples/example-vite-kitchen-sink/vite.config.ts

Length of output: 1225

import { DojoContext } from "@/dojo/provider";

async function main() {
Expand Down
51 changes: 32 additions & 19 deletions packages/create-dojo/src/commands/start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,12 @@ const templates = [
async function init(projectName: string, cwd: string, template: string) {
const projectPath = path.join(cwd, projectName);
const clientPath = path.join(projectPath, "client");
const dojoStarterPath = path.join(projectPath, "dojo-starter");
const contractPath = path.join(projectPath, "contract");

// Create project directories
await fs.mkdir(projectPath, { recursive: true });
await fs.mkdir(clientPath, { recursive: true });
await fs.mkdir(dojoStarterPath, { recursive: true });
await fs.mkdir(contractPath, { recursive: true });

// Clone template into client directory
console.log(`Downloading ${template} into client directory...`);
Expand All @@ -66,26 +66,21 @@ async function init(projectName: string, cwd: string, template: string) {
// Rewrite package.json in client directory
await rewritePackageJson(projectName, clientPath);

console.log(`Cloning dojo-starter repository...`);
const gitCloneResult = spawn.sync(
"git",
[
"clone",
"https://github.com/dojoengine/dojo-starter.git",
dojoStarterPath,
],
{ stdio: "inherit" }
);

if (gitCloneResult.status !== 0) {
throw new Error(`Failed to clone dojo-starter repository.`);
}
// Update dojoConfig.ts imports
await rewriteDojoConfigFile(clientPath);

// Clone dojo-starter
console.log(`Downloading dojo-starter...`);
spawn.sync("npx", ["degit", `dojoengine/dojo-starter`, dojoStarterPath], {
stdio: "inherit",
});
const contractRes = spawn.sync(
"npx",
["degit", `dojoengine/dojo-starter`, contractPath],
{
stdio: "inherit",
}
);
if (contractRes.status !== 0) {
throw new Error(`Failed to clone template: ${template}`);
}
Comment on lines +74 to +83
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix incorrect error message in contract download

The error message still references the template variable when failing to clone the dojo-starter repository.

Apply this fix:

    if (contractRes.status !== 0) {
-       throw new Error(`Failed to clone template: ${template}`);
+       throw new Error('Failed to clone dojo-starter repository');
    }
📝 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.

Suggested change
const contractRes = spawn.sync(
"npx",
["degit", `dojoengine/dojo-starter`, contractPath],
{
stdio: "inherit",
}
);
if (contractRes.status !== 0) {
throw new Error(`Failed to clone template: ${template}`);
}
const contractRes = spawn.sync(
"npx",
["degit", `dojoengine/dojo-starter`, contractPath],
{
stdio: "inherit",
}
);
if (contractRes.status !== 0) {
throw new Error('Failed to clone dojo-starter repository');
}


console.log(`Project initialized at ${projectPath}`);
console.log("Congrats! Your new project has been set up successfully.\n");
Expand Down Expand Up @@ -116,6 +111,24 @@ async function rewritePackageJson(projectName: string, clientPath: string) {
await fs.writeFile(packageJsonPath, JSON.stringify(packageJson, null, 2));
}

async function rewriteDojoConfigFile(clientPath: string) {
const dojoConfigPath = path.join(clientPath, "dojoConfig.ts");

try {
let content = await fs.readFile(dojoConfigPath, "utf-8");

// Update relative imports to account for new directory structure
content = content.replace(
/from ['"]\.{0,2}\/.*manifest(?:_dev)?\.json['"]/g,
'from "../contract/target/dev/manifest.json"'
);

await fs.writeFile(dojoConfigPath, content, "utf-8");
} catch (error) {
console.warn(`Warning: Could not update dojoConfig.ts: ${error}`);
}
}

Comment on lines +114 to +131
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Revise manifest path handling in rewriteDojoConfigFile

The current implementation needs adjustment based on the examples:

  1. The target path "../contract/target/dev/manifest.json" is incorrect. Examples show two common patterns:

    • Most examples use: "../../worlds/dojo-starter/manifest_dev.json"
    • Kitchen sink uses: "../../worlds/onchain-dash/manifests/release/deployment/manifest.json"

  2. The regex pattern should be more specific but flexible enough to handle both patterns.

 async function rewriteDojoConfigFile(clientPath: string) {
     const dojoConfigPath = path.join(clientPath, "dojoConfig.ts");
+    const manifestPath = "../../worlds/dojo-starter/manifest_dev.json";
 
     try {
         let content = await fs.readFile(dojoConfigPath, "utf-8");
 
         content = content.replace(
-            /from ['"]\.{0,2}\/.*manifest(?:_dev)?\.json['"]/g,
-            'from "../contract/target/dev/manifest.json"'
+            /from ['"](?:\.{1,2}\/)+(?:worlds|contract)\/.*?manifest(?:_dev)?\.json['"]/g,
+            `from "${manifestPath}"`
         );
 
         await fs.writeFile(dojoConfigPath, content, "utf-8");
     } catch (error) {
         throw new Error(
             `Failed to update manifest path in dojoConfig.ts: ${error}. ` +
             `Please ensure the file exists and contains a manifest import.`
         );
     }
 }
🔗 Analysis chain

Enhance robustness of config file rewriting

Several improvements could make this function more reliable:

  1. The regex pattern could be more specific to avoid unintended matches
  2. The hardcoded path assumes a specific structure
  3. Error handling should be more robust for a critical setup step

Consider this enhanced implementation:

 async function rewriteDojoConfigFile(clientPath: string) {
     const dojoConfigPath = path.join(clientPath, "dojoConfig.ts");
+    const manifestPath = "../contract/target/dev/manifest.json";
 
     try {
         let content = await fs.readFile(dojoConfigPath, "utf-8");
 
-        // Update relative imports to account for new directory structure
         content = content.replace(
-            /from ['"]\.{0,2}\/.*manifest(?:_dev)?\.json['"]/g,
-            'from "../contract/target/dev/manifest.json"'
+            /from ['"](\.{0,2}\/[^"']*?manifest(?:_dev)?\.json)['"]/g,
+            `from "${manifestPath}"`
         );
 
         await fs.writeFile(dojoConfigPath, content, "utf-8");
     } catch (error) {
-        console.warn(`Warning: Could not update dojoConfig.ts: ${error}`);
+        throw new Error(
+            `Failed to update manifest path in dojoConfig.ts: ${error}. ` +
+            `Please ensure the file exists and contains a manifest import.`
+        );
     }
 }

Let's verify the manifest path structure:

This will help us verify the actual import paths used in the examples to ensure our regex and target path assumptions are correct.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the manifest path structure is consistent across templates
for template in $(fd -t d -d 1 . examples/ | grep 'example-'); do
  rg -l "manifest(_dev)?\.json" "$template"
done

Length of output: 1956

Script:

#!/bin/bash
# Check the actual manifest import paths in dojoConfig files
for config in $(rg -l "manifest(_dev)?\.json" examples/); do
  echo "=== $config ==="
  rg -A 1 -B 1 "from ['\"].*manifest(_dev)?\.json['\"]" "$config"
done

Length of output: 4433

async function getLatestVersion(): Promise<string> {
return new Promise((resolve, reject) => {
https
Expand Down