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

Content hashes change based on entry order #281

Closed
lfre opened this issue Sep 19, 2018 · 9 comments
Closed

Content hashes change based on entry order #281

lfre opened this issue Sep 19, 2018 · 9 comments

Comments

@lfre
Copy link
Contributor

lfre commented Sep 19, 2018

Compiling scss to css, I noticed that the contenthash on files changed when a new entry was added before, even though the content of the files have not changed. Without this plugin, the contenthash in the JS files remain the same regardless of order, which tells me it's not webpack, or sass-loader.

Any help is appreciated. Find the minimal reproduce scenario below:

Node: 8.12.0 (LTS)
Plugin: 0.4.3
css-loader: 1.0.0
sass-loader: 7.1.0
Webpack: 4.19.1

  entry: {
    file1: './file1.scss'
    file2: './file2.scss', // When commented out, the contenthash for all entries after (file3.[contenthash].css), will change regardless if the content has changed.
    file3: './file3.scss' 
  },
  output: {
	path: resolve('./'),
  },
  plugins: [
    new MiniCssExtractPlugin({
      filename: '[name].[contenthash].css',
    }),
  ],
  module: {
    rules: [
      {
        test: /\.scss$/,
        use: [
          {
            loader: MiniCssExtractPlugin.loader,
          },
          {
            loader: 'css-loader',
          },
          {
            loader: 'sass-loader',
          },
        ],
      },
    ],
  },
@michael-ciniawsky
Copy link
Member

entry: {
  file1: './file1.scss'
  file2: './file2.scss',
  file3: './file3.scss' 
}

(S)CSS (non-JS) entries aren't supported by webpack (yet), use a JS entry and import the (S)CSS files instead

entry.js

import file1 from './file1.scss'
...
entry: {
  app: './entry.js'
}

@lfre
Copy link
Contributor Author

lfre commented Sep 19, 2018

@michael-ciniawsky It's the same issue. I can't have 1 single entry.js import all scss files, because that will result in 1 single css file. I need a css file per scss entry (route based). If I do:

entry: {
   file1: './file1.js',
   file2: './file2.js',
   file3: './file3.js'
}

Where each js file does a import file from './file[n].scss'. If I comment out file2 from the entry, the contenthash of file3.css changes for no apparent reason.

@michael-ciniawsky
Copy link
Member

I can't have 1 single entry.js import all scss files, because that will result in 1 single css file. I need a css file per scss entry (route based)

It was just an example... Still you need to import the CSS into the JS file (per route)

Sync Chunks

{
   entry: {
     admin: './admin.js' // import styles from './admin.scss'
     dashboard: './dashboard.js' // import styles from './dashboard.scss'
     ...
   }
}

Async Chunks

// import styles from './route.scss' (in route.js)
import('./route.js').then((module) => ...)

Anyways...

Where each js file does a import file from './file[n].scss'. If I comment out file2 from the entry, the contenthash of file3.css changes for no apparent reason.

Please provide a small reproduction repo

@lfre
Copy link
Contributor Author

lfre commented Sep 20, 2018

@michael-ciniawsky I took a quick look and it's unclear to me what the contentHash is supposed to represent because it's not the contents of the CSS file, unfortunately. I replaced this line https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/index.js#L243 with hash.update(m.content) and it accurately maps to the file contents, so when I remove/add entry points above, the hash doesn't change. I'll reply back with the link to the repo for further investigation. Thanks!

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Sep 20, 2018

https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/index.js#L242-L244

if (m.type === MODULE_TYPE) {
  m.updateHash(hash);
}

https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/index.js#L91-L96

updateHash(hash) {
    super.updateHash(hash); // <= 1.
    hash.update(this.content);
    hash.update(this.media || '');
    hash.update(JSON.stringify(this.sourceMap || ''));
}

https://github.com/webpack/webpack/blob/master/lib/Module.js#L310-L314

updateHash(hash) {
  hash.update(`${this.id}`); // <= 2.
  hash.update(JSON.stringify(this.usedExports)); // <= 3.
  super.updateHash(hash);
}

I took a quick look and it's unclear to me what the contentHash is supposed to represent because it's not the contents of the CSS file, unfortunately.

Yeah, I think you found a bug here :). You're right to be under the impression that [contenthash] is supposed to rely on the modules contents exclusively. I don't know why it was implemented that way or why it might be needed tbh but it's definitely weird...

@lfre
Copy link
Contributor Author

lfre commented Sep 20, 2018

@michael-ciniawsky I created a repo here: https://github.com/lfre/mini-css-extract-plugin-contenthash/. Instructions are in README and a comment: https://github.com/lfre/mini-css-extract-plugin-contenthash/blob/master/webpack.config.js#L10

The issue is hash.update(${this.id}); because when CSSModule extends from webpack's Module, ids are indexed by default. If you define a predictable id i.e this.id = dependency.context in the constructor, it should fix it without (hopefully) any side-effects. https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/index.js#L43. Let me know what you think and I can put up a PR for it. Thanks!

@michael-ciniawsky
Copy link
Member

I will take a look, but it's already clear that there is an issue and I will ping folks in slack to take a look at this aswell, ideally Tobias (sokra) give us some insights on why adding the id may still be required (atm) I can't reliably comment on that

@sokra
Copy link
Member

sokra commented Sep 21, 2018

I think in this case id isn't needed. Actually we don't even need to assign a id here. You can try to assign this.id = "" in the constructor of the module. Maybe this works.

@lfre
Copy link
Contributor Author

lfre commented Sep 22, 2018

Thanks for looking into it @michael-ciniawsky @sokra. I opened a PR to fix it, I'd appreciate any feedback on updating the tests. Thanks!

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

No branches or pull requests

3 participants