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

Serialize cache_ttl_by_status as object in cf properties #443

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

caiges
Copy link
Contributor

@caiges caiges commented Feb 14, 2024

Fixes #442

Serializes ttl_status_map as an object instead of a JavaScript Map using a serde_wasm_bindgen serializer.

Inspecting cf properties, TTL attributes are now present.

Before:

request cf properties: Cf { inner: IncomingRequestCfProperties { obj: Object { obj: JsValue(Object({"apps":true,"cacheEverything":true,"cacheKey":"","cacheTtl":0,"cacheTtlByStatus":{},"minify":{"__wbg_ptr":1455240},"mirage":true,"polish":"off","resolveOverride":"","scrapeShield":true})) } } }

After

request cf properties: Cf { inner: IncomingRequestCfProperties { obj: Object { obj: JsValue(Object({"apps":true,"cacheEverything":true,"cacheKey":"","cacheTtl":0,"cacheTtlByStatus":{"200-299":5,"300-399":10},"minify":{"__wbg_ptr":1311768},"mirage":true,"polish":"off","resolveOverride":"","scrapeShield":true})) } } }

Repeating the test from the reported issue:

const originResponse = await fetch(url, {
  cf: {
    cacheEverything: true,
    cacheTtlByStatus: {
      '200-299': 5,
      '300-399': 10,
    },
  }
});

The behavior now matches that of the JavaScript implementation:

172.69.64.52 - - [14/Feb/2024:04:55:56 +0000] "GET / HTTP/1.1" 304"
172.69.64.52 - - [14/Feb/2024:04:56:07 +0000] "GET / HTTP/1.1" 304"
172.69.64.52 - - [14/Feb/2024:04:56:18 +0000] "GET / HTTP/1.1" 304"
172.69.64.52 - - [14/Feb/2024:04:56:29 +0000] "GET / HTTP/1.1" 304"
172.69.64.52 - - [14/Feb/2024:04:56:40 +0000] "GET / HTTP/1.1" 304"

Copy link
Member

@kflansburg kflansburg left a comment

Choose a reason for hiding this comment

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

LGTM

@kflansburg
Copy link
Member

This test is broken on main, I'm looking into it

@kflansburg
Copy link
Member

I think I've fixed the test in #445 , once I land that we can rebase.

@kflansburg
Copy link
Member

@caiges Could you rebase?

@caiges
Copy link
Contributor Author

caiges commented Feb 26, 2024

@kflansburg ✔️

@kflansburg kflansburg merged commit 75a8408 into cloudflare:main Feb 26, 2024
3 checks passed
jdon pushed a commit to jdon/workers-rs that referenced this pull request Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Providing cache_ttl_by_status cache setting for fetch requests not respected.
2 participants