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.62k stars 222 forks source link

[Bug/Performance]: MenuRoot creates too many event listeners per keystroke #1176

Closed spaceemotion closed 3 months ago

spaceemotion commented 3 months ago

Environment

Development/Production OS: Windows 10 (WSL2)
Node version: 20.13.1
Package manager: pnpm@8.15.5
Radix Vue version: 1.9.2
Vue version: 3.4.35
CSS framework: tailwindcss@3.4.7
Client OS: Windows 10
Browser: Microsoft Edge Version 127.0.2651.74 (Official build) (64-bit)

Link to minimal reproduction

https://stackblitz.com/edit/hnxppu?file=src%2Fmain.ts (if profiling, check the preview frame, and then you'll find a ton of method calls happening per keydown event)

Steps to reproduce

Type in the input box and see the listener count in the dev tools profiler go up for no good reason.

Describe the bug

In my app I have a lot of potential menus that can be opened. We're talking three digits here. Radix-vue adds one-off event listeners on every keystroke which adds a ton of new event listeners and causes a lot of memory usage (that has to be GCed).

Expected behavior

Radix-Vue not creating that many event listeners. Maybe a shared event pool could help?

Context & Screenshots (if applicable)

From what I can tell the lines that create the event listeners are: https://github.com/radix-vue/radix-vue/blob/562c896ba609028a7fbd5d343aeb70fcde26f211/packages/radix-vue/src/Menu/MenuRoot.vue#L78-L85

Here are some profiler shots, micro view: image

and macro view: image

The images show a range of around 4k listeners suddenly jumping up to 23k listeners within 3 seconds as I am typing in my app.

spaceemotion commented 3 months ago

I've been looking at the code and am wondering why the "is the user using a keyboard or a finger/mouse" checks aren't something that's handled on a more global level (unless I misread what the handlers actually do)

romansp commented 3 months ago

I'm willing to work on this.

I think you're absolutely correct @spaceemotion - doesn't seem there's any reason to keep attaching and removing these event handlers per every keystroke and we should just have a global ref that's shared across all "Menu"-like components. From what it seems this was ported as is from Radix UI and never touched after.

Looks like a perfect spot for createSharedComposable from vueuse.