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): app.onUnmount() for registering cleanup functions (close: #4516) #4619

Merged
merged 8 commits into from
Apr 29, 2024

Conversation

LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Sep 17, 2021

This PR adds a new feature for Plugins. Library Authors can now register a callback function using app.onUnmount() which will be called immediately after the app has been unmounted with app.unmount()

Details

  • the function doesn't receive any arguments. the app instance should be available from the closure of the install function
  • if the install function returns anything but a function with zero arguments, it will be ignored.This this should ensure a reasonable guard against unintentional effects from plugins accidentally returning something before this feature was added.
// ✅ Valid
function install (app: App) {
  function cleanupSomeSideeffect() { /* ...*/ }

  app.onUnmount(cleanupSomeSideffect)
}

Open Questions (from previous iteration)

  • is the argument length guard a good idea? => no.
  • Can we accept that we might break rare edge cases where plugins currently return a function with zero arguments that is not meant to be called after unmount? => no
  • If the answer to the previous question is "no" - Should we instead provide an API on app like app.onUnmount()? => yes

close: #4516

@LinusBorg LinusBorg changed the title feat(runtime-core): Allow Plugins to return cleanup function feat(runtime-core): Allow Plugins to return cleanup function (close: #4516) Sep 17, 2021
@github-actions
Copy link

github-actions bot commented Sep 17, 2021

Size report

Path Size
vue.global.prod.js 46.19 KB (+0.08% 🔺)
runtime-dom.global.prod.js 30.55 KB (+0.11% 🔺)
index.js 18.05 KB (+0.17% 🔺)

@LinusBorg LinusBorg marked this pull request as ready for review September 17, 2021 08:51
@unbyte
Copy link
Contributor

unbyte commented Sep 19, 2021

Some thoughts:

  1. implicit checking function.length can lead to bugs that are difficult to troubleshoot (for plugin authors).
  2. Murphy's law. 🤯
  3. at present, the general practice of registering unmount hooks is to replace the original app.mount function; I think app.onUnmount is better, since it's more in line with the existing usage(, and more elegant).

@yyx990803
Copy link
Member

I also think the length check seems a bit unnecessary/incomplete.

app.onUnmount is safer and also more generic as it can be used even in non-plugin scenarios.

@LinusBorg
Copy link
Member Author

I agree with your comments and will adjust the PR accordingly

instead of return value, which was too brittle
@LinusBorg LinusBorg changed the title feat(runtime-core): Allow Plugins to return cleanup function (close: #4516) feat(runtime-core): app.onUnmount() for registering cleanup functions (close: #4516) Sep 27, 2021
packages/runtime-core/__tests__/apiCreateApp.spec.ts Outdated Show resolved Hide resolved
packages/runtime-core/src/apiCreateApp.ts Show resolved Hide resolved
packages/runtime-core/src/apiCreateApp.ts Outdated Show resolved Hide resolved
packages/runtime-core/src/apiCreateApp.ts Outdated Show resolved Hide resolved
@aantipov
Copy link

Hey 👋 any news here?

@LinusBorg LinusBorg dismissed yyx990803’s stale review October 20, 2023 15:23

dismissing review as all points have been resolved, and we need to to a new review anyway as it's been a while ^^

@github-actions
Copy link

github-actions bot commented Oct 20, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 94.1 kB (+3.34 kB) 35.5 kB (+1.04 kB) 32 kB (+916 B)
vue.global.prod.js 151 kB (+3.44 kB) 54.8 kB (+1.13 kB) 48.9 kB (+936 B)

Usages

Name Size Gzip Brotli
createApp 54 kB (+3.2 kB) 20.9 kB (+1 kB) 19 kB (+879 B)
createSSRApp 57.3 kB (+3.2 kB) 22.2 kB (+999 B) 20.2 kB (+870 B)
defineCustomElement 56.3 kB (+3.2 kB) 21.6 kB (+1.02 kB) 19.6 kB (+856 B)
overall 67.8 kB (+3.29 kB) 25.9 kB (+995 B) 23.4 kB (+852 B)

replace jest with vi
@yyx990803 yyx990803 changed the base branch from main to minor April 29, 2024 10:25
@yyx990803 yyx990803 merged commit 582a3a3 into minor Apr 29, 2024
11 checks passed
@yyx990803 yyx990803 deleted the feat-plugin-cleanup-hook-4516 branch April 29, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review This PR requires more reviews ✨ feature request New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

[plugin]How can a plugin know app mount/unmount ?
7 participants