fix(create): Fix resolving path in repos using hoisted node_modules #3802
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughReplaces hard-coded node_modules lookups with a new exported helper Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as create-vendure-app
participant Resolver as resolvePackageRootDir
participant FS as Filesystem
participant Core as @vendure/core
participant Email as @vendure/email-plugin
Note over Resolver,FS: resolvePackageRootDir uses require.resolve and ascends dirs\nor falls back to cwd/node_modules
User->>CLI: run npx @vendure/create
CLI->>Resolver: resolvePackageRootDir('ts-node')
Resolver->>FS: require.resolve + ascend dirs
FS-->>Resolver: package root path
Resolver-->>CLI: path(ts-node)
CLI->>Resolver: resolvePackageRootDir('@vendure/core')
Resolver->>FS: require.resolve + ascend dirs
Resolver-->>CLI: path(@vendure/core)
CLI->>Resolver: resolvePackageRootDir('@vendure/email-plugin')
Resolver-->>CLI: path(email-plugin)
CLI->>Core: require(cli/populate) & require(dist/index) via resolved paths
CLI->>FS: copy email templates from resolved email-plugin path to project
alt copy fails
CLI->>User: log "Failed to copy email templates"
CLI->>CLI: process.exit(1)
else success
CLI-->>User: continue scaffold
end
alt any unhandled error
CLI->>User: log error
CLI->>CLI: process.exit(1)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/create/src/create-vendure-app.ts (1)
270-271: Hoist issue remains: direct node_modules pathing for mysql/pgThese imports will fail in hoisted workspaces. Resolve by package id.
-const mysql = await import(path.join(root, 'node_modules/mysql')); +const mysqlModule = await import('mysql'); +const mysql = (mysqlModule as any).default ?? (mysqlModule as any);-const { Client } = await import(path.join(root, 'node_modules/pg')); +const { Client } = await import('pg');Also applies to: 300-301
🧹 Nitpick comments (3)
packages/create/src/create-vendure-app.ts (3)
73-76: Top-level catch + exit(1) is goodPrevents hanging on failures and surfaces an error code. Consider logging err.stack when available for easier diagnostics.
195-196: Typo in user-facing message“inst all” → “install”.
-outro(pc.red(`Failed to inst all dependencies. Please try again.`)); +outro(pc.red(`Failed to install dependencies. Please try again.`));
301-301: Optional: simpler ts-node registrationThis avoids filesystem resolution and is hoist-safe.
-require(path.join(resolveDirName('ts-node'))).register(); +require('ts-node').register(); +// or: +// require('ts-node/register');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/create/src/create-vendure-app.ts(4 hunks)packages/create/src/helpers.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/create/src/create-vendure-app.ts (2)
packages/create/src/logger.ts (1)
log(10-24)packages/create/src/helpers.ts (1)
resolveDirName(536-544)
🔇 Additional comments (1)
packages/create/src/create-vendure-app.ts (1)
27-28: Importing resolveDirName here makes senseThis aligns the file with the new helper.
Comment on lines
500
to
508
| const emailPackageDirname = resolveDirName('@vendure/email-plugin'); | ||
| const templateDir = path.join(emailPackageDirname, '/templates'); | ||
| try { | ||
| await fs.copy(templateDir, path.join(root, 'static', 'email', 'templates')); | ||
| } catch (err: any) { | ||
| log(pc.red('Failed to copy email templates.')); | ||
| log(err); | ||
| process.exit(0); | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: Wrong template path and inappropriate exit(0) on error
- Leading slash in path.join makes templateDir absolute (/templates).
- Exiting with 0 masks failure and aborts the rest of the scaffolding.
-const emailPackageDirname = resolveDirName('@vendure/email-plugin');
-const templateDir = path.join(emailPackageDirname, '/templates');
+const emailPackageDirname = resolveDirName('@vendure/email-plugin');
+const templateDir = path.join(emailPackageDirname, 'templates');
try {
await fs.copy(templateDir, path.join(root, 'static', 'email', 'templates'));
} catch (err: any) {
- log(pc.red('Failed to copy email templates.'));
- log(err);
- process.exit(0);
+ log(pc.yellow('Warning: failed to copy email templates; continuing without them.'));
+ log(err?.message ?? String(err), { level: 'verbose' });
+ return;
}📝 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 emailPackageDirname = resolveDirName('@vendure/email-plugin'); | |
| const templateDir = path.join(emailPackageDirname, '/templates'); | |
| try { | |
| await fs.copy(templateDir, path.join(root, 'static', 'email', 'templates')); | |
| } catch (err: any) { | |
| log(pc.red('Failed to copy email templates.')); | |
| log(err); | |
| process.exit(0); | |
| } | |
| const emailPackageDirname = resolveDirName('@vendure/email-plugin'); | |
| const templateDir = path.join(emailPackageDirname, 'templates'); | |
| try { | |
| await fs.copy(templateDir, path.join(root, 'static', 'email', 'templates')); | |
| } catch (err: any) { | |
| log(pc.yellow('Warning: failed to copy email templates; continuing without them.')); | |
| log(err?.message ?? String(err), { level: 'verbose' }); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In packages/create/src/create-vendure-app.ts around lines 500 to 508, the
templateDir is built with a leading slash which makes it resolve to an absolute
"/templates" path and the catch uses process.exit(0) which incorrectly signals
success; change the path construction to join the package dirname with
'templates' (no leading slash) so it points to the package's templates folder,
and in the catch log the error with context and terminate with a non-zero exit
(e.g. process.exit(1)) so failures are not masked.
michaelbromley
requested changes
Sep 8, 2025
Member
michaelbromley
left a comment
michaelbromley
left a comment
There was a problem hiding this comment.
2 very tiny changes please 🙏
fix(create): Commit new
HouseinIsProgramming
force-pushed
the
fix-create-command
branch
from
46eaacf to
4a36ed5
Compare
|
michaelbromley
approved these changes
Sep 8, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3506
Description
This PR adds a new helper function that uses
require.resolve()to find the package directory instead of usingroot + /path-to-package.This solves the problem where node_modules are hoisted (moved up the directory tree), causing the hardcoded path approach to fail when packages aren't in the expected location.
Before:
root + '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/node_modules/package-name'After:
require.resolve('package-name')to dynamically locate the packageAlso adds
process.exit(0)on errors to prevent "locking the console".Checklist
📌 Always:
👍 Most of the time:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores