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

fix: only cache get requests #42

Merged
merged 1 commit into from
Jul 24, 2023
Merged

fix: only cache get requests #42

merged 1 commit into from
Jul 24, 2023

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Jul 24, 2023

Adding a HEAD request in another PR and came across this issue.

@alanshaw alanshaw merged commit e5b1fe2 into main Jul 24, 2023
@alanshaw alanshaw deleted the fix/only-cache-get-requests branch July 24, 2023 11:03
Copy link
Contributor

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

What happens if you try and cache a HEAD?

More generally I'd like to flag that I'm finding the withX pattern for workers less easy to work with than just providing simpler getX functions that a given route handler explictly calls rather than the "add deps to a context and assume they are set in a handler" pattern. In this case, magic auto caching is suprising... and it'd also be reasonable to want to cache HEAD responses, which would involve accessing the caches directly and re-writing the request to be a get for the cache, which is, for example, something a route handler should do rather than a withX

@alanshaw
Copy link
Member Author

What happens if you try and cache a HEAD?

Cloudflare throws an error when caching anything not GET.

More generally I'd like to flag that I'm finding the withX pattern for workers less easy to work

ACK, lets open an issue though because this perhaps not the best place to discuss.

"add deps to a context and assume they are set in a handler" pattern

There's no assumptions - every handler defined here checks their dependencies exist.

alanshaw pushed a commit that referenced this pull request Jul 24, 2023
🤖 I have created a release *beep* *boop*
---


##
[3.3.1](v3.3.0...v3.3.1)
(2023-07-24)


### Bug Fixes

* only add Content-Disposition header if not already exists
([#40](#40))
([f3cda76](f3cda76))
* only cache get requests
([#42](#42))
([e5b1fe2](e5b1fe2))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants