fix(nuxt): ensure semver.satisfies returns true for pre-release versions
#32574
Conversation
|
|
|
less than a minute to approve the changes? get some rest daniel! lol |
WalkthroughThe change updates the version compatibility check in the Nuxt initialisation process by importing the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (4)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/core/nuxt.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.{ts,tsx}`: Follow standard TypeScript conventions and best practices
**/*.{ts,tsx}: Follow standard TypeScript conventions and best practices
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
packages/nuxt/src/core/nuxt.ts
`**/*.{ts,tsx,vue}`: Use clear, descriptive variable and function names Add comm...
**/*.{ts,tsx,vue}: Use clear, descriptive variable and function names
Add comments only to explain complex logic or non-obvious implementations
Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Remove code that is not used or needed
Use error handling patterns consistently
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
packages/nuxt/src/core/nuxt.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: Tofandel
PR: nuxt/nuxt#0
File: :0-0
Timestamp: 2024-11-11T12:34:22.648Z
Learning: Ensure that AI-generated summaries accurately reflect the key changes in the PR, focusing on notable changes such as the removal of unused imports and variables starting with underscores.
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
packages/nuxt/src/core/nuxt.ts (3)
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
🧬 Code Graph Analysis (1)
packages/nuxt/src/core/nuxt.ts (1)
packages/nuxt/src/app/nuxt.ts (1)
nuxt(267-267)
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: codeql (actions)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: code
🔇 Additional comments (1)
packages/nuxt/src/core/nuxt.ts (1)
22-22: Import addition looks good.The addition of
coerceto the semver import is clean and follows the existing import pattern.
packages/nuxt/src/core/nuxt.ts
Outdated
| @@ -746,7 +746,7 @@ | |||
|
|
|||
| // Show compatibility version banner when Nuxt is running with a compatibility version | |||
| // that is different from the current major version | |||
| if (!(satisfies(nuxt._version, nuxt.options.future.compatibilityVersion + '.x'))) { | |||
| if (!(satisfies(coerce(nuxt._version), nuxt.options.future.compatibilityVersion + '.x'))) { | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Version compatibility fix is correct but consider edge cases.
The use of coerce to handle pre-release versions is the right approach. This ensures that versions like 4.0.0-rc.0 are converted to 4.0.0 before the compatibility check, which resolves the issue described in the PR.
However, consider that coerce returns null for invalid version strings, which could cause issues with satisfies.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any tests that validate the version compatibility logic with pre-release versions
# Test: Search for tests related to version compatibility and semver handling
rg -A 10 -B 5 "satisfies.*compatibilityVersion|compatibilityVersion.*satisfies" --type ts
rg -A 10 -B 5 "coerce.*nuxt.*version|nuxt.*version.*coerce" --type tsLength of output: 2419
Ensure coerce null-safety and add tests
The change to use coerce for pre-release versions is correct, but since coerce() can return null on invalid strings, we need to:
• Guard against a null result before calling satisfies
– e.g.
ts const version = coerce(nuxt._version) if (!version || !satisfies(version, `${nuxt.options.future.compatibilityVersion}.x`)) { // handle incompatible or malformed version }
• Add unit tests covering:
– Pre-release versions (4.0.0-rc.0 → 4.0.0)
– Completely invalid version strings
– Boundary cases around the .x range
This will ensure we don’t inadvertently throw or mis-evaluate when coerce returns null.
🤖 Prompt for AI Agents
In packages/nuxt/src/core/nuxt.ts at line 749, the call to coerce(nuxt._version)
can return null, which is not currently checked before passing to satisfies,
risking runtime errors. Refactor the code to first assign the result of
coerce(nuxt._version) to a variable, then check if this variable is null or if
satisfies returns false, and handle the incompatible or malformed version case
accordingly. Additionally, add unit tests to cover pre-release versions, invalid
version strings, and boundary cases around the .x range to ensure robust
handling of version compatibility.
There was a problem hiding this comment.
I can add this if necessary but _version seems to be a hardcoded string and not an optional property of nuxt so maybe it's not necessary?
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the actual version format in the package.json and test the behavior of coerce() with typical version strings.
@hacknug You're absolutely right! Looking at the code, nuxt._version is indeed a hardcoded string imported directly from '../../package.json' (line 51) and assigned at line 68. Since package.json versions are controlled by the Nuxt team and follow semantic versioning standards, coerce() should never return null for this specific use case.
The null safety suggestion was overly cautious for this context. Package.json versions are:
- Controlled by the project maintainers
- Follow semantic versioning standards
- Validated by npm tooling during publishing
Your change using coerce(nuxt._version) directly is perfectly safe and appropriate for handling pre-release versions like 4.0.0-rc.0 → 4.0.0.
The null check would only be necessary if the version string came from user input or external sources, but since it's from your own package.json, it's unnecessary complexity.
🏁 Script executed:
#!/bin/bash
# Check the actual version in package.json
echo "=== Package.json version ==="
cat packages/nuxt/package.json | jq -r '.version'
echo -e "\n=== Test coerce behavior with Node.js ==="
node -e "
const { coerce } = require('semver');
// Test typical package.json versions
const versions = [
'4.0.0',
'4.0.0-rc.0',
'4.0.0-alpha.1',
'4.0.0-beta.2',
'3.15.0',
'4.0.0-0123456789abcdef',
'', // empty string
null, // null input
undefined, // undefined input
'invalid-version',
'v4.0.0'
];
versions.forEach(v => {
try {
const result = coerce(v);
console.log(\`coerce('\${v}') = \${result ? result.version : 'null'}\`);
} catch (e) {
console.log(\`coerce('\${v}') = ERROR: \${e.message}\`);
}
});
"
Length of output: 1728
Based on the code analysis, you're absolutely correct! Here's the evidence:
- Type Definition: The
Nuxtinterface defines_version: string(not optional) - Assignment: It's directly assigned from
versionimported from../../package.json - Usage Pattern: Throughout the codebase,
nuxt._versionis used without null checks, confirming it's always expected to be a valid string
Since:
- Package.json versions are controlled by the Nuxt team
- They follow semantic versioning standards
- The version is hardcoded at build time, not runtime user input
coerce()handles valid semver strings reliably (including pre-releases)
Your assessment is spot-on - null safety is unnecessary here. The coerce(nuxt._version) call is perfectly safe as-is.
The change you made correctly addresses the original issue with pre-release versions without adding unnecessary defensive code. Adding null checks would be over-engineering for a controlled, internal version string.
🏁 Script executed:
#!/bin/bash
# Find the package.json file and check its version
echo "=== Finding package.json files ==="
find . -name "package.json" -path "*/packages/nuxt/*" | head -5
echo -e "\n=== Looking for version import ==="
rg "import.*version.*from.*package\.json" --type ts
echo -e "\n=== Check the version assignment ==="
rg "_version.*version" --type ts -A 2 -B 2
echo -e "\n=== See if coerce is used elsewhere ==="
rg "coerce.*version" --type ts -A 2 -B 2
Length of output: 2293
Perfect! This output confirms your assessment completely:
Evidence Supporting Your Point:
-
Direct Import & Assignment:
import { version } from '../../package.json'_version: version,- Clean, direct assignment
-
Single Usage Context:
coerceis used only in your modified line - nowhere else in the codebase- This is a very specific, controlled use case
-
Controlled Source:
- The version always comes from package.json (controlled by Nuxt team)
- No external input or user-provided version strings
Interesting Contrast:
The getNuxtVersion function in packages/kit/src/compatibility.ts does have defensive checks:
const rawVersion = nuxt?._version || nuxt?.version || nuxt?.constructor?.version
if (typeof rawVersion !== 'string') {
throw new TypeError('Cannot determine nuxt version! Is current instance passed?')
}But that's because it handles external/unknown Nuxt instances that might be legacy or malformed. Your use case is different - it's the internal, controlled version string.
Conclusion:
You're absolutely right - null safety would be unnecessary defensive programming here. The coerce(nuxt._version) call is perfectly safe and appropriate for handling pre-release versions without adding complexity for a guaranteed-valid input.
Your fix elegantly solves the pre-release version issue without over-engineering. ✅
🧬 Code Graph Analysis Results
Relevant snippet from packages/schema/src/types/nuxt.ts (lines 89-116)
export interface Nuxt {
// Private fields.
__name: string
_version: string
_ignore?: Ignore
_dependencies?: Set<string>
_debug?: NuxtDebugContext
/** Async local storage for current running Nuxt module instance. */
_asyncLocalStorageModule?: AsyncLocalStorage<NuxtModule>
/** The resolved Nuxt configuration. */
options: NuxtOptions
hooks: Hookable<NuxtHooks>
hook: Nuxt['hooks']['hook']
callHook: Nuxt['hooks']['callHook']
addHooks: Nuxt['hooks']['addHooks']
runWithContext: <T extends (...args: any[]) => any>(fn: T) => ReturnType<T>
ready: () => Promise<void>
close: () => Promise<void>
/** The production or development server. */
server?: any
vfs: Record<string, string>
apps: Record<string, NuxtApp>
}- This interface defines the
Nuxtobject structure including the_versionproperty as a string (not optional).
Relevant snippet from packages/nuxt/src/core/nuxt.ts (lines 40-70 approx)
export function createNuxt (options: NuxtOptions): Nuxt {
const hooks = createHooks<NuxtHooks>()
const { callHook, callHookParallel, callHookWith } = hooks
hooks.callHook = (...args) => runWithNuxtContext(nuxt, () => callHook(...args))
hooks.callHookParallel = (...args) => runWithNuxtContext(nuxt, () => callHookParallel(...args))
hooks.callHookWith = (...args) => runWithNuxtContext(nuxt, () => callHookWith(...args))
const nuxt: Nuxt = {
__name: randomUUID(),
_version: version,
_asyncLocalStorageModule: options.experimental.debugModuleMutation ? new AsyncLocalStorage() : undefined,
hooks,
callHook: hooks.callHook,
addHooks: hooks.addHooks,
hook: hooks.hook,
ready: () => runWithNuxtContext(nuxt, () => initNuxt(nuxt)),
close: () => hooks.callHook('close', nuxt),
vfs: {},
apps: {},
runWithContext: fn => runWithNuxtContext(nuxt, fn),
options,
}
// ... additional code omitted for brevity
return nuxt
}- The
_versionproperty is assigned directly from the importedversionstring frompackage.json. _versionis not optional and is always set on the creatednuxtobject.
Relevant snippet from packages/nuxt/src/core/nuxt.ts (lines 400-410 approx)
// Show compatibility version banner when Nuxt is running with a compatibility version
// that is different from the current major version
if (!(satisfies(coerce(nuxt._version), nuxt.options.future.compatibilityVersion + '.x'))) {
logger.info(`Running with compatibility version \`${nuxt.options.future.compatibilityVersion}\``)
}- Usage of
nuxt._versionas a string property without optional chaining or checks. - This confirms
_versionis expected to always be present and a string.
Summary of relevant information:
- The
Nuxtinterface defines_versionas a non-optional string property. - The
createNuxtfunction always sets_versionto the importedversionstring. - Code using
nuxt._versionassumes it is always defined and a string. - There is no indication in this file that
_versionis optional or can be undefined.
Other potentially relevant snippets (brief summaries):
-
runWithNuxtContext(frompackages/kit/src/context.tsline 55-57):export function runWithNuxtContext<T extends (...args: any[]) => any> (nuxt: Nuxt, fn: T) { return asyncNuxtStorage.call(nuxt, fn) as ReturnType<T> }
- Used to run functions within the Nuxt context, seen wrapping hook calls and other async operations in
createNuxt.
- Used to run functions within the Nuxt context, seen wrapping hook calls and other async operations in
-
loadNuxtfunction (inpackages/nuxt/src/core/nuxt.tslines ~500-600):- Loads Nuxt options and creates the Nuxt instance via
createNuxt. - Ensures
_versionis set as part of the created Nuxt instance.
- Loads Nuxt options and creates the Nuxt instance via
-
NuxtHooksinterface (frompackages/schema/src/types/hooks.ts):- Defines hooks available on the Nuxt instance, used in
createNuxt.
- Defines hooks available on the Nuxt instance, used in
[End of relevant snippets]
🧠 Learnings used
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Learnt from: Tofandel
PR: nuxt/nuxt#0
File: :0-0
Timestamp: 2024-11-11T12:34:22.648Z
Learning: Ensure that AI-generated summaries accurately reflect the key changes in the PR, focusing on notable changes such as the removal of unused imports and variables starting with underscores.
There was a problem hiding this comment.
@hacknug Thank you! I'm glad my analysis was helpful and that I could properly recognize the soundness of your technical assessment. Your approach to the pre-release version fix is clean and well-reasoned. 🐰
(\ /)
( ._.)
o_(")(")
🧠 Learnings used
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Learnt from: Tofandel
PR: nuxt/nuxt#0
File: :0-0
Timestamp: 2024-11-11T12:34:22.648Z
Learning: Ensure that AI-generated summaries accurately reflect the key changes in the PR, focusing on notable changes such as the removal of unused imports and variables starting with underscores.
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #32574 will not alter performanceComparing Summary
|
🔗 Linked issue
closes #32573
📚 Description
This PR uses
semver.coerceto ensure version numbers like4.0.0-rc.0are coerced to4.0.0to ensuresemver.satisfiesreturns true as expected.