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

perf(nuxt): invalidate getRouteMeta cache on page contents change #25514

Merged

Conversation

BobbieGoede
Copy link
Member

@BobbieGoede BobbieGoede commented Jan 30, 2024

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Instead of using file contents as key, we cache the contents separately using absolutePath as key. While I'm not sure if there is much performance gained by not using large strings as keys, using the contents as key will add an entry on every file change, so with the current implementation the cache will keep on growing during development as old entries won't be removed.

(changed PR title to better communicate the change)

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

Copy link

stackblitz bot commented Jan 30, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely need to fix the ever-growing cache. But we can't cache based on the path as the path will stay the same even if the contents changes (e.g. if the user updates the meta and the site hot-reloads).

@BobbieGoede
Copy link
Member Author

BobbieGoede commented Jan 30, 2024

We definitely need to fix the ever-growing cache. But we can't cache based on the path as the path will stay the same even if the contents changes (e.g. if the user updates the meta and the site hot-reloads).

I know, this change uses a second cache to store the contents, if the contents have changed the metaCache entry is cleared.

This is still indirectly using contents to cache metaCache, but doing so by caching contents by absolutePath separately in pageContentsCache. This way the keys aren't large, but the bigger gain is not having the cache grow on every change, only compared on every change.

@danielroe
Copy link
Member

danielroe commented Jan 30, 2024

Ahh. Great! That sounds like a good solution.

Will recheck it.

@BobbieGoede BobbieGoede changed the title perf: use absolutePath as cache key in getRouteMeta perf(nuxt): use absolutePath as cache key in getRouteMeta Jan 30, 2024
@BobbieGoede BobbieGoede changed the title perf(nuxt): use absolutePath as cache key in getRouteMeta perf(nuxt): invalidate getRouteMeta cache on page contents change Jan 31, 2024
@BobbieGoede BobbieGoede force-pushed the perf/get-route-meta-caching-by-path branch 2 times, most recently from 2f79eec to d133285 Compare January 31, 2024 14:01
@BobbieGoede
Copy link
Member Author

BobbieGoede commented Jan 31, 2024

I thought I had those flaky scroll to top tests figured out 🀦 now it's failing in Ubuntu..

@BobbieGoede BobbieGoede force-pushed the perf/get-route-meta-caching-by-path branch 2 times, most recently from 2bad97b to 39c5727 Compare February 4, 2024 11:33
@BobbieGoede BobbieGoede force-pushed the perf/get-route-meta-caching-by-path branch from 39c5727 to 06bc1f5 Compare February 4, 2024 21:06
@danielroe danielroe merged commit 3e5560e into nuxt:main Feb 4, 2024
33 of 34 checks passed
@github-actions github-actions bot mentioned this pull request Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants