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.

Sign up for GitHub

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

Jump to bottom

fix(vModel): skip value update in mounted hook if value was previously updated #12482

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

fix(vModel): skip value update in mounted hook if value was previously updated #12482

wants to merge 2 commits into from

Conversation

edison1105
Copy link
Member

@edison1105 edison1105 commented Nov 28, 2024 •
edited
Loading

close #12460

The root cause is that the component's mounted hook is executed asynchronously, while the beforeUpdate hook is executed synchronously. If an update occurs during the component's mounting, the beforeUpdate hook will execute before mounted. This also causes vModelText's beforeUpdate to execute before mounted, resulting in the value set in the beforeUpdate hook being updated again in the mounted hook.

  • playground 1 onBeforeUpdate of the component is executed before onMounted
  • playground 2 beforeUpdate of the directive is executed before mounted

I'm not sure if this is the correct solution, but the impact should be minimal. Perhaps the correct approach is to ensure that the mounted hook always executes before the beforeUpdate hook.

@github-actions GitHub Actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 100 kB (+123 B) 38 kB (+34 B) 34.3 kB (+62 B)
vue.global.prod.js 158 kB (+123 B) 57.8 kB (+28 B) 51.4 kB (+4 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 47.2 kB 18.3 kB 16.8 kB
createApp 55.2 kB 21.3 kB 19.5 kB
createSSRApp 59.3 kB 23.1 kB 21 kB
defineCustomElement 60.1 kB 22.9 kB 20.8 kB
overall 69.1 kB 26.5 kB 24.1 kB

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Nov 28, 2024 •
edited
Loading

Open in Stackblitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@12482

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@12482

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@12482

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@12482

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@12482

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@12482

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@12482

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@12482

@vue/shared

npm i https://pkg.pr.new/@vue/shared@12482

vue

npm i https://pkg.pr.new/vue@12482

@vue/compat

npm i https://pkg.pr.new/@vue/compat@12482

commit: 90daa5c

@edison1105
Copy link
Member Author

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Nov 28, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools ✅ success ✅ success
nuxt ✅ success ✅ success
pinia ✅ success ✅ success
primevue ✅ success ✅ success
quasar ✅ success ✅ success
radix-vue ✅ success ✅ success
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 ✅ success
vuetify ✅ success ✅ success
vueuse ✅ success ❌ failure
vue-simple-compiler ✅ success ✅ success

@edison1105 edison1105 marked this pull request as draft November 28, 2024 08:10
@edison1105 edison1105 marked this pull request as ready for review November 28, 2024 08:49
@edison1105 edison1105 added scope: v-model 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. ready for review This PR requires more reviews labels Nov 28, 2024
@edison1105 edison1105 changed the title fix(vModel): skip value update in mounted hook if value was previously set fix(vModel): skip value update in mounted hook if value was previously updated Nov 28, 2024
@jods4
Copy link
Contributor

jods4 commented Nov 28, 2024

To be honest I don't know what the best resolution is.

This feels a bit like a hack/work-around: vModelText is modified so that out-of-order hooks do not impact the <input> value, but the root cause of problem is still there and may affect other directives or components.

I think the intuition / expectation of Vue developers are:

  1. mounted is called after component is inserted in DOM.
  2. beforeUpdate is never called before mounted.

Based on this, I'd say that if an update happens while the component is being mounted, then maybe one solution may be this: no beforeUpdate or updated hooks are called; and when mounted is invoked it contains the current values, not a snapshot of values when it was enqueued?

@edison1105
Copy link
Member Author

then maybe one solution may be this: no beforeUpdate or updated hooks are called; and when mounted is invoked it contains the current values, not a snapshot of values when it was enqueued?

If beforeUpdate or updated is not called, it might cause some edge cases, I think it should put the beforeUpdate and updated hooks into pendingPostFlushCbs to ensure they are called.

@jods4
Copy link
Contributor

jods4 commented Nov 29, 2024

@edison1105 Maybe that would be a solution, too.
It's an important change because what was a sync hook would become async/delayed but I can't tell if this is an issue or not.
To mitigate this, one approach could be to queue beforeUpdate and updated only when mounted hasn't been called yet?

@edison1105
Copy link
Member Author

@jods4

one approach could be to queue beforeUpdate and updated only when mounted hasn't been called yet?

Yes, that's what I meant above.

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
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. ready for review This PR requires more reviews scope: v-model
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Directives mounted called *after* beforeUpdate
3 participants
@edison1105 @vue-bot @jods4