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

Caching issues in Development since migrating to Shakapacker #88

Closed
AlecRust opened this issue Mar 29, 2022 · 23 comments · Fixed by #108 or #234
Closed

Caching issues in Development since migrating to Shakapacker #88

AlecRust opened this issue Mar 29, 2022 · 23 comments · Fixed by #108 or #234
Assignees
Labels

Comments

@AlecRust
Copy link
Contributor

AlecRust commented Mar 29, 2022

We used to run a pretty standard Webpacker 5 setup, which in Development resulted in HTML like this:

<head>
  <link rel="stylesheet" media="all" href="/packs/css/application-e793e27b.css" data-turbo-track="reload" />
  <script src="/packs/js/application-57eff0528424c89ab096.js" data-turbo-track="reload" defer="defer"></script>
  <!-- etc. -->

We then migrated to (equally plain/default) Shakapacker 6 setup, which in Development now results in HTML like this:

<head>
  <link rel="stylesheet" media="all" href="/packs/css/application.css" data-turbo-track="reload" />
  <script src="/packs/js/runtime.js" data-turbo-track="reload" defer="defer"></script>
  <script src="/packs/js/vendors-node_modules_webpack-dev-server_client_index_js_protocol_ws_3A_hostname_localhost_por-54b5bb.js" data-turbo-track="reload" defer="defer"></script>
  <script src="/packs/js/vendors-node_modules_babel_runtime_regenerator_index_js-node_modules_hotwired_turbo-rails_app-56e6f5.js" data-turbo-track="reload" defer="defer"></script>
  <script src="/packs/js/application.js" data-turbo-track="reload" defer="defer"></script>
  <!-- etc. -->

We expected SplitChunks to be enabled here, however the contenthash has also been removed from some filenames, like runtime.js and most problematically application.js/application.css.

This is resulting in caching issues where the e.g. application.js file gets updated by Shakapacker during development, but the browser serves a cached version even after page refresh since the filename hasn't changed.

I can fix this with the following config/webpack/webpack.config.js, but it adds the missing hash only to the JS file, not also the CSS:

const { webpackConfig, merge } = require('shakapacker')

const customConfig = {
  // Add contenthash to filenames in development to prevent browser serving cached files
  output: {
    filename: '[name].[contenthash].js'
  }
}

module.exports = merge(webpackConfig, customConfig)

Is this an expected change in Shakapacker? It doesn't seem a good default?

Edit: Looks like this might be a performance thing? Still, this is unexpected when migrating from Webpacker and probably not a good default.

@AlecRust AlecRust added the bug label Mar 29, 2022
@bgwilson87
Copy link

I agree. I'm in the process of migrating to shakapacker, and it took me a while to figure out why my js and css changes were not updating in my browser on development (application.js and application.css were being cached since they no longer had the contenthash)

@bgwilson87
Copy link

Here's a workaround that includes adding the contenthash to css

const { webpackConfig, merge } = require('shakapacker')

let miniCssExtractPlugin = webpackConfig.plugins.find((p, i) => { return p.constructor.name === 'MiniCssExtractPlugin' });

if (miniCssExtractPlugin && miniCssExtractPlugin.options) {
  miniCssExtractPlugin.options.filename = 'css/[name]-[contenthash:8].css';
  miniCssExtractPlugin.options.chunkFilename = 'css/[id]-[contenthash:8].css';
}

const customConfig = {
  output: {
    filename: 'js/[name]-[contenthash].js',
    chunkFilename: 'js/[name]-[contenthash].chunk.js'
  }
}

module.exports = merge(webpackConfig, customConfig)

@AlecRust AlecRust changed the title Caching issues since Webpacker -> Shakapacker Caching issues in Development since migrating to Shakapacker Apr 1, 2022
@tomdracz
Copy link
Collaborator

tomdracz commented Apr 2, 2022

Thanks for reporting. Not entirely sure I would agree with this being unreasonable default in development. For me, it does make sense to skip expensive operations in environment where you'll see frequent changes and recompiles.

This being a change to the behaviour from webpacker is unfortunate and I wouldn't be surprised if it wasn't documented properly. The whole road from webpacker to shakapacker, with v6 being in development hell for months did leave us with gaps in changelog and docs. Any suggestion for improvements would be greatly appreciated!

What are your development setups? Never thought about it and haven't looked into it fully but I've half expected dev server to get assets out with no cache headers to prevent any browser caching - that feels fairly sensible. If it doesn't do that, then I agree we should address that in one way or another.

Will look into it further!

@tomdracz
Copy link
Collaborator

tomdracz commented Apr 3, 2022

@AlecRust @bgwilson87 What browsers are you seeing this in? Just tried to replicate and cannot quite get the issue you're seeing.

Headers from my dev server response below:
Screenshot 2022-04-03 at 11 35 34

Notice that:

  • No cache control header
  • No last-modified header
  • Etag header is present however and that will serve as the main way of caching/cache busting in the default config

There is caching there based on Etag header however I can see this busted fine with changes:

  • Very first fresh request after dropping cache gives me 200 OK response
  • Reload, either triggered manually or through changing a file without HMR - I get 304 Not Modified response - request served from cache as expected
  • If I modify my main pack file, I get another ETag and 200 OK - again as expected
  • If I modify any other file in the import tree, I will get new ETag and 200 OK for that file but the main pack file will still be served from cache as it's not changed in this scenario - it's referencing an import but not the code in the imported file directly, hence it's still feasible to have it cached

Is the above not what you're seeing? Would be great to get more details about your setups and how the staleness ends up happening. Any repro case would be great if we are to dig into this further

@justin808
Copy link
Member

I added this. It was based on https://webpack.js.org/guides/build-performance/#avoid-production-specific-tooling.

Rails "sprockets" also works like this.

Maybe there's some interaction with using bundle splitting that doesn't work well?

However, @tomdracz, that's a very nice explanation of how this setting is more efficient for development.

@bgwilson87
Copy link

Thanks for the explanations, and also thanks for taking the webpacker torch and continuing to support it. I went through the effort a few years ago to completely get off of sprockets in my project, so I was dismayed to hear that Rails is going in a different direction.

I was able to resolve my issues by running this command to toggle caching (I had a caching-dev.txt file in my tmp directory)
rails dev:cache

https://github.com/rails/rails/blob/53410537594be0c0a528cff53dce433dd386cb6a/railties/lib/rails/generators/rails/app/templates/config/environments/development.rb.tt#L18

It may be helpful to mention this in the v6 upgrade docs in case others have caching enabled on development.

@tomdracz
Copy link
Collaborator

tomdracz commented Apr 4, 2022

Thanks for the update @bgwilson87 Good detective work! I'm struggling to replicate this behaviour locally. If you could try enabling caching again and attaching the response headers from the asset requests, that would be great! Locally, I still get no Cache-Control on the asset so would be good to see behaviour on yours.

@AlecRust
Copy link
Contributor Author

AlecRust commented Apr 4, 2022

Looks like I already had caching disabled for Development when I checked rails dev:cache.

I'm struggling to reproduce this issue, but it didn't seem to happen every time originally. I experienced it a couple times myself, and others on my team experienced it too (pack files change but browser serves cached application.js or applicatipm.css file). We're on Chromium/Brave mostly, I don't know if the browser itself had a temporary caching bug. All we narrowed this down to is the lack of contenthash in the filenames, and nobody has reported this issue since manually adding the contenthash.

Also worth noting this prevented the issue:
image

I see Cache-Control: no-cache which allows the browser to cache the file, is this what we want?

image

I don't have very concrete information for you sadly, but it seems the contenthash removal in Shakapacker 7 has the potential to cause caching issues in Development. I wonder if contenthash in filenames should be an option in webpacker.yml?

@bgwilson87
Copy link

Here's what I get when caching is enabled:
Screen Shot 2022-04-04 at 11 32 30 AM

@tomdracz
Copy link
Collaborator

tomdracz commented Apr 4, 2022

@bgwilson87 Are you running webpack dev server? I think I got to the bottom of this:

  • If not running webpack dev server - I get caching with tmp/caching-dev.txt
  • If running webpack dev server - I do not, all works as expected

HOWEVER, if I've already managed to run the app once with caching and without dev server running, then I will still get cached version as it will respect the Cache-Control header set originally - this will persist until I clear the cache or wait until it clears. I expect you're hitting similar scenario - either not running dev server or something triggering a request before dev server is running.

This makes sense now - with dev server running we will proxy everything to express app started by that, without it, we compile on the fly and use Rails public server hence caching applies.

Great spot, I will update the docs to take note of all of that!

@AlecRust

Also worth noting this prevented the issue:
I see Cache-Control: no-cache which allows the browser to cache the file, is this what we want?

Yeah, Cache-Control: no-cache is sent by Chrome-like browsers when you toggle that Disable cache - it will revalidate the asset and stop you from seeing the problem, but yeah, it's still there!

