Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix: fix path resolution issue in cli.ts #1748

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0xbryer
Copy link

@0xbryer 0xbryer commented Dec 17, 2024

Description

This update addresses an issue with path resolution in the following code snippet:

Previous Code

const sourceDir = path.resolve(  
  fileURLToPath(import.meta.url),   
  '../../../templates/next'  
);  

Problem

The fileURLToPath(import.meta.url) function returns the absolute path to the current file, but passing it directly into path.resolve without using path.dirname can cause problems. This is particularly an issue if the file's path contains special characters (e.g., %20 for spaces), as it may not be decoded properly.

Fix

The code now explicitly extracts the directory name using path.dirname, ensuring robust path resolution:

const sourceDir = path.resolve(  
  path.dirname(fileURLToPath(import.meta.url)),   
  '../../../templates/next'  
);  

Why This Matters

  • fileURLToPath(import.meta.url) provides the file path, not the directory.
  • Using path.dirname ensures the proper resolution of relative paths, making the code more reliable and compatible with various environments.
  • The updated approach avoids potential issues with encoded characters in URLs.

This change improves the overall resilience and compatibility of the file resolution logic.

Copy link

vercel bot commented Dec 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
onchainkit-coverage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 17, 2024 11:48am
onchainkit-routes ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 17, 2024 11:48am

Copy link

vercel bot commented Dec 17, 2024

@0xbryer is attempting to deploy a commit to the Coinbase Team on Vercel.

A member of the Team first needs to authorize it.

@dschlabach dschlabach changed the title fix: Fix path resolution issue Update cli.ts fix: fix path resolution issue in cli.ts Dec 17, 2024
@@ -14,7 +14,7 @@ import {
} from './utils.js';

const sourceDir = path.resolve(
fileURLToPath(import.meta.url),
path.dirname(fileURLToPath(import.meta.url)),
'../../../templates/next'
Copy link
Contributor

Choose a reason for hiding this comment

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

What OS are you using? Is this working for you locally as you have in your commit?

Suggested change
'../../../templates/next'
'../../templates/next'

Copy link
Author

Choose a reason for hiding this comment

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

macOS, works fine

Copy link
Contributor

Choose a reason for hiding this comment

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

How are you running the script?

You'll need to ensure that it works after running these commands from create-onchain root:

bun run build
node dist/esm/cli.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants