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(runtime-core, reactivity): baseWatch and onWatcherCleanup #9927

Open
wants to merge 31 commits into
base: minor
Choose a base branch
from

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
@LittleSound LittleSound force-pushed the feat/onEffectCleanup-and-baseWatch branch from 53263a5 to e550ce9 Compare January 5, 2024 16:05
@LittleSound LittleSound force-pushed the feat/onEffectCleanup-and-baseWatch branch 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 90.7 kB (+908 B) 34.5 kB (+289 B) 31.1 kB (+265 B)
vue.global.prod.js 148 kB (+908 B) 53.7 kB (+253 B) 48 kB (+223 B)

Usages

Name Size Gzip Brotli
createApp 51.2 kB (+1.03 kB) 20 kB (+350 B) 18.3 kB (+353 B)
createSSRApp 54.5 kB (+1.03 kB) 21.3 kB (+349 B) 19.4 kB (+294 B)
defineCustomElement 53.5 kB (+1.03 kB) 20.7 kB (+348 B) 18.9 kB (+263 B)
overall 64.9 kB (+1.03 kB) 25 kB (+325 B) 22.7 kB (+317 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

2 participants