I still don't think content hash should be added to development but I will document that as a feasible option for those who want to keep it. Initially as config override as outlined somewhere above but if we think it's useful enough, I would be happy to get this going as an option on opt-in basis.

@bgwilson87
Copy link

@tomdracz that all sounds right, I am not using webpack dev server.

And if I run rails webpacker:clobber to remove the compiled output, and then use webpack dev server, with tmp/caching-dev.txt, I see the same functionality as you (no Cache-Control header)

@justin808
Copy link
Member

@tomdracz the problem seems related to shakapacker assuming that development means the headers don't use caching.

I'm frankly surprised that turning on caching for the Rails server also turns on caching for the browsers.

If it does, then maybe webpacker could pick that up and adjust the config?

How about if we check that if tmp/caching-dev.txt exists, then we keep the hashes in development?

However, this depends on having that snippet of standard code to be in development.rb. Then again, rails dev:cache depends on that to work.

@tomdracz
Copy link
Collaborator

tomdracz commented Apr 24, 2022

@tomdracz the problem seems related to shakapacker assuming that development means the headers don't use caching.

I'm frankly surprised that turning on caching for the Rails server also turns on caching for the browsers.

If it does, then maybe webpacker could pick that up and adjust the config?

How about if we check that if tmp/caching-dev.txt exists, then we keep the hashes in development?

However, this depends on having that snippet of standard code to be in development.rb. Then again, rails dev:cache depends on that to work.

I'd honestly avoid anything more here. We could start picking up the rails config value but that feels bit too magical and us veering towards doing too much again.

What we do need is better way to adjust those configs to make stuff like enabling digests in filenames much easier.

@bughit
Copy link

bughit commented Dec 7, 2022

@tomdracz

I have caught Chrome multiple times loading undigested packs from memory (devtools show that). I don't know how it gets into such a state but it does. CTRL-F5 snaps it out of it and then it starts revalidating (if-modified-since). But somehow it eventually happens again. The only way to avoid this with 100% certainty is with digests in the file names, so ideally that should be easy to enable with a single .yml config key.

@justin808 justin808 reopened this Dec 8, 2022
@justin808
Copy link
Member

If:

  1. This is really a problem
  2. The extra compile time from generating hashes is small

then it might be worth adding the hashes back as the default.

@bughit
Copy link

bughit commented Dec 8, 2022

Here's one scenario that reproduces it:

  • had the page with the pack in question open, with devtools ("disable cache" is not on)
  • at this point pack reloads are revalidated
  • closed chrome
  • restarted
  • previous tabs were restored
  • opened devtools ("disable cache" is not on)
  • normal refresh
  • the pack was reloaded from memory
    image
  • and will continue to be for subsequent refreshes

@Judahmeek
Copy link
Contributor

@bughit so this problem doesn't exist if "disable cache" is active, correct?

@bughit
Copy link

bughit commented Dec 28, 2022

@bughit so this problem doesn't exist if "disable cache" is active, correct?

Haven't checked.

@sschafercdd
Copy link

I tried various things to get this to work, but was unsuccessful in disabling caching.

So I added the following to config/webpack/development.js to force fingerprinting of JS & CSS files. Seems to work just fine.

  // add hashing of generated JS files
  clientWebpackConfig.output.filename = 'js/[name]-[contenthash].js'
  clientWebpackConfig.output.chunkFilename = 'js/[name]-[contenthash].chunk.js'

  // add hashing of generated CSS files
  clientWebpackConfig.plugins.forEach(plugin => {
    if (plugin.options && plugin.options.filename === 'css/[name].css') {
      plugin.options.filename = 'css/[name]-[contenthash].css'
    }
  })

@justin808
Copy link
Member

@sschafercdd any idea of what the root cause of needing a content hash in development is?

@scottschafer
Copy link

I don't know exactly why, but JS and CSS files are being served up with caching allowed, no matter what I've tried. So adding the content hash prevents stale JS/CSS from being used.

@justin808
Copy link
Member

See #234. Should we finish this one up, @ahangarha? This is not a blocker for v7.

@ahangarha ahangarha self-assigned this Jun 6, 2023
@ahangarha
Copy link
Contributor

@scottschafer
May you please try justin808/allow-config-useContentHash branch and confirm if it fixes your issue? Ensure you set useContentHash: true for the development environment in config/shakapacker.yml (or perhaps, in your case, config/webpacker.yml).

Please let us know the result.

We are about to release version 7. This might be the last piece to add to the new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment