Skip to content
Open

Next #1192

Show file tree
Hide file tree
Changes from all 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