add default Cache-Control header to files router #4348
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4348 +/- ##
==========================================
- Coverage 92.7% 92.65% -0.05%
==========================================
Files 118 118
Lines 8346 8351 +5
==========================================
+ Hits 7737 7738 +1
- Misses 609 613 +4
Continue to review full report at Codecov.
|
|
this should probably be exposed as a function / options upon initializing parse-server. |
|
@flovilmart I add a The last failed test seems not related to my code, can you help me run the tests again? |
|
@zhxiaogg just retriggered the last stage of the build, should be good in a moment. |
|
@montymxb @flovilmart can you help me reviewing it and can this be merged? |
src/Options/index.js
Outdated
| @@ -64,6 +64,8 @@ export interface ParseServerOptions { | |||
| webhookKey: ?string; | |||
| /* Key for your files */ | |||
| fileKey: ?string; | |||
| /* Cache-Control header for files router */ | |||
| filesCacheControl: ?string; | |||
There was a problem hiding this comment.
Note: definitions are generated from this file, just add the default value after an run the script in resources/buildConfigDefinitions.js
There was a problem hiding this comment.
Can you add this into the Advanced Options section of the README as well. We'll want to have this visibly documented.
Beyond that I really like this, nice bit of functionality.
|
Just to note someone pointed out that it might be handy to also have each file indicate it's own cache control as well. We don't need that for this PR, but it would be great to have macro/micro control on caching for files. Have the ability to set global cache control (this), and maybe later add the ability to have files indicate their own cache control as well to supersede the global (for particular cases as needed). |
|
@zhxiaogg bump |
|
@montymxb @flovilmart Sorry for delayed update. I added a default value for the setting and also added it to the README. |
|
Nice, I'll have to take a look at this later.
…On Dec 10, 2017 19:07, "XiaoGuang Zhang" ***@***.***> wrote:
@montymxb <https://github.com/montymxb> @flovilmart
<https://github.com/flovilmart> Sorry for delayed update. I added a
default value for the setting and also added it to the README.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4348 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE1d4C5A2BxENKGrHUHdj_k_hX_oFw_1ks5s_JyMgaJpZM4Qdk8p>
.
|
src/Options/Definitions.js
Outdated
| @@ -137,7 +137,8 @@ module.exports.ParseServerOptions = { | |||
| }, | |||
| "filesCacheControl": { | |||
| "env": "PARSE_SERVER_FILES_CACHE_CONTROL", | |||
| "help": "Cache-Control header for files router" | |||
| "help": "Cache-Control header for files router", | |||
| "default": "public, max-age=86400" | |||
There was a problem hiding this comment.
I don't think we want a default value here. Changing expected behavior by introducing caching would cause unexpected issues if someone needed to update their file within that 24 hr period. It would be best if it was unset as it was before.
| @@ -232,6 +232,7 @@ The client keys used with Parse are no longer necessary with Parse Server. If yo | |||
| #### Advanced options | |||
|
|
|||
| * `fileKey` - For migrated apps, this is necessary to provide access to files already hosted on Parse. | |||
| * `filesCacheControl` - Set `Cache-Control` header when serving files with builtin files router. Defaults to `public, max-age=86400` (1 day). | |||
There was a problem hiding this comment.
Thanks for adding this. Still not sure about the default value though...@flovilmart do you want to weigh in on this?
src/Options/index.js
Outdated
| @@ -65,7 +65,7 @@ export interface ParseServerOptions { | |||
| /* Key for your files */ | |||
| fileKey: ?string; | |||
| /* Cache-Control header for files router */ | |||
| filesCacheControl: ?string; | |||
| filesCacheControl: ?string; // = public, max-age=86400 | |||
There was a problem hiding this comment.
Same here, if we drop the default we'll want to remove this comment.
There was a problem hiding this comment.
Overall looks good with adding in the Advanced Options category. Not entirely onboard with setting the default value though. Waiting to see what @flovilmart thinks of this.
|
I’d like to keep the defaults empty as well, also most of the files should be served by a 3rd party (S3 etc..) how does this apply on those cases? |
|
Closing, as having many conflicts, please reopen and address all the concerns. |
are there any plans to include caching of files in the roadmap? |
ghost
mentioned this pull request
I find this issue: #1857 but the
Cache-Controlheader is not yet implemented in the files router, so I add some simple code.I think the header value should be configurable, but I'm not sure where to put it. Need help.