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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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

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/12344234285/artifacts/2323808138

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";

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants