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

fix(memory): defaultThemeOptions moved inline theme.ts #18824

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

memic84
Copy link

@memic84 memic84 commented Dec 5, 2023

Description

When using vuetify in Nuxt SSR, with high traffic there are memory leaks on certain objects. I was able to isolate those to the theme composable and fix them. Resolves 18393

Markup:

This repository can be used to reproduce: https://github.com/memic84/vuetify-memory-leak-reproduction

First you should link your local vuetify code with yarn link and yarn link "vuetify"

and then you can run Nuxt.

yarn && yarn build && node --inspect .output/server/index.mjs

To test the fix, link the PR code to the vuetify reproduction code.

@johnleider
Copy link
Member

At a glance this seems very heavy handed. I'll carve out some time this week to investigate.

@MajesticPotatoe MajesticPotatoe added T: bug Functionality that does not work as intended/expected E: theme Theme composable labels Dec 6, 2023
@johnleider
Copy link
Member

I took a different approach to this. I'm not convinced rtlClasses needs to change either. More info here af07b7f

If you want to refactor this to only address rtlClasses that's fine, but I'm going to need a valid reproduction that shows the leak. Thank you!

@memic84
Copy link
Author

memic84 commented Dec 7, 2023

I took a different approach to this. I'm not convinced rtlClasses needs to change either. More info here af07b7f

If you want to refactor this to only address rtlClasses that's fine, but I'm going to need a valid reproduction that shows the leak. Thank you!

@johnleider

I have split the PR into two. This one contains the locale composable: #18787

I was able te reproduce and isolate the memory leak in (the computed rtlClass): https://github.com/vuetifyjs/vuetify/blob/2c2b7deba00146f7918dfac90ea9dc31c6c86200/packages/vuetify/src/composables/locale.ts#L91C12-L91C12 (not sure why the computed is leaking, best guess is that ik keeps references)

Here is the reproduction: https://github.com/memic84/vuetify-memory-leak-reproduction

When running the nuxt build with node --inspect, and use ApacheBench or something similair (every 200 users), the memory keeps going up.

Steps are also mentioned here in the description: #18393

@johnleider
Copy link
Member

Your P.R.s both contain the same changes. Please make sure #18787 doesn't have the same stuff as this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E: theme Theme composable S: stale This issue is untriaged and hasn't seen any activity in at least six months. T: bug Functionality that does not work as intended/expected
3 participants