fix(dev-cli): Add warning about Turbopack usage #6189
Conversation
🦋 Changeset detectedLatest commit: ad9684d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughA new changeset was added to document a patch update for the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (1)
packages/dev-cli/src/commands/setup.js (1)
62-90: Consider simplifying the path resolution logic.The
getOutputFileTracingRootfunction works correctly but is quite complex. Consider using Node.js built-in utilities for better readability and maintainability.async function getOutputFileTracingRoot() { const monorepoRoot = await getMonorepoRoot(); if (!monorepoRoot) { throw new Error(NULL_ROOT_ERROR); } - const p1 = path.resolve(monorepoRoot); - const p2 = path.resolve(process.cwd()); - - const root1 = path.parse(p1).root; - const root2 = path.parse(p2).root; - - if (root1 !== root2) return null; - - const parts1 = p1.slice(root1.length).split(path.sep); - const parts2 = p2.slice(root2.length).split(path.sep); - - const len = Math.min(parts1.length, parts2.length); - const common = []; - for (let i = 0; i < len; i++) { - if (parts1[i] === parts2[i]) common.push(parts1[i]); - else break; - } - - return common.length ? path.join(root1, ...common) : root1; + + const resolved1 = path.resolve(monorepoRoot); + const resolved2 = path.resolve(process.cwd()); + + // Find common path using path.relative and checking if it goes up + const relative = path.relative(resolved1, resolved2); + if (relative.startsWith('..')) { + // Current dir is not within monorepo root, find common ancestor + const relative2 = path.relative(resolved2, resolved1); + if (relative2.startsWith('..')) { + // Both paths have different roots, find lowest common denominator + return path.dirname(path.resolve(resolved1, relative.split(path.sep).filter(p => p === '..').map(() => '..').join(path.sep))); + } + } + + return resolved1; }Or consider using a well-tested library like
find-common-dirif available in the project dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/easy-papers-hug.md(1 hunks)packages/dev-cli/src/commands/setup.js(6 hunks)packages/dev-cli/src/commands/watch.js(2 hunks)packages/dev-cli/src/utils/getDependencies.js(0 hunks)packages/dev-cli/src/utils/getPackageJSON.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/dev-cli/src/utils/getDependencies.js
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.{js,ts,tsx,jsx}`: All code must pass ESLint checks with the project's configuration. Use Prettier for consistent code formatting.
**/*.{js,ts,tsx,jsx}: All code must pass ESLint checks with the project's configuration.
Use Prettier for consistent code formatting.
packages/dev-cli/src/commands/watch.jspackages/dev-cli/src/utils/getPackageJSON.jspackages/dev-cli/src/commands/setup.js
`packages/**`: All publishable packages under the @clerk namespace must be located in the packages/ directory.
packages/**: All publishable packages under the @clerk namespace must be located in the packages/ directory.
packages/dev-cli/src/commands/watch.jspackages/dev-cli/src/utils/getPackageJSON.jspackages/dev-cli/src/commands/setup.js
🪛 Biome (1.9.4)
packages/dev-cli/src/commands/setup.js
[error] 254-254: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
.changeset/easy-papers-hug.md (1)
1-6: LGTM!The changeset properly documents the patch update with a clear description of the new Turbopack warning functionality.
packages/dev-cli/src/commands/watch.js (1)
8-8: LGTM!The refactor to use
getPackageJSONinstead of the removedgetDependenciesutility is clean and maintains the same functionality.Also applies to: 17-17
packages/dev-cli/src/commands/setup.js (3)
39-43: Excellent refactoring of detectFramework to be synchronous.Making
detectFrameworksynchronous and accepting apkgJSONparameter improves testability and follows the single responsibility principle. The function is now pure and easier to reason about.
254-259: LGTM! Excellent implementation of Turbopack detection and warning.The logic correctly detects Turbopack usage in the Next.js dev script and provides a helpful warning with the computed
outputFileTracingRootvalue. This addresses the PR objectives perfectly.
195-195: LGTM!The update to use
getPackageJSONinstead ofgetDependenciesmaintains the same functionality while using the new centralized utility.
| export async function getPackageJSON(pathToPackageJSON) { | ||
| const packageJSON = await readFile(pathToPackageJSON, 'utf-8'); | ||
| return JSON.parse(packageJSON); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations and JSON parsing.
The function lacks error handling for common failure scenarios such as file not found or invalid JSON syntax. Consider adding try-catch blocks to provide more meaningful error messages.
export async function getPackageJSON(pathToPackageJSON) {
- const packageJSON = await readFile(pathToPackageJSON, 'utf-8');
- return JSON.parse(packageJSON);
+ try {
+ const packageJSON = await readFile(pathToPackageJSON, 'utf-8');
+ return JSON.parse(packageJSON);
+ } catch (error) {
+ if (error.code === 'ENOENT') {
+ throw new Error(`package.json not found at: ${pathToPackageJSON}`);
+ }
+ if (error instanceof SyntaxError) {
+ throw new Error(`Invalid JSON in package.json at: ${pathToPackageJSON}`);
+ }
+ throw error;
+ }
}📝 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.
| export async function getPackageJSON(pathToPackageJSON) { | |
| const packageJSON = await readFile(pathToPackageJSON, 'utf-8'); | |
| return JSON.parse(packageJSON); | |
| } | |
| export async function getPackageJSON(pathToPackageJSON) { | |
| try { | |
| const packageJSON = await readFile(pathToPackageJSON, 'utf-8'); | |
| return JSON.parse(packageJSON); | |
| } catch (error) { | |
| if (error.code === 'ENOENT') { | |
| throw new Error(`package.json not found at: ${pathToPackageJSON}`); | |
| } | |
| if (error instanceof SyntaxError) { | |
| throw new Error(`Invalid JSON in package.json at: ${pathToPackageJSON}`); | |
| } | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/dev-cli/src/utils/getPackageJSON.js around lines 8 to 11, the
function getPackageJSON lacks error handling for file reading and JSON parsing
failures. Wrap the file reading and JSON.parse calls in a try-catch block, and
in the catch block, throw or log a meaningful error message indicating whether
the failure was due to file access issues or invalid JSON syntax.
| @@ -217,6 +250,13 @@ export async function setup({ js = true, skipInstall = false }) { | |||
| CLERK_SECRET_KEY: instance.secretKey, | |||
| ...(js ? { NEXT_PUBLIC_CLERK_JS_URL: 'http://localhost:4000/npm/clerk.browser.js' } : {}), | |||
| }); | |||
|
|
|||
| if (pkgJSON.scripts?.dev && pkgJSON.scripts.dev.includes('--turbo')) { | |||
There was a problem hiding this comment.
Fix optional chaining as suggested by static analysis.
The static analysis tool correctly identified that pkgJSON.scripts.dev should use optional chaining to prevent potential runtime errors when scripts is undefined.
- if (pkgJSON.scripts?.dev && pkgJSON.scripts.dev.includes('--turbo')) {
+ if (pkgJSON.scripts?.dev?.includes('--turbo')) {📝 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.
| if (pkgJSON.scripts?.dev && pkgJSON.scripts.dev.includes('--turbo')) { | |
| if (pkgJSON.scripts?.dev?.includes('--turbo')) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 254-254: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In packages/dev-cli/src/commands/setup.js at line 254, the code accesses
pkgJSON.scripts.dev without optional chaining, which can cause runtime errors if
scripts is undefined. Update the condition to use optional chaining on scripts
as well, changing pkgJSON.scripts.dev to pkgJSON.scripts?.dev to safely handle
cases where scripts might be undefined.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
Description
This PR adds a warning when Turbopack usage has been detected during
clerk-dev setup, and logs the correct value to set foroutputFileTracingRoot.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Bug Fixes
Chores