dulnan / nuxt-multi-cache

Advanced caching of components, routes and data for Nuxt 3. Dynamically define CDN cache control headers. Provides cache management API for purging items by key or using cache tags.
https://nuxt-multi-cache.dulnan.net
MIT License
193 stars 17 forks source link

Route caching limitations - nuxtApp.payload.path #39

Open or2e opened 4 months ago

or2e commented 4 months ago

https://github.com/nuxt/nuxt/issues/26071

dulnan commented 1 month ago

Currently trying to setup a reproduction. As you stated correctly, using any query parameter as part of the cache key is dangerous. And if I got that right, the bug happens when the first request has a query parameter and subsequent requests (served from cache), either with or without a query parameter will add the query param from the cached route in the browser.

I'm not sure how this could be fixed. It's indeed a bug, but kind of also an "expected bug" :smile:

I think the proper solution would be to enforce query parameters before rendering the page. So in our example, if a request is made to /test?id=foobar then the app should redirect to /test, if this route does not expect query parameters. And if it's something like /product?id=3, then the route should enforce just this one query param and also enforce a valid ID. So if someone requests /product?id=foobar, the result should be a 404.

or2e commented 1 month ago

The current workaround was to add a server-side plugin

import { getRequestURL } from 'h3';

export default defineNuxtPlugin((nuxtApp) => {
    if (nuxtApp.ssrContext === undefined) return;

    // https://github.com/nuxt/nuxt/issues/26071
    const url = getRequestURL(nuxtApp.ssrContext.event);
    url.searchParams.forEach((_value, key) => url.searchParams.delete(key));

    nuxtApp.payload.path = url.pathname;
});

But it's better to figure out why this is expected behavior in nitro: https://github.com/unjs/nitro/blob/main/src/runtime/internal/app.ts#L155 https://github.com/unjs/nitro/blob/main/src/runtime/internal/cache.ts I'll try to look at this later.

dulnan commented 1 month ago

But won't this lead to hydration issues? The cached route that is being served expects a certain query parameter value, say foo. When a request is made with value bar and when altering the path to then contain bar the SSR markup and entire state still expects foo. I guess it would be possible, as long as the query value is only used for client-side things. It does sound like a source of weird bugs though.

Something that crossed my mind just now is to offer a new method on the route cache helper, something like setQuery(). That way, it would be possible to do something like this:

<template>
  <div id="query-id">{{ id }}</div>
</template>

<script lang="ts" setup>
import { useRouteCache, useRoute, computed } from '#imports'

const route = useRoute()

const id = computed(() => route.query.id)

useRouteCache((helper) => helper.setCacheable().setQuery('id'))
</script>

This would give the route cache a hint about which query parameters (and possibly even values) would be allowed. It could then be used to build the appropriate cache key. If setQuery is not called, then the serveCachedRoute handler would instead return a redirect to the path without any query parameters. If it's called with setQuery('id'), the handler enforces a single query parameter with this name (and possible some value validation that would also be defined, a regex or so).