stealjs / steal

Gets JavaScript
https://stealjs.com
MIT License
1.37k stars 521 forks source link

Duplicate Requests when using cache key #1515

Closed MarcGodard closed 4 years ago

MarcGodard commented 4 years ago

Bug: When using cache key option, duplicate requests for the same css file (also using doneJS) one without the cache key and one with. In Production only. This Cache Key Feature.

Build doneJS project, Run in Production. Can also see this here: on my website in network

Software Version
Steal version 2.2.4
Steal-tools version 2.2.5
node -v v12.6.0
npm -v 6.9.0
Browser All
Operating system All
MarcGodard commented 4 years ago

Can someone respond? I notice that this cache key feature is still not working as expected.

matthewp commented 4 years ago

My guess is that the server-side version is not running the cache extension for some reason. Do you see the one with the cache key first or the one without?

MarcGodard commented 4 years ago

@matthewp I see the one without first.

Could it be something with my service (added here)?

const ssr = require('done-ssr-middleware')
const path = require('path')

module.exports = ssr({
  config: path.join(__dirname, '/package.json!npm'),
  main: 'site-template/index.stache!done-autorender',
  envs: {
    'server-production': {
      renderingBaseURL: '/dist',
      baseURL: 'public',
      bundlesPath: 'dist/bundles'
    },
    'server-development': {
      baseURL: 'client'
    }
  }
}, {
  strategy: 'seo',
  xhrCache: false,
  timeout: 30000
})
matthewp commented 4 years ago

I think you need to add cacheVersion to your server-production config. That's how it knows to append the version number. https://github.com/stealjs/steal/blob/c9dd1eb19ed3f97aeb93cf9dcea5d68ad5d0ced9/src/cache-bust/cache-bust.js#L10

MarcGodard commented 4 years ago

@matthewp when I do that I get this error in the console:

error: ENOENT: no such file or directory, open 'public/dist/bundles/site-template/index.js?v=10&v=10' {"errno":-2,"code":500,"syscall":"open","path":"public/dist/bundles/site-template/index.js?v=10&v=10","statusCode":404,"url":"file:public/dist/bundles/site-template/index.js?v=10&v=10"}

The index.js file is there.

Here are the changes I made:

const ssr = require('done-ssr-middleware')
const path = require('path')

module.exports = ssr({
  config: path.join(__dirname, '/package.json!npm'),
  main: 'site-template/index.stache!done-autorender',
  envs: {
    'server-production': {
      renderingBaseURL: '/dist',
      baseURL: 'public',
      bundlesPath: 'dist/bundles',
      cacheVersion: process.env.HEROKU_RELEASE_VERSION || 10,
      cacheKey: 'v'
    },
    'server-development': {
      baseURL: 'client'
    }
  }
}, {
  strategy: 'seo',
  xhrCache: false,
  timeout: 30000
})

Also not sure why its there twice index.js?v=10&v=10

You have any thoughts?

matthewp commented 4 years ago

Oh yeah, I guess that makes sense, you don't want to do it that way. This might be specific to CSS and not a general issue with the cache extension.

If you can, can you edit your node_modules/done-css/css.js file here: https://github.com/donejs/css/blob/30bf1b52a5c1bb7c1a1a96eadf8ddebceccda168/css.js#L65

And manually add the version just to see if that works?

link.setAttribute("href", css.href + '?v=123');
MarcGodard commented 4 years ago

@matthewp Yes, that worked. Unfortunately, I cannot deploy it with that.

matthewp commented 4 years ago

I'm looking into a possible fix.

matthewp commented 4 years ago

Couple of things:

  1. I'm seeing a bug in the cache extension where it puts the version number on twice for some reason.
  2. This would have to be a new option, something like renderingCacheVersion so it only applies to what gets rendered SSR. This would be something that only the CSS plugin knows about (for now).
matthewp commented 4 years ago

I think the duplicate number is just a product of the test helpers being used in the tests. Not sure it's a real bug, so going to proceed with #2.

matthewp commented 4 years ago

Moving this to done-css.

matthewp commented 4 years ago

Moved here: https://github.com/donejs/css/issues/66