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

Serve correctly hashed static files with the Cache-Control header #10390

Merged
merged 9 commits into from Apr 15, 2020

Conversation

eashwar
Copy link
Contributor

@eashwar eashwar commented Apr 11, 2020

Issue: #8278

By @eashwar and @christopherhafke.

What we did

After reproducing the behavior described in #8278, we added the Cache-Control header to both the static files in the user-specified staticDir as well as any files served from /static/media (i.e., files loaded and hashed by file-loader).

How to test

To test this change, one can write any story that uses some static file (like an image) located in the public or src directories of a project. An example of a story that we wrote for examples/cra-react15:

import logo from '../logo.svg';
...
export const Story3 = () => (
  <Button onClick={action('clicked')}>
    <img src={logo} alt='A nice logo.'/>
  </Button>
);
Story3.story = { name: 'with an image'};

Run yarn storybook, go to the story (with browser devtools open). The response header for the image file should have the header Cache-Control. Reload the page, and the network request made for the image (in this example, logo.svg), should say something along the lines of 200 (from memory/disk cache) in the response header (as opposed to 304 not modified).

Is this testable with Jest or Chromatic screenshots?

We're unsure about how we could test something like this with Jest, or if that needs to be done at all - build-dev.js and dev-server.js don't seem to have tests written for them.

Does this need a new example in the kitchen sink apps?

This is unlikely, as the PR does not add any new functionality (instead improves existing functionality).

Does this need an update to the documentation?

No.

@eashwar
Copy link
Contributor Author

eashwar commented Apr 11, 2020

It looks like the build is failing - we rebased onto the (as of writing this message) latest commit on next, which also seems to be failing. Should we/will we have to rebase again when next reaches a non-failing state?

@eashwar eashwar force-pushed the 8278-cache-control-static-files branch from 03cbea3 to 9d25aa1 Compare April 14, 2020 16:59
@ndelangen ndelangen self-assigned this Apr 14, 2020
@ndelangen
Copy link
Member

So when a user DOES make a change to the file, will it still show the new version?

@christopherhafke
Copy link
Contributor

christopherhafke commented Apr 14, 2020

@ndelangen Yes, when changes are made (e.g. edits, creation) the new changes are correctly pulled in with the expected 200 OK response (rather than the 200 OK (from disk cache) or 200 OK (from memory cache) response from a cached asset).

@eashwar
Copy link
Contributor Author

eashwar commented Apr 14, 2020

However, this doesn't work for files without the hash in their name (i.e., files in the staticDir directories specified by the user like the public folder in a CRA app). Notably, adding this change to things that aren't in the /static/media folder wasn't actually asked for in the issue #8278 so we're going to remove that addition (but keep the added header for hashed files).

@donaldpipowitch
Copy link
Contributor

Is there a way to get this behaviour also for files covered by staticDir? I have fonts coming from there as well. I also tried to putting them into a /static/media directory, but files coming from /:staticDir/static/media don't seem to get the cache control header. It's really just for files which were directly referenced by file-loader if I'm not mistaken.

@shilman
Copy link
Member

shilman commented Dec 11, 2020

cc @eashwar @ndelangen

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.

None yet

5 participants