Skip to content

Feature/query caching and debug flags #961

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rvveber
Copy link
Collaborator

@rvveber rvveber commented May 12, 2025

Perfomance Improvement: Adds query caching that is persisted across browser restarts
Developer Exprience Improvement: Adds and documents usage of debug flags

@rvveber rvveber force-pushed the feature/query-caching-and-debug-flags branch 2 times, most recently from b9cc806 to 436410d Compare May 12, 2025 17:07
@rvveber rvveber marked this pull request as ready for review May 12, 2025 17:08
@rvveber rvveber force-pushed the feature/query-caching-and-debug-flags branch from 436410d to 1084952 Compare May 13, 2025 08:25
Query-results are automatically stored in localstorage.
This leverages caching in a controlled way,
improves render-blocking loading times
and unnecessary repeated requests.

- Adds @tanstack/react-query-persist-client
- Adds @tanstack/query-sync-storage-persister
- Updates @tanstack/react-query

Signed-off-by: Robin Weber <[email protected]>
@rvveber rvveber force-pushed the feature/query-caching-and-debug-flags branch 2 times, most recently from 47ef827 to 867840e Compare May 13, 2025 09:17
Debug flags allow the application to behave differently.
They can be set in various ways
and allow to debug certain aspects of the application.
They can ease development
and help debug errors in production environments.

Debug flags need to be enabled with debug namespace(s).
e.g."feature:language" would instruct the application
to be more verbose about its language features.
e.g. "no-cache" would instruct the application
to avoid caching entirely.

The value for the debug configuration
is always a string
with debug namespaces separated by comma.

Signed-off-by: Robin Weber <[email protected]>
@rvveber rvveber force-pushed the feature/query-caching-and-debug-flags branch 2 times, most recently from 94f78cb to b0de2ee Compare May 13, 2025 15:03
@rvveber rvveber requested a review from AntoLC May 15, 2025 09:47
@@ -761,6 +768,9 @@ def post_setup(cls):
"OIDC_ALLOW_DUPLICATE_EMAILS cannot be set to True simultaneously. "
)

if cls.DEBUG and "no-cache" in cls.DEBUG_NAMESPACES:
cls.THEME_CUSTOMIZATION_CACHE_TIMEOUT = 0
Copy link
Member

Choose a reason for hiding this comment

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

You can directly set this settings in your environment variable IMO

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes in env.d\development\common.

Copy link
Collaborator Author

@rvveber rvveber May 20, 2025

Choose a reason for hiding this comment

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

As long as it is only theme customization we are caching.
But what if there will be more in the future?

In an attempt to achieve the same functionality on the backend
i found this to be a functional example to illustrate how the debug namespaces would work.
(The frontend has the same functionality)

Comment on lines +61 to +67
DEBUG_RAW = values.Value(
default=False,
environ_name="DEBUG",
environ_prefix=None,
)
DEBUG_NAMESPACES = DEBUG_RAW.split(",") if isinstance(DEBUG_RAW, str) else []
DEBUG = bool(DEBUG_RAW)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really confident with this and It's I think error prone.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, it is.

Arguably we can remove the extended DEBUG handling for the backend.

I see too options:
A: Remove the debug handling for the backend
B: Write a dedicated function to handle it solidly

Which to choose?

Copy link
Collaborator

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

It is not working as expected, it keeps all the requests in cache, by doing like that:

  • the logout is not working anymore
  • the doc list is in cache so if someone invite you or send you a public link, it will not appear anymore in the doc list (until the cache is invalidate)
  • I guess all the requests that change regularly between multiple users will be in cache, giving lot of side effect during collaboration

What could be interesting is to have by default all the requests not in cache (as we have already), and to decide the ones that we want to have with a persistant cache, like useConfig.
⚠️ After we have a subtility with useConfig, it has a persistant cache, but we still do the request in background, if the response has some changes, it will rehydrate the components with the new changes.

