Skip to content
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

fix(runtime-core): handle nested reactive object in guardReactiveProps #11693

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

linzhe141
Copy link
Contributor

@linzhe141 linzhe141 commented Aug 23, 2024

Copy link

github-actions bot commented Aug 23, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 99.1 kB (+74 B) 37.5 kB (+25 B) 33.8 kB (-18 B)
vue.global.prod.js 157 kB (+74 B) 57.3 kB (+28 B) 51 kB (+9 B)

Usages

Name Size Gzip Brotli
createApp 54.6 kB (+74 B) 21.1 kB (+20 B) 19.3 kB (+13 B)
createSSRApp 58.5 kB (+74 B) 22.8 kB (+19 B) 20.8 kB (+12 B)
defineCustomElement 59.2 kB (+74 B) 22.6 kB (+22 B) 20.6 kB (+13 B)
overall 68.2 kB (+74 B) 26.2 kB (+23 B) 23.8 kB (-22 B)

Copy link

pkg-pr-new bot commented Aug 23, 2024

commit: e320f30

@vue/compiler-core

pnpm add https://pkg.pr.new/@vue/compiler-core@11693

@vue/compiler-dom

pnpm add https://pkg.pr.new/@vue/compiler-dom@11693

@vue/compiler-sfc

pnpm add https://pkg.pr.new/@vue/compiler-sfc@11693

@vue/compiler-ssr

pnpm add https://pkg.pr.new/@vue/compiler-ssr@11693

@vue/reactivity

pnpm add https://pkg.pr.new/@vue/reactivity@11693

@vue/runtime-core

pnpm add https://pkg.pr.new/@vue/runtime-core@11693

@vue/runtime-dom

pnpm add https://pkg.pr.new/@vue/runtime-dom@11693

@vue/server-renderer

pnpm add https://pkg.pr.new/@vue/server-renderer@11693

@vue/shared

pnpm add https://pkg.pr.new/@vue/shared@11693

vue

pnpm add https://pkg.pr.new/vue@11693

@vue/compat

pnpm add https://pkg.pr.new/@vue/compat@11693

Open in Stackblitz

setup() {
const { obj } = toRefs(state)
setTimeout(async () => {
state.obj.style.color = 'yellow'
Copy link
Member

Choose a reason for hiding this comment

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

Why not move those 3 lines to the end of this case?

@edison1105 edison1105 added the 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. label Aug 23, 2024
@edison1105 edison1105 changed the title fix: fix guardReactiveProps in v-bind case fix(runtime-core): handle nested reactive object in guardReactiveProps Aug 23, 2024
function generateProps(props: Data & VNodeProps) {
const target: Data & VNodeProps = {}
for (const key in props) {
if (isProxy(props[key])) {
Copy link
Member

@edison1105 edison1105 Aug 23, 2024

Choose a reason for hiding this comment

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

should check props[key] is Object first.
consider the following situation:

const state = reactive({
      title: 'hi',
      obj: {
        style: {
          color: 'red',
        },
      },
    })

In theory, only 2 layers of nesting need to be handled here, and for safety, maybe it is necessary to deal with deep recursive processing.

@edison1105 edison1105 added the ready for review This PR requires more reviews label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. ready for review This PR requires more reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

when plain element with v-bind="obj" has style property, style property mutation is not work
3 participants