Skip to content

fix(reactivity): correct dirty assign in render function#10091

Merged
yyx990803 merged 5 commits into
vuejs:mainfrom
Doctor-wu:fix-recurse-computed-trigger-effect
Jan 12, 2024
Merged

fix(reactivity): correct dirty assign in render function#10091
yyx990803 merged 5 commits into
vuejs:mainfrom
Doctor-wu:fix-recurse-computed-trigger-effect

Conversation

@Doctor-wu

@Doctor-wu Doctor-wu commented Jan 12, 2024

Copy link
Copy Markdown
Member

close #10082

@Doctor-wu

Doctor-wu commented Jan 12, 2024

Copy link
Copy Markdown
Member Author
CleanShot 2024-01-12 at 15 20 14@2x

Will add some description and test suite later

@github-actions

github-actions Bot commented Jan 12, 2024

Copy link
Copy Markdown

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 Doctor-wu mentioned this pull request Jan 12, 2024
Closed
@Doctor-wu

Doctor-wu commented Jan 12, 2024

Copy link
Copy Markdown
Member Author
CleanShot 2024-01-12 at 20 15 42@2x CleanShot 2024-01-12 at 20 14 28@2x

@Doctor-wu

Doctor-wu commented Jan 12, 2024

Copy link
Copy Markdown
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

edison1105 commented Jan 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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

@Doctor-wu

Doctor-wu commented Jan 12, 2024

Copy link
Copy Markdown
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

Doctor-wu commented Jan 12, 2024

Copy link
Copy Markdown
Member Author

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

@yyx990803

yyx990803 commented Jan 12, 2024

Copy link
Copy Markdown
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

yyx990803 commented Jan 12, 2024

Copy link
Copy Markdown
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
@Doctor-wu

Doctor-wu commented Jan 12, 2024

Copy link
Copy Markdown
Member Author

@yyx990803 glad to help~

@LinusBorg

LinusBorg commented Jan 12, 2024

Copy link
Copy Markdown
Member

/ecosystem-ci run

@vue-bot

vue-bot commented Jan 12, 2024

Copy link
Copy Markdown
Contributor

📝 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

@johnsoncodehk johnsoncodehk mentioned this pull request Jan 13, 2024
Merged
yyx990803 added a commit that referenced this pull request Jan 13, 2024
@yyx990803
revert: "fix(reactivity): correct dirty assign in render function (#1…
1fb17ac 
…0091)"

This reverts commit 8d04205.
yyx990803 added a commit that referenced this pull request Jan 13, 2024
@yyx990803
revert: fix(reactivity): correct dirty assign in render function (#10091
8b18481 
)

This reverts commit 8d04205.

close #10098
close #10100
@yyx990803

yyx990803 commented Jan 13, 2024

Copy link
Copy Markdown
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

Doctor-wu commented Jan 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@Doctor-wu @edison1105 @yyx990803 @LinusBorg @vue-bot