fix: change strategy for route caching #10248
Conversation
🦋 Changeset detectedLatest commit: c3f1678 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: astro
ematipico
force-pushed
the
fix/change-key-cache
branch
from
a8f5242
to
83a4a0f
Compare
| if (this.mode === 'production' && this.cache[route.route]?.staticPaths) { | ||
| this.logger.warn(null, `Internal Warning: route cache overwritten. (${route.route})`); |
There was a problem hiding this comment.
I think this will switch one conflict for another. Imagine a project with two integrations such that:
- Integration A injects route
/[...slug]pre-rendered that generates the paths/fooand/bar - Integration B injects route
/[...slug]pre-rendered that generates the paths/bazand/qux - A
src/pages/[...slug].astrofile in the project that generates the path/homeand/about
Those three are the same route, so they would conflict in the cache. Two integrations may define the same route for different components, and a single integration may define the same component for different routes.
Maybe the cache key should be the combination of both
There was a problem hiding this comment.
Maybe the cache key should be the combination of both
I thought about it too! And thank you for the comment, it's good to see that I'm not the only one :) I'll apply the change
|
This |
* main: (327 commits) [ci] format fix(node): listen on 0.0.0.0 if server.host is set to true (withastro#10282) [ci] format fix(dev): cosider `base` when special-casing `/_image` (withastro#10274) [ci] format update login flow to support Brave (withastro#10258) [ci] format improve link command (withastro#10257) Updates deprecated Node.js 16 github actions (withastro#10270) Fix Vitest check fail again (withastro#10266) [ci] format Adds auto completion of `astro:` events when adding or removing event listeners on `document` (withastro#10263) Update Vite to latest (withastro#10259) [ci] release (withastro#10236) [ci] format fix(i18n): localised index pages are overwritten (withastro#10250) fix: change strategy for route caching (withastro#10248) Fix TypeScript type definitions for `Code` component (withastro#10251) chang changeset (withastro#10253) Removes morph animations when setting transition:animate=none (withastro#10247) ...
Changes
Closes #8241
Closes PLT-1716
This PR changes the key of our internal route caching. Previously, we were using
RouteData#component, which used to cover most of the cases, although it seems too restrictive, because the same component can emit multiple paths.This means that, given the minimum use case, the first time we hit
/blog/world, we correctly render the route and cache its params, props and static paths using the/blog/[post].astroas key. When we attempt to render/fr/blog/world, we hit the cache because this path is rendered by the same component, although the static paths don't match, so we emit an error and return a 404.With this change, we use
RouteData#routeas key. Given the previous use case, we cache the entry using the/blog/[post]key when rendering/blog/world. Then, when we attempt to render/fr/blog/world,RouteData#routevalue is/fr/blog/[post], which isn't inside the cache and we correctly resolve the route and its static paths.Testing
I tested in a local project, and it works. The current tests should pass.
I tried to add an integration test, but I was receiving a strange error, I will investigate later.
Docs
N/A, bugfix