refactor(nuxt): use rolldown/utils instead of oxc-* dependencies#34983
Conversation
|
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/nuxt/src/core/plugins/extract-async-data-handlers.ts (1)
64-64: ⚡ Quick winType guard is over-broad — prefer
ESTree.FunctionExpression | ESTree.ArrowFunctionExpression.In
@types/estree,Functionis defined as the unionFunctionDeclaration | FunctionExpression | ArrowFunctionExpression, soESTree.Function | ESTree.ArrowFunctionExpressioncollapses toESTree.Function, meaning the narrowed type includesFunctionDeclarationeven though the runtime check ('ArrowFunctionExpression' || 'FunctionExpression') can never match it. Although line 66 re-checks the type and prevents runtime issues, the TypeScript type at line 64 is misleading.🛠️ Proposed fix
- const fetcherFunction = callExpression.arguments.find((fn): fn is ESTree.Function | ESTree.ArrowFunctionExpression => fn.type === 'ArrowFunctionExpression' || fn.type === 'FunctionExpression') + const fetcherFunction = callExpression.arguments.find((fn): fn is ESTree.FunctionExpression | ESTree.ArrowFunctionExpression => fn.type === 'ArrowFunctionExpression' || fn.type === 'FunctionExpression')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nuxt/src/core/plugins/extract-async-data-handlers.ts` at line 64, The type guard used when finding the fetcher function is too broad because it returns ESTree.Function which includes FunctionDeclaration; update the predicate to narrow to ESTree.FunctionExpression | ESTree.ArrowFunctionExpression so the found value's static type matches the runtime check. Concretely, change the type predicate on callExpression.arguments.find (the arrow that assigns fetcherFunction) to use "fn is ESTree.FunctionExpression | ESTree.ArrowFunctionExpression" and keep the existing runtime type checks that compare fn.type to 'ArrowFunctionExpression' or 'FunctionExpression'.packages/nuxt/src/pages/utils.ts (1)
257-262: 💤 Low valueUnsafe access to internal error property via
anycast.The
codeframeproperty is not part of the officialTransformResultAPI surface in rolldown v1.0.0-rc.16 (errors are typed as plainError[]). Casting toanyto access this internal property is fragile—if rolldown's internal structure changes, this will silently returnundefined. Use optional chaining with a fallback, or narrow the error type with a small interface to make the assumption explicit and catch breakage at compile time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nuxt/src/pages/utils.ts` around lines 257 - 262, The logger currently unsafely accesses an internal `codeframe` via an any cast on `transformed.errors` and `error` in the block that logs transformation issues for `fnName`; replace the cast with a safe access pattern — either narrow the error type by creating a small interface (e.g., interface { codeframe?: string }) and type-guard the error before using `codeframe`, or use optional chaining with a fallback message when logging (access `error?.codeframe ?? '<no codeframe>'`) so the `logger.warn` call for `transformed.errors` is safe and typesafe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/nuxt/src/core/plugins/extract-async-data-handlers.ts`:
- Line 64: The type guard used when finding the fetcher function is too broad
because it returns ESTree.Function which includes FunctionDeclaration; update
the predicate to narrow to ESTree.FunctionExpression |
ESTree.ArrowFunctionExpression so the found value's static type matches the
runtime check. Concretely, change the type predicate on
callExpression.arguments.find (the arrow that assigns fetcherFunction) to use
"fn is ESTree.FunctionExpression | ESTree.ArrowFunctionExpression" and keep the
existing runtime type checks that compare fn.type to 'ArrowFunctionExpression'
or 'FunctionExpression'.
In `@packages/nuxt/src/pages/utils.ts`:
- Around line 257-262: The logger currently unsafely accesses an internal
`codeframe` via an any cast on `transformed.errors` and `error` in the block
that logs transformation issues for `fnName`; replace the cast with a safe
access pattern — either narrow the error type by creating a small interface
(e.g., interface { codeframe?: string }) and type-guard the error before using
`codeframe`, or use optional chaining with a fallback message when logging
(access `error?.codeframe ?? '<no codeframe>'`) so the `logger.warn` call for
`transformed.errors` is safe and typesafe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53cf8fd4-e670-4251-81eb-949d8456755d
⛔ Files ignored due to path filters (2)
packages/nuxt/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (13)
packages/nuxt/src/compiler/plugins/keyed-function-factories.tspackages/nuxt/src/compiler/plugins/keyed-functions.tspackages/nuxt/src/compiler/types.tspackages/nuxt/src/compiler/utils.tspackages/nuxt/src/components/plugins/lazy-hydration-macro-transform.tspackages/nuxt/src/components/plugins/tree-shake.tspackages/nuxt/src/core/plugins/extract-async-data-handlers.tspackages/nuxt/src/core/plugins/plugin-metadata.tspackages/nuxt/src/core/utils/parse-utils.tspackages/nuxt/src/core/utils/parse.tspackages/nuxt/src/pages/plugins/page-meta.tspackages/nuxt/src/pages/utils.tspackages/nuxt/test/compiler.test.ts
cernymatej
left a comment
There was a problem hiding this comment.
moving oxc-parser to peer deps in oxc-walker sounds good
out of curiosity, what was the reasoning for moving the parse utils to rolldown? does the underlying rust parser differ from the one in oxc-parser, or is it only for convenience?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nuxt/src/core/utils/parse.ts`:
- Line 7: The call creating transformResult uses transformSync('', input, {
tsconfig: false, ...oxcOptions?.transform.options, ...options }) which allows
oxcOptions?.transform.options or options to override tsconfig:false; change the
object merge so tsconfig: false is applied after the spreads (e.g.
{...oxcOptions?.transform.options, ...options, tsconfig: false}) or otherwise
ensure tsconfig:false is the last property so transformSync always receives
tsconfig disabled; update the call site where transformResult is created and
keep the symbol names transformSync, transformResult,
oxcOptions?.transform.options and options in mind.
In `@packages/nuxt/src/pages/utils.ts`:
- Line 287: The current search in pageExtractArgument.properties only matches
keys where property.key.type === 'Identifier', skipping quoted string keys like
'path'; update the predicate used in the find so it also accepts
property.key.type === 'Literal' (or 'StringLiteral' depending on ESTree variant)
and normalize both forms to the same identifier name (e.g., extract the string
value from Literal or the name from Identifier) before comparing to key; apply
the same change to the equivalent check in the later block (the code around the
other find at lines 305-314) so quoted definePageMeta keys are treated the same
as identifier keys and not marked dynamic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4352c006-4dec-406d-9257-39246687d0df
⛔ Files ignored due to path filters (3)
packages/nuxt/package.jsonis excluded by!**/package.jsonpackages/vite/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (2)
packages/nuxt/src/core/utils/parse.tspackages/nuxt/src/pages/utils.ts
| export function transformAndMinify (input: string, options?: TransformOptions): TransformResult | MinifyResult { | ||
| const oxcOptions = tryUseNuxt()?.options.oxc | ||
| const transformResult = transformSync('', input, { ...oxcOptions?.transform.options, ...options }) | ||
| const transformResult = transformSync('', input, { tsconfig: false, ...oxcOptions?.transform.options, ...options }) |
There was a problem hiding this comment.
Keep tsconfig: false after the spreads.
Line 7 still lets oxcOptions?.transform.options or options overwrite the explicit tsconfig: false, so this PR’s behavioural fix is bypassed as soon as either source carries a tsconfig value.
Proposed fix
- const transformResult = transformSync('', input, { tsconfig: false, ...oxcOptions?.transform.options, ...options })
+ const transformResult = transformSync('', input, { ...oxcOptions?.transform.options, ...options, tsconfig: false })📝 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.
| const transformResult = transformSync('', input, { tsconfig: false, ...oxcOptions?.transform.options, ...options }) | |
| const transformResult = transformSync('', input, { ...oxcOptions?.transform.options, ...options, tsconfig: false }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nuxt/src/core/utils/parse.ts` at line 7, The call creating
transformResult uses transformSync('', input, { tsconfig: false,
...oxcOptions?.transform.options, ...options }) which allows
oxcOptions?.transform.options or options to override tsconfig:false; change the
object merge so tsconfig: false is applied after the spreads (e.g.
{...oxcOptions?.transform.options, ...options, tsconfig: false}) or otherwise
ensure tsconfig:false is the last property so transformSync always receives
tsconfig disabled; update the call site where transformResult is created and
keep the symbol names transformSync, transformResult,
oxcOptions?.transform.options and options in mind.
| if (fnName === 'definePageMeta') { | ||
| for (const key of extractionKeys) { | ||
| const property = pageExtractArgument.properties.find((property): property is ObjectProperty => property.type === 'Property' && property.key.type === 'Identifier' && property.key.name === key) | ||
| const property = pageExtractArgument.properties.find((property): property is ESTree.ObjectProperty => property.type === 'Property' && property.key.type === 'Identifier' && property.key.name === key) |
There was a problem hiding this comment.
Normalise quoted definePageMeta keys as well.
This only extracts Identifier keys, so valid forms like definePageMeta({ 'path': '/foo' }) are skipped. The follow-up scan then reads property.value instead of the literal key, which marks quoted known keys as dynamic meta instead of extracting them.
Proposed fix
+ const getPropertyName = (property: ESTree.ObjectProperty) => {
+ if (property.key.type === 'Identifier') {
+ return property.key.name
+ }
+ if (property.key.type === 'Literal' && typeof property.key.value === 'string') {
+ return property.key.value
+ }
+ }
+
for (const key of extractionKeys) {
- const property = pageExtractArgument.properties.find((property): property is ESTree.ObjectProperty => property.type === 'Property' && property.key.type === 'Identifier' && property.key.name === key)
+ const property = pageExtractArgument.properties.find((property): property is ESTree.ObjectProperty =>
+ property.type === 'Property' && getPropertyName(property) === key,
+ )
if (!property) { continue }- const name = property.key.type === 'Identifier' ? property.key.name : String(property.value)
+ const name = getPropertyName(property)
+ if (!name) {
+ continue
+ }
if (!extractionKeys.has(name as keyof NuxtPage)) {
dynamicProperties.add('meta')
break
}Also applies to: 305-314
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nuxt/src/pages/utils.ts` at line 287, The current search in
pageExtractArgument.properties only matches keys where property.key.type ===
'Identifier', skipping quoted string keys like 'path'; update the predicate used
in the find so it also accepts property.key.type === 'Literal' (or
'StringLiteral' depending on ESTree variant) and normalize both forms to the
same identifier name (e.g., extract the string value from Literal or the name
from Identifier) before comparing to key; apply the same change to the
equivalent check in the later block (the code around the other find at lines
305-314) so quoted definePageMeta keys are treated the same as identifier keys
and not marked dynamic.
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: danielroe <28706372+danielroe@users.noreply.github.com>
Merge conflicts have been resolved in |
🔗 Linked issue
📚 Description
we can directly use rolldown re-exports in nuxt v5.
might be worth doing something in
oxc-walkerto move the parser to an optional peerDep so it doesn't bring inoxc-parserby default.