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.

Sign up for GitHub

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

fix: change strategy for route caching #10248

Merged
merged 2 commits into from
Feb 28, 2024
Merged

fix: change strategy for route caching #10248

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83a4a0f
fix: change strategy for route caching
ematipico Feb 27, 2024
c3f1678
use route and component as cache key
ematipico Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/great-flowers-own.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

Fixes an issue where multiple injected routes with the same `entrypoint` but different `pattern` were incorrectly cached, causing some of them not being rendered in the dev server.
8 changes: 4 additions & 4 deletions packages/astro/src/core/render/route-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,14 @@ export class RouteCache {
// NOTE: This shouldn't be called on an already-cached component.
// Warn here so that an unexpected double-call of getStaticPaths()
// isn't invisible and developer can track down the issue.
if (this.mode === 'production' && this.cache[route.component]?.staticPaths) {
this.logger.warn(null, `Internal Warning: route cache overwritten. (${route.component})`);
if (this.mode === 'production' && this.cache[route.route]?.staticPaths) {
this.logger.warn(null, `Internal Warning: route cache overwritten. (${route.route})`);
Copy link
Member

@Fryuni Fryuni Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 /foo and /bar
  • Integration B injects route /[...slug] pre-rendered that generates the paths /baz and /qux
  • A src/pages/[...slug].astro file in the project that generates the path /home and /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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.cache[route.component] = entry;
this.cache[route.route] = entry;
}

get(route: RouteData): RouteCacheEntry | undefined {
return this.cache[route.component];
return this.cache[route.route];
}
}

Expand Down
Loading