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

Route caching limitations - nuxtApp.payload.path #39

Open
or2e opened this issue Mar 4, 2024 · 3 comments
Open

Route caching limitations - nuxtApp.payload.path #39

or2e opened this issue Mar 4, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@or2e
Copy link
Contributor

or2e commented Mar 4, 2024

nuxt/nuxt#26071

@dulnan
Copy link
Owner

dulnan commented Jun 16, 2024

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" 😄

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.

@dulnan dulnan added the bug Something isn't working label Jun 16, 2024
@or2e
Copy link
Contributor Author

or2e commented Jun 16, 2024

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
Copy link
Owner

dulnan commented Jun 17, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants