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

Add hash to webpack requests to enable caching #9776

Merged
merged 3 commits into from
Feb 12, 2021
Merged

Add hash to webpack requests to enable caching #9776

merged 3 commits into from
Feb 12, 2021

Conversation

afshin
Copy link
Member

@afshin afshin commented Feb 11, 2021

References

Related to #9762. cc @zhamujun

Code changes

Add a version argument to generated webpack output for both the main build and prebuilt extensions when in production mode.
This is needed because the server is deciding whether to cache static files based on the existence of this argument, which is also the behavior of tornado's StaticFileHandler. This same behavior exists when running using the classic notebook server.

The files generated by webpack are of the form 3341.cf37a29a49e9bc70ba18.js (with no argument), but the requests it makes include the version argument, allowing the caching behavior to work.

User-facing changes

JupyterLab will load faster when built in production mode (after the files are cached).

Backwards-incompatible changes

N/A

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor

I'm confused. I think we want caching - that's why we have the content hash in the filename, so caching would work. But your comment seems to indicate that we do not want caching.

@jasongrout
Copy link
Contributor

jasongrout commented Feb 12, 2021

First, it does seem that this improves the current situation.

A couple of thoughts:

  1. The no-cache header (according to MDN) does not mean that the request should not be cached, only that the cache should be validated each time with a lightweight request. Is this actually preventing caching in practice? Or is this lightweight request even out of the question (for example, a proxy cache that actually decides to store/not store based on no-cache?) (@zhamujun, is that what you are seeing?)
  2. It seems that we should fix things at the server level to not add that no-cache when we know we don't need it (like in our case where we are serving assets we've specifically built to be cache-friendly). Looking at the Jupyter Server code, it seems it is written as a blunt hammer for assets that do not handle caching properly. I mean, it's pretty heavy-handed to not cache anything we are serving, unless the request happens to have a ?v= argument. It feels like we've introduced a workaround in Jupyter Server for assets that don't handle caching properly, and now we have to work around the work around to say that yes, we are handling caching properly.
  3. The Jupyter Server filehandler is also returning None for the etags. Is that also messing up validating the cache?

Copy link
Contributor

@jasongrout jasongrout left a comment

Choose a reason for hiding this comment

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

Since this does make things better with the existing Jupyter Server conventions, +1 to merging (with my suggested comment change, if my comment change is correct), though I think in the long run it's better to make Jupyter Server have better caching conventions.

builder/src/extensionConfig.ts Outdated Show resolved Hide resolved
@jasongrout
Copy link
Contributor

I've made a Jupyter Server issue for tweaking the default (or at least handler-level) caching behavior at jupyter-server/jupyter_server#413

@jasongrout
Copy link
Contributor

jasongrout commented Feb 12, 2021

Arguably, we should also set the immutable cache-control header on these hashed webpack resources for firefox/webkit users to avoid even 304 requests. In a different PR in some future time...

@jasongrout
Copy link
Contributor

The Jupyter Server filehandler is also returning None for the etags. Is that also messing up validating the cache?

Answering this: the change to not use etags was made in jupyter/notebook@7ea4db6. Min decided there that we would use last-modified time to control caching, rather than explicitly compute a sha1 hash etag (which apparently is Tornado's default behavior). I think this is fine, and certainly is cheaper, and does not mess up cache validation.

@blink1073 blink1073 changed the title Add hash to bundle to prevent aggressive caching Add hash to webpack requests to enable caching Feb 12, 2021
blink1073 and others added 2 commits February 12, 2021 06:13
Co-authored-by: Jason Grout <jasongrout@users.noreply.github.com>
@blink1073 blink1073 merged commit 9da5dd6 into jupyterlab:master Feb 12, 2021
@blink1073
Copy link
Contributor

@meeseeksdev please backport to 3.0.x

@lumberbot-app
Copy link

lumberbot-app bot commented Feb 12, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout 3.0.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 9da5dd6cb9a656b9f9f4644daf676e52eaa19f18
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #9776: Add hash to webpack requests to enable caching'
  1. Push to a named branch :
git push YOURFORK 3.0.x:auto-backport-of-pr-9776-on-3.0.x
  1. Create a PR against branch 3.0.x, I would have named this PR:

"Backport PR #9776 on branch 3.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@zhamujun
Copy link

zhamujun commented Feb 12, 2021

  1. The no-cache header (according to MDN) does not mean that the request should not be cached, only that the cache should be validated each time with a lightweight request. Is this actually preventing caching in practice? Or is this lightweight request even out of the question (for example, a proxy cache that actually decides to store/not store based on no-cache?) (@zhamujun, is that what you are seeing?)

Yes. I was seeing a "304" response.

@mykhailovs
Copy link

Hello! Got the same issue with Jupyterlab 1.2.7
While I'm building jupyterlab with flag --dev-build=False I still get 'no-cache' in 'cache-control' header for vendor-main**.js file. May I ask you to help me, maybe there is a way to change this header to something like 'public,max-age=123456'
Thanks

@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Apr 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintenance status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants