Skip to content

fix(NuxtLinkLocale): make component generic to support custom prop …#4007

Open
houtan-rocky wants to merge 2 commits into
nuxt-modules:mainfrom
houtan-rocky:houtan-rocky/fix/nuxtlinklocale-custom-prop-type
Open

fix(NuxtLinkLocale): make component generic to support custom prop …#4007
houtan-rocky wants to merge 2 commits into
nuxt-modules:mainfrom
houtan-rocky:houtan-rocky/fix/nuxtlinklocale-custom-prop-type

Conversation

@houtan-rocky

@houtan-rocky houtan-rocky commented Jun 15, 2026

Copy link
Copy Markdown

🔗 Linked issue

#3558

📚 Description

Convert component definition from non‑generic defineComponent<NuxtLinkLocaleProps<boolean>> to a generic function pattern. This allows the custom prop to accept true and correctly infers slot props, resolving the TypeScript error Type 'true' is not assignable to type 'false'.

Summary by CodeRabbit

  • Refactor
    • Strengthened the locale link component’s handling of internal vs. external destinations, with improved TypeScript typing and safer prop preparation to reduce edge-case issues.
  • Bug Fixes
    • Ensured locale-aware links render the expected localized URL and navigate correctly when using custom link props/slot content.
  • Tests
    • Added coverage verifying the localized href output and that clicking the link routes to the correct localized page.

…uxt-modules#3558)

Convert component definition from non‑generic `defineComponent<NuxtLinkLocaleProps<boolean>>` to a generic function pattern. This allows the `custom` prop to accept `true` and correctly infers slot props, resolving the TypeScript error `Type 'true' is not assignable to type 'false'`.
@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

NuxtLinkLocaleImpl in src/runtime/components/NuxtLinkLocale.ts is refactored from an object-style defineComponent with a setup function to a generic defineComponent render-function form. The Slots type is added to the vue import to type the second argument ({ slots }). All existing behaviors—dev-time conflict warning for simultaneous to/href, absolute-URL detection, locale-aware route resolution via useLocaleRoute, internal/external link determination, and NuxtLink prop construction (including deletion of href and locale fields)—are preserved inside the new render function. The closing syntax of the defineComponent call is updated to place the render function as the first argument and component options as the second. A new test case verifies that NuxtLinkLocale renders the expected localized href when using a custom prop and navigates correctly. The fixture page is extended with a NuxtLinkLocale example demonstrating usage with a scoped slot. No exported public entity signatures change.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: making the NuxtLinkLocale component generic to support the custom prop, which is the primary objective addressed in this PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/runtime/components/NuxtLinkLocale.ts (1)

46-119: ⚡ Quick win

Consider forwarding all slots instead of just the default slot.

Line 118 currently passes slots.default to the child NuxtLink component. While this works for the default slot, a more idiomatic and future-proof approach would be to pass the entire slots object:

return () => h(NuxtLink, getNuxtLinkProps(), slots)

This pattern ensures that any named slots (current or future) are properly forwarded to NuxtLink without requiring code changes.

♻️ Proposed change
-  return () => h(NuxtLink, getNuxtLinkProps(), slots.default)
+  return () => h(NuxtLink, getNuxtLinkProps(), slots)
🤖 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 `@src/runtime/components/NuxtLinkLocale.ts` around lines 46 - 119, The
NuxtLinkLocaleImpl component currently forwards only the default slot to the
NuxtLink component in its return statement. To ensure all named slots are
properly forwarded to NuxtLink, replace `slots.default` with the entire `slots`
object in the h(NuxtLink, getNuxtLinkProps(), slots.default) call. This makes
the slot forwarding more idiomatic and future-proof by automatically including
any named slots without requiring code changes.
🤖 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 `@src/runtime/components/NuxtLinkLocale.ts`:
- Around line 46-119: The NuxtLinkLocaleImpl component currently forwards only
the default slot to the NuxtLink component in its return statement. To ensure
all named slots are properly forwarded to NuxtLink, replace `slots.default` with
the entire `slots` object in the h(NuxtLink, getNuxtLinkProps(), slots.default)
call. This makes the slot forwarding more idiomatic and future-proof by
automatically including any named slots without requiring code changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 60cf4b37-a42d-4927-81b3-7e9782e56f0e

📥 Commits

Reviewing files that changed from the base of the PR and between bd54cb9 and 6fad67c.

📒 Files selected for processing (1)
  • src/runtime/components/NuxtLinkLocale.ts

…ectly

Add test case in `prefix.spec.ts` that ensures `<NuxtLinkLocale custom>`:
- Renders a custom element with the correct localized href
- Navigates to the expected route when clicked

This confirms the generic component definition works as expected and
prevents regression of nuxt-modules#3558.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
specs/basic_usage.spec.ts (1)

793-795: Replace raw click with clickNavigate for consistent navigation pattern.

Line 793 uses click() + waitForURL(), but this suite uses clickNavigate() throughout for route transitions. This pattern is more reliable for navigation timing.

Suggested patch
-      await page.locator('`#custom-link` a').click()
-      await page.waitForURL(url('/en/about'))
+      await page.locator('`#custom-link` a').clickNavigate()
       expect(page.url()).toContain('/en/about')
🤖 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 `@specs/basic_usage.spec.ts` around lines 793 - 795, The click() and
waitForURL() pattern used on the `#custom-link` a locator is inconsistent with the
clickNavigate() pattern used throughout the rest of the test suite. Replace the
separate click() and waitForURL() calls with a single clickNavigate() call to
maintain consistency with the established navigation pattern and improve
reliability for navigation timing.
🤖 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 `@specs/basic_usage.spec.ts`:
- Around line 793-795: The click() and waitForURL() pattern used on the
`#custom-link` a locator is inconsistent with the clickNavigate() pattern used
throughout the rest of the test suite. Replace the separate click() and
waitForURL() calls with a single clickNavigate() call to maintain consistency
with the established navigation pattern and improve reliability for navigation
timing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 19fc4603-ed21-4578-99e3-9984db8e5fd4

📥 Commits

Reviewing files that changed from the base of the PR and between 6fad67c and 5357b2e.

📒 Files selected for processing (2)
  • specs/basic_usage.spec.ts
  • specs/fixtures/basic/pages/index.vue
✅ Files skipped from review due to trivial changes (1)
  • specs/fixtures/basic/pages/index.vue

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant