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

feat(reactivity): base watch, getCurrentWatcher, and onWatcherCleanup #9927

Merged
merged 13 commits into from
Aug 20, 2024

Conversation

LittleSound
Copy link
Member

@LittleSound LittleSound commented Dec 26, 2023

This PR achieves two objectives:

  1. It refactors the apiWatch implementation, incorporating most of its functionality into the baseWatch in reactivity.

  2. It incorporates the onWatcherCleanup, previously used in vue/core-vapor repository, directly into the reactivity package of vue/core repository.

baseWatch

The rationale behind refactoring apiWatch is that Vapor requires Watch APIs. However runtime-vapor should not import runtime-core directly. If we don't share reusable Watch API logic between runtime-core and runtime-vapor, it will result in significant code duplication. Furthermore, we plan to extract other codes that can be shared between these two runtimes into separate packages.

onWatcherCleanup

The onWatcherCleanup function has already been implemented in Vapor. Here are corresponding Issue and PR links:

Issue / PR

watchEffect(() => {
  addEventListener(el1, eventName.value, handler)
  addEventListener(el2, eventName.value, handler)
})

function addEventListener(el, event, handler) {
  el.addEventListener(event, handler)
  if (getWatcherEffect()) {
    onWatcherCleanup(() => {
      el.removeEventListener(event, handler)
    })
  }
}

Additionally, provide support for watch

watch(id, async (newId, oldId) => {
  const { response, cancel } = doAsyncWork(newId)
  // `cancel` will be called if `id` changes, cancelling
  // the previous request if it hasn't completed yet
  onWatcherCleanup(cancel)
  data.value = await response
})

@LittleSound LittleSound force-pushed the feat/onEffectCleanup-and-baseWatch branch from f3e5303 to 7c5f05a Compare December 28, 2023 09:32
@LittleSound
Copy link
Member Author

LittleSound commented Dec 28, 2023

I ran vitest bench, my modified apiWatch seems to be slower than the original apiWatch. However, I don't have experience in optimizing its performance. Are there any tips or documents that I can refer to?

Minor

image

This PR

image

@LittleSound
Copy link
Member Author

Regarding the API design of baseWatch, I haven't made too many changes other than some new callbacks. It is basically the same as doWatch.

If you have any better design suggestions, please put them forward here. I am very happy to listen your suggestions.

@LittleSound LittleSound marked this pull request as ready for review December 28, 2023 13:44
@sxzz sxzz changed the base branch from minor to main January 4, 2024 15:25
@LittleSound LittleSound force-pushed the feat/onEffectCleanup-and-baseWatch branch 2 times, most recently from e550ce9 to 770c21d Compare January 5, 2024 16:07
Copy link

github-actions bot commented Jan 8, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 99.1 kB (+452 B) 37.5 kB (+132 B) 33.8 kB (+112 B)
vue.global.prod.js 157 kB (+445 B) 57.3 kB (+155 B) 51 kB (+172 B)

Usages

Name Size Gzip Brotli
createApp 54.5 kB (+454 B) 21.1 kB (+174 B) 19.3 kB (+176 B)
createSSRApp 58.5 kB (+455 B) 22.8 kB (+220 B) 20.8 kB (+198 B)
defineCustomElement 59.1 kB (+457 B) 22.6 kB (+190 B) 20.6 kB (+213 B)
overall 68.1 kB (+459 B) 26.2 kB (+197 B) 23.8 kB (+201 B)

@LittleSound
Copy link
Member Author

LittleSound commented Jan 21, 2024

I created another separate PR #10173 for onEffectCleanup in order to reduce the reviewing time cost and get merged faster.

@LittleSound LittleSound changed the base branch from main to minor March 7, 2024 12:05
@LittleSound
Copy link
Member Author

LittleSound commented Mar 7, 2024

onEffectCleanup has been implemented in 2cc5615, and the scheduler was recently refactored in #10407. I will update this PR to support these new changes, which may take some time.

