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

Static file cache issues #2686

Closed
2 of 5 tasks
grimasod opened this issue Apr 8, 2019 · 0 comments
Closed
2 of 5 tasks

Static file cache issues #2686

grimasod opened this issue Apr 8, 2019 · 0 comments
Labels
bug Bug reports
Milestone

Comments

@grimasod
Copy link
Contributor

grimasod commented Apr 8, 2019

Current behavior

Multiple related issues

  1. The static file default cache (maxAge) is 43 minutes
  2. service-worker.js uses this default maxAge setting, it should be zero
  3. Content-Type is not correctly set for service-worker.js

File: /core/scripts/server.js

const serve = (path, cache, options) => express.static(resolve(path), Object.assign({
  maxAge: cache && isProd ? 60 * 60 * 24 * 30 : 0
}, options))

(1) As stated in the Express docs, maxAge is in milliseconds (it's converted to seconds for the Response Header cache-control: max-age value)
https://expressjs.com/en/api.html#express.static

So this value 60 * 60 * 24 * 30 = 2592000 milliseconds converts to 2592 seconds = 43 minutes and 12 seconds. Presumably it was intended to be 30 days.

app.use('/service-worker.js', serve('dist/service-worker.js', {
  setHeaders: {'Content-Type': 'text/javascript; charset=UTF-8'}
}))

(2) The above function call only has 2 parameters, the middle one is missing. It should be a boolean, that determines whether or not to set the max-age value. Instead the second parameter is an object, which converts to True, so service-worker.js gets a max-age value when it shouldn't.

(Modern browsers might ignore this, but it's still better to have a zero value. It can also be overwritten by the web server, such as Nginx, but it should be correct by default)

(3) The third parameter, "options" is also missing , so the Content-Type doesn't get set. Furthermore, setHeaders must be a function, not an object. As described in the docs:
https://expressjs.com/en/api.html#setHeaders

Expected behavior

  1. The static file default cache (maxAge) is 1 month (or other value?)
  2. service-worker.js has zero max-age
  3. Content-Type is set to "text/javascript" for service-worker.js

Steps to reproduce the issue

  1. Open a browser tab/window with Developer Tools open on the Network tab
  2. Browse to https://demo.storefrontcloud.io/service-worker.js
  3. Select the service-worker.js file and note the following item in the Response Headers:
    "cache-control: public, max-age=2592"

Repository

Can you handle fixing this bug by yourself?

  • YES
  • NO

Preparing Pull Request

Which Release Cycle state this refers to? Info for developer.

Pick one option.

  • This is a bug report for test version on https://test.storefrontcloud.io - In this case Developer should create branch from develop branch and create Pull Request 2. Feature / Improvement back to develop.
  • This is a bug report for current Release Candidate version on https://next.storefrontcloud.io - In this case Developer should create branch from release branch and create Pull Request 3. Stabilisation fix back to release.
  • This is a bug report for current Stable version on https://demo.storefrontcloud.io and should be placed in next stable version hotfix - In this case Developer should create branch from hotfix or master branch and create Pull Request 4. Hotfix back to hotfix.

Environment details

  • Browser: Chrome 73
  • OS: Mac
  • Node: 8.11.4
  • Code Version: all, including 1.8.4, Master, Develop

Additional information

@grimasod grimasod added the bug Bug reports label Apr 8, 2019
@patzick patzick added this to the 1.10.0-rc.1 milestone Apr 8, 2019
@patzick patzick closed this as completed Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants