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(reactivity): correct dirty assign in render function #10091

Merged
merged 5 commits into from
Jan 12, 2024

Conversation

Doctor-wu
Copy link
Member

close #10082

@Doctor-wu
Copy link
Member Author

CleanShot 2024-01-12 at 15 20 14@2x

Will add some description and test suite later

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 89.7 kB (+16 B) 34.2 kB (+3 B) 30.8 kB (+8 B)
vue.global.prod.js 147 kB (+16 B) 53.4 kB (+3 B) 47.7 kB (-16 B)

Usages

Name Size Gzip Brotli
createApp 50.1 kB (+16 B) 19.6 kB (+2 B) 17.9 kB (-57 B)
createSSRApp 53.5 kB (+16 B) 20.9 kB (+2 B) 19.1 kB (-12 B)
defineCustomElement 52.4 kB (+16 B) 20.4 kB (+2 B) 18.6 kB (+8 B)
overall 63.8 kB (+16 B) 24.7 kB (+1 B) 22.4 kB (+11 B)

@Doctor-wu Doctor-wu changed the title fix(reactivity): correct dirty assign in recursive render fix(reactivity): correct dirty assign in allowRecurse Jan 12, 2024
@Doctor-wu
Copy link
Member Author

CleanShot 2024-01-12 at 20 15 42@2x CleanShot 2024-01-12 at 20 14 28@2x

@Doctor-wu
Copy link
Member Author

@johnsoncodehk Could you help to review this? First time to read the new reactivity system, not sure if it's totally ok, but it works to solve the issue and pass all tests

@Doctor-wu Doctor-wu changed the title fix(reactivity): correct dirty assign in allowRecurse fix(reactivity): correct dirty assign in render function Jan 12, 2024
@edison1105
Copy link
Member

edison1105 commented Jan 12, 2024

this doesn't seem to fix that, this demo should work(works in 3.3).

@Doctor-wu
Copy link
Member Author

this doesn't seem to fix that, this demo should work(works in 3.3).

This demo seems to be another issue different from this one, maybe we should commit another issue?

@Doctor-wu
Copy link
Member Author

Whatever, I will take a look, mention me if you commit another issue~

@yyx990803
Copy link
Member

I think this is the right fix for #10082. The other case from #10090 is not the same underlying issue (mutating state in another computed)

@yyx990803
Copy link
Member

Btw good job @Doctor-wu ! I was looking at this as well before dinner and also located the issue to be in triggerEffects. Came back and found that you have fixed it :)

@yyx990803 yyx990803 merged commit 8d04205 into vuejs:main Jan 12, 2024
11 checks passed
@Doctor-wu
Copy link
Member Author

@yyx990803 glad to help~

@LinusBorg
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Jan 12, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools failure failure
nuxt failure success
pinia success success
quasar failure success
radix-vue success undefined undefined
router success success
test-utils success success
vant failure success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success failure
vuetify success success
vueuse success success
vue-simple-compiler success success

@yyx990803
Copy link
Member

Looks like this caused pretty serious regressions in Nuxt and Quasar, unfortunately the tests in core didn't really capture those complicated cases.

@Doctor-wu
Copy link
Member Author

Doctor-wu commented Jan 13, 2024

It's a pity that it didn't really solve the problem. It is challenging to include numerous complex cases in the core tests. I believe it would be reasonable to allow the ecosystem to develop more complex cases. Perhaps we should run the ecosystem CI before merging

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.

Providing a computed prop to child breaks the reactivity of the computed
5 participants