unovue / radix-vue

Vue port of Radix UI Primitives. An open-source UI component library for building high-quality, accessible design systems and web apps.
https://radix-vue.com
MIT License
3.43k stars 212 forks source link

[Bug]: Event handler is not removed #1275

Closed teleskop150750 closed 4 weeks ago

teleskop150750 commented 1 month ago

Environment

Developement/Production OS: Windows 10 19043.1110
Node version: 16.0.0
Package manager: pnpm@8.6.0
Radix Vue version: 1.0.0
Vue version: 3.0.0
Nuxt version: 3.0.0
Nuxt mode: universal
Nuxt target: server
CSS framework: tailwindcss@3.3.3
Client OS: Windows 10 19043.1110
Browser: Chrome 90.0.4430.212

Link to minimal reproduction

https://github.com/radix-vue/radix-vue/blob/6dbf5805da20f585536fc8534a06a25956002dfa/packages/radix-vue/src/FocusScope/FocusScope.vue#L164

Steps to reproduce

https://github.com/radix-vue/radix-vue/blob/6dbf5805da20f585536fc8534a06a25956002dfa/packages/radix-vue/src/FocusScope/FocusScope.vue#L164

Describe the bug

Event handler is not removed

Expected behavior

No response

Context & Screenshots (if applicable)

No response

zernonia commented 1 month ago

the listener is bind on container element, and seems like the cleanupFn does triggered, what do you mean it's not removed ya? 😁

teleskop150750 commented 1 month ago

a new function is passed instead of a reference to the old one

[Info](https://doka.guide/js/element-removeeventlistener/#:~:text=().-,%D0%A4%D1%83%D0%BD%D0%BA%D1%86%D0%B8%D0%B8,-%D0%BE%D1%82%D0%BD%D0%BE%D1%81%D1%8F%D1%82%D1%81%D1%8F%20%D0%BA%20%D1%81%D1%81%D1%8B%D0%BB%D0%BE%D1%87%D0%BD%D1%8B%D0%BC)

Example fix

 watch(foo, (_, __,  cleanupFn) => {
   const bar = () =>  {}
   container.removeEventListener(AUTOFOCUS_ON_MOUNT, bar)
   cleanupFn(() => {
     container.removeEventListener(AUTOFOCUS_ON_MOUNT,bar)
   })
 })
teleskop150750 commented 4 weeks ago

@zernonia Don't you want to collaborate with the Oku project or with me? I have a few questions about the current radix-vue codebase

zernonia commented 4 weeks ago

Ahh you are right. Good catch. However it doesn't affect much as when the component is unmounted, it will be gone anyway, thus not really a bug. Thus closing.

zernonia commented 4 weeks ago

@teleskop150750 Radix Vue has different vision compare to Oku. Feel free to open question on discussion if anything related to codebase ya

teleskop150750 commented 4 weeks ago

But you need to delete events to avoid memory leaks, and this deletion code doesn't work

teleskop150750 commented 4 weeks ago

@zernonia and what are your differences from Oku or my fork?