perf(Button): split link and plain button behavior#6273
perf(Button): split link and plain button behavior#6273J-Michalek wants to merge 1 commit intonuxt:v4from
Conversation
commit: |
📝 WalkthroughWalkthroughThis change optimizes the Button component's rendering behavior and introduces performance benchmarking. The Button component now explicitly disables automatic attribute inheritance via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes RationaleThe Button.vue changes involve conditional rendering logic with two distinct code paths (link vs. non-link), explicit attribute forwarding with special handling for certain props, and refactored binding patterns. While individual changes follow clear patterns, the interaction between attribute inheritance control, computed properties, and conditional rendering requires separate reasoning for each branch. The benchmark file is straightforward and repetitive in structure. Combined scope and logic density warrant moderate review effort. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/Button.vue`:
- Around line 123-130: The link branch rendering of ULink is missing the
required data-slot attribute; update the ULink root element (the template branch
where isLink is true and ULink is used) to include data-slot="name" (matching
the naming used elsewhere) alongside the existing props (v-slot, v-bind, :type,
:disabled, custom) so the link branch has the same slot identification attribute
as the non-link root; ensure the attribute is added on the ULink tag (the
component used with v-slot="{ active, ...slotProps }") and preserved when
merging $attrs/omit(linkProps).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f9201fd-994d-4e85-a357-153f6945ec8d
📒 Files selected for processing (2)
src/runtime/components/Button.vuetest/components/Button.bench.ts
| <ULink | ||
| v-if="isLink" | ||
| v-slot="{ active, ...slotProps }" | ||
| v-bind="{ ...$attrs, ...omit(linkProps, ['type', 'disabled', 'onClick']) }" | ||
| :type="type" | ||
| :disabled="disabled || isLoading" | ||
| v-bind="omit(linkProps, ['type', 'disabled', 'onClick'])" | ||
| custom | ||
| > |
There was a problem hiding this comment.
Add data-slot on the link-branch root element.
Line 123 renders ULink without a data-slot attribute, while the non-link root already has one.
Proposed fix
<ULink
+ data-slot="link"
v-if="isLink"As per coding guidelines: "Add data-slot="name" attributes on all template elements for slot identification".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/components/Button.vue` around lines 123 - 130, The link branch
rendering of ULink is missing the required data-slot attribute; update the ULink
root element (the template branch where isLink is true and ULink is used) to
include data-slot="name" (matching the naming used elsewhere) alongside the
existing props (v-slot, v-bind, :type, :disabled, custom) so the link branch has
the same slot identification attribute as the non-link root; ensure the
attribute is added on the ULink tag (the component used with v-slot="{ active,
...slotProps }") and preserved when merging $attrs/omit(linkProps).
|
@J-Michalek This doesn't solve the core problem when rendering buttons with a link 😬 |
|
@benjamincanac Yes, I know, but I've no idea how to improve that at the moment. If I remember correctly NuxtLink itself was pretty heavy. |
|
@benjamincanac I did some more benchmarking and I think this PR or changes related to this PR should be applied: #6310 doesn't seem to provide much help atleast on initial render, I think it helps with the dep tracking issues. |
|
@J-Michalek What do you mean? It does exactly the same thing at the Link level 🤔 https://github.com/nuxt/ui/pull/6310/changes#diff-c3072f78bc4ba4ddd2eaf378a34f73548386783dfdde8fa0dce9585c27e61fe6R207 |
2 participants
🔗 Linked issue
Resolves #4154
❓ Type of change
📚 Description
The link functionality gets pretty heavy when there are many buttons on the same page for example in a list of items. This does not help the case where there are many links on the page, but atleast the users that want basic buttons don't have the performance overload of the link.
Some meassurements before and after:
📝 Checklist