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(watch): should not fire pre watcher on child component unmount #7181

Merged
merged 8 commits into from
Dec 8, 2023

Conversation

rudyxu1102
Copy link
Contributor

@rudyxu1102 rudyxu1102 commented Nov 19, 2022

Fixes #7030

@@ -360,7 +360,10 @@ function doWatch(
} else {
// default: 'pre'
job.pre = true
if (instance) job.id = instance.uid
if (instance) {
Copy link
Member

Choose a reason for hiding this comment

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

this change is unnecessary.
we just check cb.id !== instance.uid in flushPreFlushCbs will be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated.

packages/runtime-core/src/scheduler.ts Outdated Show resolved Hide resolved
}

const Parent = {
porps: ['a'],
Copy link
Contributor

Choose a reason for hiding this comment

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

porps seems to be a typo. This line currently isn't doing anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's necessary to reproduce the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure? The spelling is wrong. It says porps, not props. There's no such option as porps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. Thanks.

Comment on lines 151 to 153
if (instance && cb.id !== instance.uid) {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this come before the checkRecursiveUpdates check? We shouldn't be incrementing the count in seen unless we're actually running the job.

Maybe something like:

if (cb && cb.pre && (!instance || cb.id === instance.id)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated.
I prefer to this condition instance && cb.id !== instance.uid that more clearer.

// #7030
it('should not fire on child component unmount w/ flush: pre', async () => {
const visible = ref(true)
const cb = jest.fn()
Copy link
Contributor

Choose a reason for hiding this comment

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

jest.fn() has been replaced with vi.fn() on the latest main branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reminding.

@dword-design
Copy link

Is there an easy way to test if this PR works?

@edison1105
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Oct 19, 2023

📝 Ran ecosystem CI: Open

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

@edison1105
Copy link
Member

Maybe ecosystem-ci should be triggered after rebase.

@github-actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB (+33 B) 32.7 kB (+26 B) 29.5 kB (+20 B)
vue.global.prod.js 132 kB (+33 B) 49.4 kB (+22 B) 44.3 kB (-43 B)

Usages

Name Size Gzip Brotli
createApp 47.9 kB (+33 B) 18.9 kB (+26 B) 17.2 kB (+20 B)
createSSRApp 50.7 kB (+33 B) 20 kB (+23 B) 18.2 kB (+10 B)
defineCustomElement 50.3 kB (+33 B) 19.7 kB (+26 B) 17.9 kB (+27 B)
overall 61.3 kB (+33 B) 23.7 kB (+27 B) 21.6 kB (+22 B)

@edison1105
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Oct 19, 2023

📝 Ran ecosystem CI: Open

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

watch runs while the component is being unmounted
6 participants