@LittleSound LittleSound changed the title feat(runtime-core, reactivity): onEffectCleanup and baseWatch feat(runtime-core, reactivity): baseWatch Mar 10, 2024
@LittleSound LittleSound force-pushed the feat/onEffectCleanup-and-baseWatch branch from 2ef9594 to 589cd11 Compare March 13, 2024 13:15
@LittleSound LittleSound changed the title feat(runtime-core, reactivity): baseWatch feat(runtime-core, reactivity): baseWatch and onWatcherCleanup Mar 13, 2024
@LittleSound
Copy link
Member Author

Due to the obvious difference between onEffectCleanup in 2cc5615 and here, after communicating with Evan, I have renamed the API in this PR to onWatcherCleanup. It will not be deleted yet.

@LittleSound LittleSound force-pushed the feat/onEffectCleanup-and-baseWatch branch from 34e750f to a5769e1 Compare March 13, 2024 14:10
Copy link
Member Author

@LittleSound LittleSound left a comment

Choose a reason for hiding this comment

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

Kevin suggested that I move the types and enums related to scheduler in baseWatch into a separate file in reactivity, hence this Commit.

@louiss0
Copy link

louiss0 commented Aug 9, 2024

Why not pass the cleanup function as the third argument?

Squashed commit of the following:

commit dad9d0f
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Thu Mar 14 20:35:19 2024 +0800

    feat: scheduler in reactivity

commit 406c750
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Thu Mar 14 14:08:12 2024 +0800

    fix: revert export alias

commit 74996b6
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Wed Mar 13 22:21:27 2024 +0800

    test: onWatcherCleanup in apiWatch

commit a5769e1
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Wed Mar 13 22:09:43 2024 +0800

    fix: remove elusive code for once

commit 589cd11
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Wed Mar 13 21:14:34 2024 +0800

    fix: errors related to immediateFirstRun

commit 3694745
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Tue Mar 12 18:16:52 2024 +0800

    refactor: rename to onWatcherCleanup, getCurrentWatcher, remove middleware

commit b3f45d2
Merge: 60a1b97 9a936aa
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Thu Mar 7 22:23:13 2024 +0800

    chore: merge branch 'minor' into feat/onEffectCleanup-and-baseWatch

commit 60a1b97
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Tue Jan 9 20:45:31 2024 +0800

    feat: middleware in baseWatch

commit 2fdda65
Merge: 39f07cd 2701355
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Mon Jan 8 17:40:54 2024 +0800

    Merge branch 'main' into feat/onEffectCleanup-and-baseWatch

commit 39f07cd
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Mon Jan 8 17:40:18 2024 +0800

    fix: should export getCurrentEffect function

commit 770c21d
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Sat Jan 6 00:07:41 2024 +0800

    fix: sync code changes according to the review in PR vuejs/vue-vapor#82

commit a6eb043
Merge: 8dd0c1f 0275dd3
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Fri Jan 5 23:43:02 2024 +0800

    chore: merge branch 'main' into feat/onEffectCleanup-and-baseWatch

commit 8dd0c1f
Merge: 2213634 274f6f7
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Sun Dec 31 20:30:29 2023 +0800

    chore: merge remote-tracking branch 'origin/minor' into feat/onEffectCleanup-and-baseWatch

commit 2213634
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Sun Dec 31 19:21:12 2023 +0800

    refactor: simplify unwatch implementation

commit f44ef0b
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Sun Dec 31 18:45:04 2023 +0800

    feat: implement getCurrentEffect

commit a078ad1
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Thu Dec 28 21:28:28 2023 +0800

    chore: rename handleWarn to onWarn

commit 90fd005
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Thu Dec 28 21:05:03 2023 +0800

    chore: organize exports

commit e9555ce
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Thu Dec 28 20:36:56 2023 +0800

    test: baseWatch

commit d99e9a6
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Thu Dec 28 20:04:42 2023 +0800

    test: onEffectCleanup in runtime-core

