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

Enable cache: no-cache compat flag and enum #3195

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

AdityaAtulTewari
Copy link
Contributor

@AdityaAtulTewari AdityaAtulTewari commented Nov 30, 2024

First part of cache: no-cache implementation:

  • Work-shopping the test infrastructure in workerd.
  • Allow no-cache construction
  • Update typescript information

Note that the actual implementation is incompelete.

@AdityaAtulTewari AdityaAtulTewari requested review from a team as code owners November 30, 2024 16:16
@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/no-cache-enable-enum branch 2 times, most recently from bb9478f to 64003ed Compare November 30, 2024 16:32
@fhanau
Copy link
Collaborator

fhanau commented Dec 2, 2024

I'll need some context here to review this well:

  • Is the compat flag the right enabling mechanism here? Are there cases where supporting cache:no-store will be incompatible? Or is this just to support using it while it is not fully stable?
  • Should this be landed while incomplete? Are there any internal PRs I should look out for?

@AdityaAtulTewari
Copy link
Contributor Author

AdityaAtulTewari commented Dec 2, 2024

Is the compat flag the right enabling mechanism here? Are there cases where supporting cache:no-store will be incompatible? Or is this just to support using it while it is not fully stable?

There are no cases where cache: no-store will be incompatible, it is just for support while we conduct additional tests on the internal side. I intend to have the no-store flag, enable this flag after a certain compat date.

Should this be landed while incomplete? Are there any internal PRs I should look out for?

In the past this is how we did it because it allowed me to experiment with tests in live workers before we pushed this feature to customers. I think that this same principle of experimentation applies here. There are no internal PRs that are associated with this change yet.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but let's be sure to test internally before merging

@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/no-cache-enable-enum branch from 64003ed to 7e4252c Compare December 16, 2024 01:31
Copy link

github-actions bot commented Dec 16, 2024

The generated output of @cloudflare/workers-types has been changed by this PR. If this is intentional, run bazel build //types && rm -rf types/generated-snapshot && cp -r bazel-bin/types/definitions types/generated-snapshot to update the snapshot. Alternatively, you can download the full generated types: https://github.com/cloudflare/workerd/actions/runs/12553943601/artifacts/2373014600

Full Type Diff
diff -r bazel-bin/types/definitions/experimental/index.d.ts types/generated-snapshot/experimental/index.d.ts
1680c1680
<   cache?: "no-store" | "no-cache";
---
>   cache?: "no-store";
1694c1694
<   cache?: "no-store" | "no-cache";
---
>   cache?: "no-store";
diff -r bazel-bin/types/definitions/experimental/index.ts types/generated-snapshot/experimental/index.ts
1688c1688
<   cache?: "no-store" | "no-cache";
---
>   cache?: "no-store";
1702c1702
<   cache?: "no-store" | "no-cache";
---
>   cache?: "no-store";

@AdityaAtulTewari AdityaAtulTewari force-pushed the AdityaAtulTewari/no-cache-enable-enum branch from 7e4252c to 984e5d1 Compare December 31, 2024 01:09
@AdityaAtulTewari AdityaAtulTewari merged commit 4ff974a into main Dec 31, 2024
14 of 16 checks passed
@AdityaAtulTewari AdityaAtulTewari deleted the AdityaAtulTewari/no-cache-enable-enum branch December 31, 2024 01:46
@danlapid
Copy link
Collaborator

@AdityaAtulTewari this PR was merged when type checks didn't pass. Please fix types.

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.

4 participants