-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(Table): replace ref with shallowRef for data binding
#6023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v4
Are you sure you want to change the base?
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
π WalkthroughWalkthroughReplaces Estimated code review effortπ― 2 (Simple) | β±οΈ ~10 minutes π₯ Pre-merge checks | β 2 | β 1β Failed checks (1 warning)
β Passed checks (2 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing touchesπ§ͺ Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
src/runtime/components/Table.vue (1)
517-519:β οΈ Potential issue | π‘ MinorWatcher correctly reassigns the array reference, which is compatible with
shallowRef.When
shallowDataistrue,datais aShallowRef. The watcher creates a new array via[...props.data], so the reference change will trigger reactivity even with shallow tracking. This is correct.However, note that the default
watchOptionsis{ deep: true }. WhenshallowDataistrue, the user likely wants to avoid deep overhead entirely. Consider documenting that users should also pass:watch-options="{ deep: false }"alongside:shallow-data="true"for maximum performance benefit, or automatically adjust the watch depth whenshallowDatais enabled.π‘ Suggestion: auto-adjust watchOptions when shallowData is true
-watch(() => props.data, () => { - data.value = props.data ? [...props.data] : [] -}, props.watchOptions) +watch(() => props.data, () => { + data.value = props.data ? [...props.data] : [] +}, props.shallowData ? { ...props.watchOptions, deep: false } : props.watchOptions)
π€ Fix all issues with AI agents
In `@src/runtime/components/Table.vue`:
- Line 259: The call to createRef is passing props.shallowData into its deep
parameter with reversed semantics; change the call that sets data (the createRef
invocation) to use the inverse boolean (i.e. deep = !props.shallowData) so
shallowData=true yields shallow reactivity: replace the current
createRef(props.data ?? [], props.shallowData) with createRef(props.data ?? [],
!props.shallowData); additionally, because createRef is evaluated once at setup,
if you need to respond to runtime changes of props.shallowData add a watcher on
props.shallowData that recreates/updates data (or switch to a different reactive
strategy) so the reactivity mode follows prop changes.
π§Ή Nitpick comments (1)
src/runtime/components/Table.vue (1)
139-144: Prop naming and documentation look reasonable.The
shallowDataprop is well-documented with a JSDoc@seelink and default value annotation.One minor consideration: users might confuse this with "partial data" or "incomplete data." A name like
shallowReactiveDataordisableDeepReactivitycould be more self-descriptive, but this is a stylistic choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
src/runtime/components/Table.vue (1)
517-519:β οΈ Potential issue | π Major
shallowDataalone doesn't eliminate deep-watch overhead β users must also setwatchOptions.deep = false.With the default
watchOptions: { deep: true }, Vue still deeply traversesprops.dataon every change even whenshallowDataistrue. The shallow ref only avoids deep tracking on reads; the watcher's deep comparison is a separate cost.Consider either:
- Auto-defaulting
watchOptions.deeptofalsewhenshallowDataistrue, or- Documenting this interaction clearly so users know to pass both props.
Option 1: Auto-adjust watch depth
-watch(() => props.data, () => { - data.value = props.data ? [...props.data] : [] -}, props.watchOptions) +watch(() => props.data, () => { + data.value = props.data ? [...props.data] : [] +}, props.shallowData ? { ...props.watchOptions, deep: false } : props.watchOptions)
π€ Fix all issues with AI agents
In `@src/runtime/components/Table.vue`:
- Line 259: The createRef call that sets const data = createRef(props.data ??
[], !props.shallowData) is evaluated only during setup so toggling
props.shallowData at runtime will not switch reactivity mode; update the
Table.vue JSDoc (near createRef/props definitions) to explicitly state that
shallowData must be set at mount time and changes after mount are ignored, and
add a short runtime guard comment next to the createRef line referencing
createRef and props.shallowData so consumers and future maintainers understand
the limitation.
π§Ή Nitpick comments (1)
src/runtime/components/Table.vue (1)
139-144: Consider renamingshallowDatato better convey its purpose.The prop name
shallowDatasuggests the data itself is shallow, but it actually controls whether the internal reactive wrapper usesshallowRefvsref. A name likeshallowReactivityordisableDeepReactivitywould be more self-describing. This is a minor naming nit β feel free to keep as-is if you prefer brevity.
6e82466 to
c04e8e3
Compare
shallowData for optimize internal data
β¦c to utilize watchOptions
shallowData for optimize internal dataref with shallowRef for data binding
commit: |
β Type of change
π Description
We have to work with large tables, where we cannot use virtualize, so we often search the page using ctrl+f. Therefore, we are looking for places where we can optimize the code.
In this case, there is no need to use
ref,shallowRefwill suffice.We testing on dev playground:
100 00050falsefalseprodπ Checklist