commit 56c87ec
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Thu Dec 28 19:44:43 2023 +0800

    test: baseWatch with onEffectCleanup

commit 7c5f05a
Merge: a8dc8e6 75dbbb8
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Thu Dec 28 17:32:00 2023 +0800

    Merge branch 'minor' of https://github.com/vuejs/core into feat/onEffectCleanup-and-baseWatch

commit a8dc8e6
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Wed Dec 27 22:43:17 2023 +0800

    fix: tracked in cleanup

commit b57405c
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Wed Dec 27 20:28:49 2023 +0800

    fix: treeshaking error

commit 4d04f5e
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Wed Dec 27 20:19:53 2023 +0800

    fix: treeshaking error

commit d1f001b
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Wed Dec 27 20:10:05 2023 +0800

    fix: lint

commit 97179ed
Merge: 2aef609 9183069
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Tue Dec 26 23:24:47 2023 +0800

    chore: merge branch 'minor' of https://github.com/vuejs/core into feat/onEffectCleanup-and-baseWatch

commit 2aef609
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Tue Dec 26 22:19:26 2023 +0800

    fix: some cases for server-renderer

commit db4463c
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Tue Dec 26 21:40:12 2023 +0800

    fix: export onEffectCleanup

commit 409b52a
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Tue Dec 26 21:31:27 2023 +0800

    refactor: the watch API with baseWatch

commit d8682e8
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Mon Dec 25 22:09:38 2023 +0800

    feat: initial code of baseWatch

commit f1fe01e
Author: Rizumu Ayaka <rizumu@ayaka.moe>
Date:   Mon Dec 25 20:50:35 2023 +0800

    refactor: externalized COMPAT case
@LittleSound LittleSound force-pushed the feat/onEffectCleanup-and-baseWatch branch from 7fbdc6e to 1730ab4 Compare August 9, 2024 14:15
@LittleSound
Copy link
Member Author

Why not pass the cleanup function as the third argument?

The third parameter of watch is still available as onCleanup. onWatcherCleanup just provides a more flexible API.

@sxzz sxzz requested a review from yyx990803 August 9, 2024 15:06
@yyx990803 yyx990803 changed the base branch from minor to main August 19, 2024 02:59
Copy link

pkg-pr-new bot commented Aug 19, 2024

commit: f0f2647

@vue/compiler-core

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

@vue/compiler-dom

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

@vue/compiler-sfc

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

@vue/compiler-ssr

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

@vue/reactivity

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

@vue/runtime-core

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

@vue/runtime-dom

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

@vue/server-renderer

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

@vue/shared

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

vue

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

@vue/compat

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

Open in Stackblitz

@LittleSound
Copy link
Member Author

Hi, Evan. Thank you for helping me improve the code

@yyx990803 yyx990803 changed the title feat(runtime-core, reactivity): baseWatch and onWatcherCleanup feat(reactivity): base watch, getCurrentWatcher, and onWatcherCleanup Aug 19, 2024
@yyx990803
Copy link
Member

Notable changes applied:

  • Rename baseWatch to watch, as "base" doesn't apply if @vue/reactivity is used directly without Vue.
  • Move scheduler related types and scheduler-specific logic back to core
  • Avoid duplicated callWithErrorHandling implementation
  • Move WatchHandle to base watch - return type of base watch is now WatchHandle instead of ReactiveEffect

@yyx990803
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Aug 19, 2024

📝 Ran ecosystem CI: Open

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

@yyx990803
Copy link
Member

/ecosystem-ci run radix-vue

@vue-bot
Copy link
Contributor

vue-bot commented Aug 20, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
radix-vue success success

@yyx990803 yyx990803 force-pushed the feat/onEffectCleanup-and-baseWatch branch from a8ea136 to f0f2647 Compare August 20, 2024 00:19
@yyx990803 yyx990803 merged commit 205e5b5 into vuejs:main Aug 20, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants