css-loader icon indicating copy to clipboard operation
css-loader copied to clipboard

SSR: generated class names are not consistent for client and server bundles since v5.1.3

Open OliverJAsh opened this issue 4 years ago • 25 comments

Bug report

We are using css-loader with server-side rendering (SSR), so we run webpack twice: once for the client and once for the server.

For the client, we need to generate CSS files, so we use mini-css-extract-plugin in combination with css-loader.

For the server, we don't need to generate CSS files, so we use exportOnlyLocals: true. We just need the CSS modules to export the same class names as the ones used in the CSS files we generated for the client (above).

Actual Behavior

Generated class names are not consistent for client and server bundles.

Expected Behavior

Generated class names are consistent for client and server bundles.

How Do We Reproduce?

Full reduced test case: https://github.com/OliverJAsh/webpack-css-loader-ident-hash-bug

package.json:

{
  "dependencies": {
    "css-loader": "^6.3.0",
    "mini-css-extract-plugin": "^2.4.2",
    "webpack": "^5.58.1",
    "webpack-cli": "^4.9.0"
  }
}

webpack.config.js:

const MiniCssExtractPlugin = require("mini-css-extract-plugin");

const { MODE } = process.env;

module.exports = {
    plugins: [new MiniCssExtractPlugin()],
    mode: "production",
    entry: "./src/main.js",
    module: {
        rules: [
            {
                test: /\.css$/,
                exclude: /node_modules/,
                use: [
                    ...(MODE === "client" ? [MiniCssExtractPlugin.loader] : []),
                    {
                        loader: "css-loader",
                        options: {
                            modules: {
                                exportOnlyLocals: MODE === "server",
                            },
                        },
                    },
                ],
            },
        ],
    },
};

src/main.js:

import styles from './main.css'
console.log(styles.container);

src/main.css:

.container {
    display: block;
}

Since this change which first appeared in v5.1.3, css-loader generates different class names for the client/server:

$ rm -rf dist && MODE=client webpack && node dist/main.js
UgjPTt4sSFtvFL9kN_LV

$ rm -rf dist && MODE=server webpack && node dist/main.js
BZjOBwj614XLb60af45_

It seems this is because css-loader now uses relativeMatchResource as part of the hash, and in this case, relativeMatchResource will be different depending on whether or not we're using mini-css-extract-plugin:

  • MODE=server (mini-css-extract-plugin is used): relativeMatchResource is ''
  • MODE=client (mini-css-extract-plugin is not used): relativeMatchResource is 'src/main.css\x00'

We are able to workaround the issue by downgrading to v5.1.2 which does not include this change.

Please paste the results of npx webpack-cli info here, and mention other relevant information

  System:
    OS: macOS 11.6
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 3.09 GB / 16.00 GB
  Binaries:
    Node: 16.3.0 - ~/.nvm/versions/node/v16.3.0/bin/node
    Yarn: 1.19.1 - /usr/local/bin/yarn
    npm: 7.15.1 - ~/.nvm/versions/node/v16.3.0/bin/npm
  Browsers:
    Chrome: 94.0.4606.71
    Chrome Canary: 96.0.4664.2
    Firefox: 90.0.2
    Firefox Nightly: 77.0a1
    Safari: 15.0
    Safari Technology Preview: 15.4
  Packages:
    css-loader: ^6.3.0 => 6.3.0
    webpack: ^5.58.1 => 5.58.1
    webpack-cli: ^4.9.0 => 4.9.0

OliverJAsh avatar Oct 08 '21 15:10 OliverJAsh

hm, weird, I will look at this in near future

alexander-akait avatar Oct 08 '21 15:10 alexander-akait

Also got that error. Downgrade to Node 14 did not solve the problem

inoyakaigor avatar Oct 08 '21 16:10 inoyakaigor

After upgrading to 6.4.0 which contains the fix from #1385, I'm getting this error in my app:

[webpack-cli] HookWebpackError: Circular hash dependency 54047 (runtime.54047.js) -> dff12 (education-route.dff12.js) -> 54047 (runtime.54047.js)
    at makeWebpackError (/Users/oliverash/Development/unsplash-web/node_modules/webpack/lib/HookWebpackError.js:48:9)
    at /Users/oliverash/Development/unsplash-web/node_modules/webpack/lib/Compilation.js:2974:12
    at eval (eval at create (/Users/oliverash/Development/unsplash-web/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:25:1)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
-- inner error --
Error: Circular hash dependency 54047 (runtime.54047.js) -> dff12 (education-route.dff12.js) -> 54047 (runtime.54047.js)
    at add (/Users/oliverash/Development/unsplash-web/node_modules/webpack/lib/optimize/RealContentHashPlugin.js:263:16)
    at add (/Users/oliverash/Development/unsplash-web/node_modules/webpack/lib/optimize/RealContentHashPlugin.js:270:9)
    at /Users/oliverash/Development/unsplash-web/node_modules/webpack/lib/optimize/RealContentHashPlugin.js:276:7
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
caused by plugins in Compilation.hooks.processAssets
Error: Circular hash dependency 54047 (runtime.54047.js) -> dff12 (education-route.dff12.js) -> 54047 (runtime.54047.js)
    at add (/Users/oliverash/Development/unsplash-web/node_modules/webpack/lib/optimize/RealContentHashPlugin.js:263:16)
    at add (/Users/oliverash/Development/unsplash-web/node_modules/webpack/lib/optimize/RealContentHashPlugin.js:270:9)
    at /Users/oliverash/Development/unsplash-web/node_modules/webpack/lib/optimize/RealContentHashPlugin.js:276:7
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

I suspect this is a regression from the other change in that release: https://github.com/webpack-contrib/css-loader/pull/1382

Update: never mind, fixed by changing contenthash:5 to contenthash.

OliverJAsh avatar Oct 11 '21 10:10 OliverJAsh

Yes, try to avoid uses very small value for hashes, we have ideas how to improve this for webpack v6, but now we have some limitations with very small value of contenthash

alexander-akait avatar Oct 11 '21 10:10 alexander-akait

@alexander-akait After updating to 6.4.0, I'm getting classname collisions. I'm trying to figure out how the two change in the release would do that, but reverting to 6.3.0 fixes them.

jsg2021 avatar Oct 12 '21 16:10 jsg2021

@jsg2021 hm, can you try to revert these changes locally https://github.com/webpack-contrib/css-loader/commit/c7db752fe6a9c7ff28d165fd24a37be08ef83af5?

alexander-akait avatar Oct 12 '21 16:10 alexander-akait

And can you provide example of collusion, i.e. class/filename/context?

alexander-akait avatar Oct 12 '21 16:10 alexander-akait

I'll test that.

Here is an example of collision:

import { styled } from 'astroturf/react';
import { User } from '../commons';

export const Container = styled.div`
	cursor: pointer;
	width: 70px;
	height: 70px;
	display: flex;
	align-items: center;
	justify-content: center;

	img,
	svg {
		width: 100%;
	}

	&:global(.flyout-open) {
		background-color: white;
		box-shadow: -1px 0 0 0 #dcdcdc;
		transition: background-color 0.3s, box-shadow 0.3s;
	}
`;

export const Box = styled.div`
	position: relative;
	width: 42px;
	height: 42px;
`;

export const Dot = styled(User.Presence)`
	position: absolute;
	right: 2px;
	bottom: 2px;
`;

on 6.4.0, each of these has the same class generated. Astrotruf extracts each template string into a virtual ComponentName.module.css with a classname based on the component's name in code. They should all get a unique class name each... except they're not. My localIdentName config is the recommended one:

localIdentName: PROD
	? '[hash:base64]' 
	: '[path][name]__[local]--[hash:base64]',

jsg2021 avatar Oct 12 '21 17:10 jsg2021

Reverting https://github.com/webpack-contrib/css-loader/commit/c7db752fe6a9c7ff28d165fd24a37be08ef83af5 didn't fix it, but reverting https://github.com/webpack-contrib/css-loader/commit/303a3a171793cf1044c131e291f5c29f9ab86c77 did.

Would restoring the relativeMatchResource bits break the consistency SSR part?

jsg2021 avatar Oct 12 '21 17:10 jsg2021

@jsg2021 Because we can't rely on matchResource, for example when we extract something it can change module or other loader above can change context (astroturf do it, i.e. we don't know resourcePath), also we can't change something on context above, loader above should provide context for us, ideally astroturf should calculate hash for each js file (from the place where astroturf do extract), based on resourcePath and provide it to css-loader.

As you can see the original post in this issue about this. mini-css-extract-plugin create pseudo module for extracting and previously we rely matchResource, so we got different locales.

alexander-akait avatar Oct 13 '21 10:10 alexander-akait

i'm up to do whatever but what is the suggestion here? How would astroturf provide css-loader a hash? I'd love to not use matchResource at all, but that was the path we were led down for doing virtual modules.

jquense avatar Oct 13 '21 13:10 jquense

@jquense We need pass to css-loader !!css-loader!./index.css?modules-hash-salt=hash (pseudo example), is it possible?

alexander-akait avatar Oct 13 '21 13:10 alexander-akait

ah maybe, that should be fine to provide a hash in the request query...will style and extract loader pass that through when they rewrite the request?

That seems a lot better actually. css-loader would use the hash for calculating the class name hashes?

jquense avatar Oct 13 '21 14:10 jquense

will style and extract loader pass that through when they rewrite the request?

Should use, if we have !!another-loader!css-loader?modules-hash-salt=hash!another-loader!./index.css, it will work

css-loader would use the hash for calculating the class name hashes?

Yes, we need small changes here, but it is not hard to implement

alexander-akait avatar Oct 13 '21 14:10 alexander-akait

adding the query to the css-loader itself is a bit harder, we'd have to rewrite the current loader chain with the new option...i think i've done that in the past tho, just a bit messy

jquense avatar Oct 13 '21 14:10 jquense

@jquense Does astroturf have pitch phase?

alexander-akait avatar Oct 13 '21 15:10 alexander-akait

The main problem what virtual module doesn't have resourcePath, which used for locals generation and there are two possible fixed:

  1. Allow to pass hash for css-loader (i.e. you can emulate different files)
  2. Added API to loaderContent for add/renaming current resourcePath, so you can create virtual paths, like src/foo/bar/index.js.css for extracted content and css-loader will use it for generation locals

alexander-akait avatar Oct 13 '21 15:10 alexander-akait

you'd have the best idea what works. It's a bit frustrating we couldn't have a resourcePath supported out of the box but webpack for these, but i'm sure there is a good reason

jquense avatar Oct 13 '21 15:10 jquense

Let's keep open until we solve it

alexander-akait avatar Oct 13 '21 16:10 alexander-akait

@alexander-akait who needs to move first?

jquense avatar Oct 18 '21 17:10 jquense

@jquense we discussed with @sokra, we will implement this in near future

alexander-akait avatar Oct 18 '21 17:10 alexander-akait

ok i'll hang and wait for that then 👍 should i tell users to not upgrade to the latest css-loader until then?

jquense avatar Oct 18 '21 17:10 jquense

Yes, better ask to keep old version

alexander-akait avatar Oct 18 '21 17:10 alexander-akait

ok let me know if there is anything i can do to help. it is a bit tough to have people pin a minor version of css-loader, esp in frameworks.

jquense avatar Oct 18 '21 17:10 jquense

Wondering if any progress is in sight here? We're currently stuck pinning css-loader 6.3.0 within another dependency due to ^6.4.0 incompatibility with astroturf (which is very annoying with our current version of npm – need to upgrade).

evnp avatar Feb 04 '22 18:02 evnp

I would also love to not pin css-loader anymore, is there anything i can do here to move this along? I don't think this is just an astroturf issue, anything that uses matchResources (which is now documented on the doc site even) will have a problem here

jquense avatar Oct 25 '22 14:10 jquense

@jquense Sorry for delay, I lose context, pinned issue to return to this in near future, sorry again

alexander-akait avatar Oct 25 '22 14:10 alexander-akait

For quick refresh:

when css-loader hashes classnames, it only looks are resourcePath. which is "wrong" for resources imported with !=! match resource, resulting in duplicate classNames for:

'style1.css!=!button.js'
'style2.css!=!button.js'

jquense avatar Oct 25 '22 16:10 jquense

@jquense ~~Yeah, I see, looking at both side will be wrong too, because sometimes you want the reversed behaviour, why don't use ?modules=true in this case? I can improve logic and we will accept this~~

Will ?hash help here? I can implement this

alexander-akait avatar Nov 10 '22 19:11 alexander-akait

@jquense I think I found the main problem, can you try https://github.com/webpack-contrib/css-loader/pull/1480, looks like I should not concatenate resource path with resource match, instead I should replace :confused:

alexander-akait avatar Nov 10 '22 22:11 alexander-akait