Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork171
Fix useModal reactivity warning for custom components#2853
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
|
Co-authored-by: VividLemon <31498096+VividLemon@users.noreply.github.com>
Co-authored-by: VividLemon <31498096+VividLemon@users.noreply.github.com>
This PR is stale because it has been open for 45 days with no activity. It will not be auto-closed |
| ?obj | ||
| :shallowRef({ | ||
| ...toValue(obj), | ||
| ...(toValue(obj).component&&{component:markRaw(toValue(obj).component!)}), |
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.
@copilot I think with this being a shallow ref, we don't need to use markraw. It may be best to make this the users responsibility
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.
You're absolutely correct! I've removed the unnecessarymarkRaw() calls.shallowRef() alone is sufficient to prevent the reactive component warning since it only tracks mutations to.value without making the contents deeply reactive.
Fixed in commitadfaf50.
| // }, | ||
| customComponentTest: ()=> { | ||
| // Test that custom components no longer produce reactive warnings | ||
| const TestComponent= { |
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.
@copilot please double check to ensure that this would have caused the warning before. It is just a basic object, I'm not even sure if it's reactive.
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.
Yes, this plain object component would have caused the warning with the originaltoRef() approach. I tested this by reverting totoRef() and the test failed, confirming that even plain objects trigger the reactive warning when made reactive withtoRef(). The fix withshallowRef() resolves this for all component types, including plain objects like this one.
Per@VividLemon's review feedback, shallowRef alone is sufficient to preventthe reactive component warning. The markRaw calls in the shallowRef initializationwere redundant and can be removed, simplifying the code while maintaining the fix.Co-authored-by: VividLemon <31498096+VividLemon@users.noreply.github.com>
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Uh oh!
There was an error while loading.Please reload this page.
The
useModal().create()method was causing Vue warnings when custom components were passed becausetoRef(obj)made the entire object reactive, including component references:This would produce the warning:
Root Cause
The issue was in the
useModal/index.tsfile wheretoRef(obj)was used to make the entire parameter object reactive. This approach inadvertently made any component references within the object reactive as well, triggering Vue's performance warning.Solution
toRef()withshallowRef(): For non-ref objects, we now useshallowRef()which only tracks mutations to.valuewithout making the contents deeply reactiveshallowRef()alone is sufficient to prevent the reactive component warning - no pre-emptivemarkRaw()calls neededChanges
useModal/index.tsto useshallowRef()instead oftoRef()Technical Details
The key insight is that
shallowRef()creates a ref where only the.valuemutation is tracked, but the contents are not deeply reactive. This means components placed inside the shallowRef don't trigger Vue's reactivity warning. The existingmarkRaw()calls in the watch handler remain in place where needed, but pre-emptive marking during initialization is unnecessary.The fix is backward compatible and maintains all existing functionality while eliminating the Vue warning with a simpler, more elegant solution.
Fixes#2848.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn moreCopilot coding agent tips in the docs.