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

Http Request Cache Part 1 #2434

Merged
merged 14 commits into from
Aug 30, 2024

Conversation

AdityaAtulTewari
Copy link
Contributor

@AdityaAtulTewari AdityaAtulTewari commented Jul 24, 2024

RM-18292

refs #698 and cloudflare/workers-sdk#2514

Follow-up to reintroduce the behavior that was reverted in #2432

Updates the Request object to support specifying a cache mode per the standard. Only undefined, 'no-store' and 'no-cache' may be specified, and the fetch implementation specifically does not implement the semantics for either 'no-store' or 'no-cache' yet.
Also adds the compatibility flag for cacheHeaderEnabled. This just lays the ground work for the additional changes.

The next step is to start adding the support for adding the appropriate header fields in the fetch implementation behind an autogate

@AdityaAtulTewari AdityaAtulTewari requested review from a team as code owners July 24, 2024 17:14
@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch from 905efd7 to 975063b Compare July 24, 2024 17:37
@AdityaAtulTewari AdityaAtulTewari changed the base branch from main to AdityaAtulTewari/claim-cache-option-enabled-compat-flag July 24, 2024 17:57
Base automatically changed from AdityaAtulTewari/claim-cache-option-enabled-compat-flag to main July 24, 2024 18:16
@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch from 3afdcc0 to 0a12f2c Compare July 26, 2024 19:04
src/workerd/jsg/struct.h Outdated Show resolved Hide resolved
src/workerd/api/http-test.wd-test Outdated Show resolved Hide resolved
src/workerd/api/http-test.wd-test Outdated Show resolved Hide resolved
@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch from 0a12f2c to fff5de3 Compare July 29, 2024 17:17
src/workerd/jsg/jsg.h Outdated Show resolved Hide resolved
src/workerd/api/http.h Outdated Show resolved Hide resolved
src/workerd/api/http.c++ Outdated Show resolved Hide resolved
src/workerd/api/http.c++ Outdated Show resolved Hide resolved
@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch 3 times, most recently from 68a2c75 to 1d13ab0 Compare July 31, 2024 23:03
src/workerd/jsg/struct.h Outdated Show resolved Hide resolved
src/workerd/api/http.h Outdated Show resolved Hide resolved
@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch 2 times, most recently from 5aa3639 to bf88e9e Compare August 1, 2024 06:02
@AdityaAtulTewari
Copy link
Contributor Author

AdityaAtulTewari commented Aug 6, 2024

hmm, does the TS override here need to account for the difference in the cache property definition? I can't recall off the top of my head how the jsg::Unimplemented cache; is currently reflected in the generated types.

Based on the results of this PR: #2480 the typescript results are no different. Do we want to add tests for every frontend?

@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch from 78f8add to 4af033d Compare August 7, 2024 14:25
@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch from 8852ad7 to 330f6cc Compare August 12, 2024 21:52
@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch from e2f2390 to c147312 Compare August 13, 2024 02:46
Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Looks good to me (from the types generation point of view) 🙂

@dario-piotrowicz
Copy link
Member

In the non-experimental types files we get this addition:
Screenshot 2024-08-19 at 09 33 11

So, the comment plus the ?: undefined declaration

I think this quite ugly... but maybe acceptable? 🤷

Ideally I think we should not add anything in those files, but I have no idea about the technical limitations and whatnot, if this is the best we can do I think it's fine

@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch from fe835ed to a293314 Compare August 19, 2024 09:44
Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Looks perfect! thanks for the ts improvement @AdityaAtulTewari! 🫶

@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/http-request-cache-part1 branch from 62a34b9 to 30185d5 Compare August 21, 2024 19:20
@AdityaAtulTewari AdityaAtulTewari merged commit 92c8f1c into main Aug 30, 2024
13 checks passed
@AdityaAtulTewari AdityaAtulTewari deleted the AdityaAtulTewari/http-request-cache-part1 branch August 30, 2024 21:56
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.

5 participants