Comment on lines +21 to +42
if (process.env.NODE_ENV === 'development') {
/**
* Enable debug namespaces as needed
*
* They can be enabled:
*
* via DEBUG environment variable
* DEBUG="features:language,no-cache,..."
*
* via browser console
* window.debug = "features:language,no-cache,...";
*
* via Local storage
* window.localStorage.debug = "features:language,no-cache,...";
*
* via code (uses Local storage)
* import debug from 'debug';
* debug.enable('no-cache,features:language,...');
*/
//debug.enable('no-cache,features:language');
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you use this part ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is just here to illustrate usage for future development and enable quick toggle from code.

Comment on lines +28 to +29
staleTime: debug.enabled('no-cache') ? 0 : 1000 * 60 * 3, // 3 minutes
gcTime: debug.enabled('no-cache') ? 0 : 1000 * 60 * 60 * 48, // 48 hours
Copy link
Collaborator

@AntoLC AntoLC May 16, 2025

Choose a reason for hiding this comment

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

About the debug, I am not sure it is the best way to do.
As a developer I like to be near the production setting, it helps me in this case to see which query I have to invalidate. If I have no-cache, I will not know that when I change the title of a document I need to invalidate the cache of the document list request.

After if you don't want cache, we could set these variables from settings:

Suggested change
staleTime: debug.enabled('no-cache') ? 0 : 1000 * 60 * 3, // 3 minutes
gcTime: debug.enabled('no-cache') ? 0 : 1000 * 60 * 60 * 48, // 48 hours
staleTime: process.env.NEXT_PUBLIC_CACHE_STALE_TIME,
gcTime: process.env.NEXT_PUBLIC_CACHE_GC_TIME

By default they will be in .env:

NEXT_PUBLIC_CACHE_STALE_TIME=1000 * 60 * 3
NEXT_PUBLIC_CACHE_GC_TIME=1000 * 60 * 60 * 48

but in .env.local you could set them as you want, .env.local is not versioned:

NEXT_PUBLIC_CACHE_STALE_TIME=0
NEXT_PUBLIC_CACHE_GC_TIME=0

https://nextjs.org/docs/pages/guides/environment-variables#environment-variable-load-order

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback.

  1. Debug Mode and Cache: The debug flag for cache control is off by default, aligning with production behavior. Developers who enable it for local debugging are intentionally altering cache settings for temporary diagnostic purposes.

  2. Data Updates and Cache Invalidation: For data modifications (e.g., updating a document title), we should adhere to the mutation pattern (as demonstrated here). This ensures that related queries are correctly invalidated and fresh data is fetched.

  3. Configurability of staleTime and gcTime:

    • Making the default staleTime and gcTime configurable via environment variables could introduce complexity. Because each useQuery can override defaults - we would need to add env configuration for each useQuery that has a different staleTime and gcTime.
    • Generally, we, as developers designing the queries, are best positioned to determine appropriate staleTime and gcTime values based on the nature of the data. Allowing arbitrary instance-level customization or overly flexible environment configurations might lead to inconsistent caching behavior and unexpected issues. It's preferable to establish sensible, well-reasoned defaults within the codebase for different types of queries.

@@ -761,6 +768,9 @@ def post_setup(cls):
"OIDC_ALLOW_DUPLICATE_EMAILS cannot be set to True simultaneously. "
)

if cls.DEBUG and "no-cache" in cls.DEBUG_NAMESPACES:
cls.THEME_CUSTOMIZATION_CACHE_TIMEOUT = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes in env.d\development\common.

@rvveber
Copy link
Collaborator Author

rvveber commented May 20, 2025

It is not working as expected, it keeps all the requests in cache, by doing like that:

  • the logout is not working anymore
  • the doc list is in cache so if someone invite you or send you a public link, it will not appear anymore in the doc list (until the cache is invalidate)
  • I guess all the requests that change regularly between multiple users will be in cache, giving lot of side effect during collaboration

Thanks for the catch!
It was naive of me thinking it would be a drop-in replacement.
There are lot of cases that need extensive testing and adaption.

To the points you found i will address the following additional points in the next days:

  • add cache busting with process.env.NEXT_PUBLIC_BUILD_ID - to avoid using cache from old builds after updates.
  • exempt the authQuery from beeing persisted
  • test all existing queries
  • ensure all mutations that access queries properly invalidate the query

What could be interesting is to have by default all the requests not in cache (as we have already), and to decide the ones that we want to have with a persistant cache, like useConfig. ⚠️ After we have a subtility with useConfig, it has a persistant cache, but we still do the request in background, if the response has some changes, it will rehydrate the components with the new changes.

This is already happening. The staleTime is the time that defines when a query will be refetched.
If staleTime has passed and the page experiences some update, for example a re-focus of the browser tab,
the query will be refetched instantly and the cache is rehydrated with the updated data.

It is just important for us to properly invalidate queries.

@rvveber rvveber marked this pull request as draft May 21, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants