-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add cache headers to responses #1454
base: master
Are you sure you want to change the base?
Conversation
[ | ||
() => `${serverUrl}/v1/projects/${projectId}`, | ||
() => `${serverUrl}/v1/projects/${projectId}/forms/${encodeURIComponent(xmlFormId)}`, | ||
() => `${serverUrl}/v1/projects/${projectId}/forms/${encodeURIComponent(xmlFormId)}.svc/Submissions('cache-test-submission')`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be reduced to testing a single URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
@lognaturel I think there's an argument for setting |
Resubmission of #1390
Closes getodk/central#662
What has been done to verify that this works as intended?
Added tests.
Why is this the best possible solution? Were any other approaches considered?
It seems simple.
An alternative approach was tried in #1390, but it was decided that content should be cached for less time.
This PR also adds more extensive tests which were not present in #1390.
Previously,
max-age=0
was being used, to ensure that any cached values were immediately considered stale. This has been changed tono-cache
, which allows caching but requires revalidation on every use. See:How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Should prevent future caching surprises.
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
No? "cache" is not currently mentioned in docs/
Before submitting this PR, please make sure you have:
make test
and confirmed all checks still pass OR confirm CircleCI build passes