Skip to content
Open

Next #1192

Show file tree
Hide file tree
Changes from 6 commits
Commits
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
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

## Release channels

This repository ships on two channels:
This repository publishes on two channels:

| Channel | Branch | Example version | JSR resolution |
|---------|--------|-----------------|----------------|
Expand Down
2 changes: 1 addition & 1 deletion deno.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@deco/deco",
"version": "1.197.0",
"version": "1.197.1-next.2",
"lock": false,
"nodeModulesDir": "auto",
"exports": {
Expand Down
2 changes: 1 addition & 1 deletion dev/deno.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@deco/dev",
"version": "1.197.0",
"version": "1.197.1-next.2",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: Version regression in dev/deno.json: changed from 1.200.1-next.1 to 1.197.1-next.2, but the PR intent is to bump to 1.200.1-next.1. This downgrade will cause @deco/dev to be published at an older version than intended, creating inconsistency with sibling packages.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/deno.json, line 3:

<comment>Version regression in `dev/deno.json`: changed from `1.200.1-next.1` to `1.197.1-next.2`, but the PR intent is to bump to `1.200.1-next.1`. This downgrade will cause `@deco/dev` to be published at an older version than intended, creating inconsistency with sibling packages.</comment>

<file context>
@@ -1,6 +1,6 @@
 {
   "name": "@deco/dev",
-  "version": "1.200.1-next.1",
+  "version": "1.197.1-next.2",
   "exports": {
     "./tailwind": "./tailwind.ts"
</file context>
Suggested change
"version": "1.197.1-next.2",
"version": "1.200.1-next.1",

"exports": {
"./tailwind": "./tailwind.ts"
},
Expand Down
35 changes: 16 additions & 19 deletions runtime/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,27 +390,24 @@ export const middlewareFor = <TAppManifest extends AppManifest = AppManifest>(
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: Treating all Set-Cookie headers as cache-disqualifying also blocks caching for framework segment cookies.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/middleware.ts, line 432:

<comment>Treating all `Set-Cookie` headers as cache-disqualifying also blocks caching for framework segment cookies.</comment>

<file context>
@@ -501,16 +424,29 @@ export const middlewareFor = <TAppManifest extends AppManifest = AppManifest>(
-        shouldCacheFromVary: ctx.var?.vary?.shouldCache !== false,
-      });
+
+      if (hasSetCookie) {
+        // Set-cookie present: never cache (same behavior as main)
+        newHeaders.set("Cache-Control", "no-store, no-cache, must-revalidate");
</file context>

const segment = tryOrDefault(() => JSON.parse(cookieSegment), {});

const active = new Set(segment.active || []);
const inactiveDrawn = new Set(segment.inactiveDrawn || []);
// Track only the `active` set. The previous `inactiveDrawn` set was
// write-only state — never read by any matcher logic — and its growth
// across pages was the main reason `deco_segment` got rewritten on
// almost every request, tripping the Set-Cookie kill-switch below and
// making HTML uncacheable. Removing it lets cohort assignment stick.
const previousActive = [...new Set<string>(segment.active || [])].sort();
const active = new Set<string>(previousActive);
for (const flag of ctx.var.flags) {
if (flag.isSegment) {
if (flag.value) {
active.add(flag.name);
inactiveDrawn.delete(flag.name);
} else {
active.delete(flag.name);
inactiveDrawn.add(flag.name);
}
}
if (!flag.isSegment) continue;
if (flag.value) active.add(flag.name);
else active.delete(flag.name);
}
const newSegment = {
active: [...active].sort(),
inactiveDrawn: [...inactiveDrawn].sort(),
};
const value = JSON.stringify(newSegment);
const hasFlags = active.size > 0 || inactiveDrawn.size > 0;

if (hasFlags && cookieSegment !== value) {
const newActive = [...active].sort();
const activeChanged =
JSON.stringify(previousActive) !== JSON.stringify(newActive);

if (active.size > 0 && activeChanged) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: When all active segment flags are removed, the cookie is never updated/cleared, leaving stale deco_segment state.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/middleware.ts, line 409:

<comment>When all active segment flags are removed, the cookie is never updated/cleared, leaving stale `deco_segment` state.</comment>

<file context>
@@ -390,27 +390,24 @@ export const middlewareFor = <TAppManifest extends AppManifest = AppManifest>(
+        const activeChanged =
+          JSON.stringify(previousActive) !== JSON.stringify(newActive);
+
+        if (active.size > 0 && activeChanged) {
+          const value = JSON.stringify({ active: newActive });
           const date = new Date();
</file context>

const value = JSON.stringify({ active: newActive });
const date = new Date();
date.setTime(date.getTime() + 30 * 24 * 60 * 60 * 1000); // 1 month
setCookie(
Expand Down
3 changes: 2 additions & 1 deletion runtime/routes/_meta.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ export const handler = createHandler(async (ctx) => {
return new Response(JSON.stringify(value), {
headers: {
"Content-Type": "application/json",
"cache-control": "must-revalidate",
"cache-control":
"public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate",
Comment on lines +24 to +25

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

HTTP cache-control must-revalidate and stale-while-revalidate compatibility

💡 Result:

The Cache-Control directives must-revalidate and stale-while-revalidate serve fundamentally different purposes and are generally considered incompatible when used together, as they impose conflicting requirements on how a cache must handle stale content [1][2][3]. must-revalidate The must-revalidate directive is a strict instruction that prohibits a cache from serving a stale response without first successfully revalidating it with the origin server [1][2][3]. If the cache is unable to reach the origin (e.g., due to network disconnection), it must return an error (typically a 504 Gateway Timeout) rather than serving the stale content [2][3][4]. This directive is intended for scenarios where serving stale data could lead to incorrect operations, such as financial transactions [2][3][4]. stale-while-revalidate The stale-while-revalidate directive is designed to improve performance by allowing a cache to serve a stale response immediately while simultaneously revalidating it in the background [1][5][6]. This directive explicitly permits the use of stale content to avoid blocking the user, prioritizing availability and latency over strict freshness [5][6]. Compatibility and Interaction When both directives are present in the same Cache-Control header, they create a conflict: - must-revalidate mandates that stale content must not be served until revalidation is complete [2][3]. - stale-while-revalidate permits stale content to be served immediately while revalidation happens in the background [5][6]. In practice, because must-revalidate is a strict requirement that caches are mandated not to ignore [2][3][4], it effectively overrides the permissive behavior of stale-while-revalidate. A compliant cache will prioritize the must-revalidate constraint, forcing the client to wait for the revalidation to complete before receiving a response, thereby negating the performance benefits of stale-while-revalidate [1][2][3]. Consequently, these directives should not be used together if the goal is to leverage the background revalidation capabilities of stale-while-revalidate [1].

Citations:


🏁 Script executed:

cat -n runtime/routes/_meta.ts | sed -n '20,30p'

Repository: deco-cx/deco

Length of output: 414


Remove must-revalidate to enable stale-while-revalidate behavior.

The cache-control header combines must-revalidate and stale-while-revalidate=300, which are conflicting directives. Per RFC 9111:

  • must-revalidate strictly prohibits serving stale content without successful revalidation
  • stale-while-revalidate=300 permits serving stale content for 300 seconds while revalidating in the background

When both are present, must-revalidate takes precedence, effectively nullifying the stale-while-revalidate optimization. The 300-second stale window will never be used.

If the intent is to allow stale responses during revalidation (performance optimization), remove must-revalidate. If strict freshness is required, remove stale-while-revalidate=300.

Proposed fix (assuming stale-while-revalidate is intended)
      "cache-control":
-        "public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate",
+        "public, max-age=60, s-maxage=60, stale-while-revalidate=300",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"cache-control":
"public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate",
"cache-control":
"public, max-age=60, s-maxage=60, stale-while-revalidate=300",
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@runtime/routes/_meta.ts` around lines 24 - 25, The Cache-Control header
string currently includes both "must-revalidate" and
"stale-while-revalidate=300", which conflict; update the "cache-control" header
value (the "cache-control" property in this module) to remove "must-revalidate"
so it becomes "public, max-age=60, s-maxage=60, stale-while-revalidate=300" (or
remove the stale directive instead if strict revalidation is desired).

Comment on lines +24 to +25

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: This response is now publicly cacheable while CORS headers vary by request origin, but Vary: Origin is missing. Add Vary: Origin (or avoid shared caching) to prevent cached cross-origin header mismatches.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/routes/_meta.ts, line 24:

<comment>This response is now publicly cacheable while CORS headers vary by request origin, but `Vary: Origin` is missing. Add `Vary: Origin` (or avoid shared caching) to prevent cached cross-origin header mismatches.</comment>

<file context>
@@ -21,7 +21,8 @@ export const handler = createHandler(async (ctx) => {
     headers: {
       "Content-Type": "application/json",
-      "cache-control": "must-revalidate",
+      "cache-control":
+        "public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate",
       etag,
</file context>
Suggested change
"cache-control":
"public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate",
"cache-control":
"public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate",
vary: "Origin",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: must-revalidate and stale-while-revalidate=300 are conflicting directives. Per RFC 9111, must-revalidate strictly prohibits serving stale content, which nullifies the stale-while-revalidate optimization that allows serving stale responses during background revalidation. The 300-second stale window will never be used. Remove must-revalidate if the intent is to benefit from stale-while-revalidate, or remove stale-while-revalidate=300 if strict freshness is required.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/routes/_meta.ts, line 25:

<comment>`must-revalidate` and `stale-while-revalidate=300` are conflicting directives. Per RFC 9111, `must-revalidate` strictly prohibits serving stale content, which nullifies the `stale-while-revalidate` optimization that allows serving stale responses during background revalidation. The 300-second stale window will never be used. Remove `must-revalidate` if the intent is to benefit from stale-while-revalidate, or remove `stale-while-revalidate=300` if strict freshness is required.</comment>

<file context>
@@ -21,7 +21,8 @@ export const handler = createHandler(async (ctx) => {
       "Content-Type": "application/json",
-      "cache-control": "must-revalidate",
+      "cache-control":
+        "public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate",
       etag,
       ...allowCorsFor(ctx.req.raw),
</file context>
Suggested change
"public, max-age=60, s-maxage=60, stale-while-revalidate=300, must-revalidate",
"public, max-age=60, s-maxage=60, stale-while-revalidate=300",

etag,
...allowCorsFor(ctx.req.raw),
},
Expand Down
2 changes: 1 addition & 1 deletion scripts/deno.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@deco/scripts",
"version": "1.197.0",
"version": "1.197.1-next.2",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: Version downgraded from 1.200.1-next.1 to 1.197.1-next.2, which contradicts the PR's stated goal of bumping to 1.200.1-next.1. This may be a merge/revert artifact that left scripts/deno.json at an older version than intended.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/deno.json, line 3:

<comment>Version downgraded from 1.200.1-next.1 to 1.197.1-next.2, which contradicts the PR's stated goal of bumping to 1.200.1-next.1. This may be a merge/revert artifact that left scripts/deno.json at an older version than intended.</comment>

<file context>
@@ -1,6 +1,6 @@
 {
   "name": "@deco/scripts",
-  "version": "1.200.1-next.1",
+  "version": "1.197.1-next.2",
   "exports": {
     "./release": "./release.ts",
</file context>
Suggested change
"version": "1.197.1-next.2",
"version": "1.200.1-next.1",

"exports": {
"./release": "./release.ts",
"./update": "./update.run.ts",
Expand Down
Loading