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

ICSS: Importing a composed class name produces wrong output #997

Open
joepie91 opened this issue Nov 23, 2019 · 20 comments
Open

ICSS: Importing a composed class name produces wrong output #997

joepie91 opened this issue Nov 23, 2019 · 20 comments

Comments

@joepie91
Copy link

  • Operating System: Linux
  • Node Version: v10.17.0
  • NPM Version: n/a, Yarn 1.15.2
  • webpack Version: 4.41.2
  • css-loader Version: 3.2.0

Expected Behavior

css-loader should produce correct output:

.outer-box__outerBox--1rUd7j4- .inner-box__innerBox--19HrVC2M.shared__box--3DFGfIs1 {
	background-color: lime;
}

Selection_407

Actual Behavior

css-loader produces incorrect output:

.outer-box__outerBox--1rUd7j4- .inner-box__innerBox--19HrVC2M shared__box--3DFGfIs1 {
	background-color: lime;
}

Selection_406

Code

See below.

How Do We Reproduce?

Full repro case here: https://git.cryto.net/joepie91/icss-loader-test

Run yarn webpack to produce output with Webpack + css-loader (incorrect output), run yarn browserify-extract to produce output with Browserify + icssify (correct output).

Both output to the same files, so you can switch between them. Open test/index.html in a browser to see the result.

The bug is caused by the import handling using the JS/HTML-formatted (space-delimited) export, rather than a CSS-formatted (dot-delimited) export.

This means that when using a composed identifier (which consists of two space-delimited class names) as an imported identifier, the resulting selector rule will be broken (see code snippets above).

This can be fixed by replacing spaces with dots in the case of ICSS imports.

@alexander-akait
Copy link
Member

Thanks for issue i will look on this in near future

@yakato
Copy link

yakato commented Dec 3, 2019

I have the same. After update all new react-scripts dependencies, also css-loader updated. With the latest version my all code based on CSS-Modules composes is broken.

@alexander-akait
Copy link
Member

@yakato please create reproducible test repo

@apexskier
Copy link

Might be related to #561, which has a minimal reproduction.

@joepie91
Copy link
Author

@apexskier That's a different issue, about imports not working at all. That has since been fixed, as far as I can tell.

The remaining issue is that specifically composed class names are not handled correctly, for which I've provided a reproduction in the initial report above.

@OHUSAR

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@OHUSAR

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@OHUSAR

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@OHUSAR

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@alexander-akait
Copy link
Member

Sorry for delay, WIP on the issue, maybe we need improve our architecture, also it is allow to validate existing names and throw an error on the build step

@alexander-akait
Copy link
Member

Here big problem 😞 @joepie91 You example is broken too.

Example:

inner-box.css

@value shadow: 10px 5px 5px red;

.innerBox {
	composes: box from "./shared.css";
	width: 64px;
	height: 64px;
	background-color: red;
}

outer-box.css

:import("./inner-box.css") {
	importedInnerBox: innerBox;
}

@value shadow from './inner-box.css';

.outerBox {
	composes: box from "./shared.css";
	width: 100px;
	height: 100px;
	background-color: blue;
	box-shadow: shadow;
}

.outerBox .importedInnerBox {
	background-color: lime;
}

And look at bundle:

"shadow":"10px.5px.5px.red"

We replace spaces on ., but it is invalid.

Don't know how we can solve it, because we don't know this values on the build step and don't know context, what we know? Only value.

@alexander-akait
Copy link
Member

I postpone it for v5, now it is the limitation

@alexander-akait
Copy link
Member

Here other problem you can import selector from other file and it is not hashed, using :local(selector) { } provide invalid export. So supporting selector is very limited now, you can use it only in the current file, importing is not working as expected

@joepie91
Copy link
Author

Don't know how we can solve it, because we don't know this values on the build step and don't know context, what we know? Only value.

This seems like a bug in icss-utils then, if it doesn't permit inserting a different value depending on the context. I've filed a bug: css-modules/icss-utils#70

Here other problem you can import selector from other file and it is not hashed, using :local(selector) { } provide invalid export. So supporting selector is very limited now, you can use it only in the current file, importing is not working as expected

I'm not sure what you mean with this. Can you give an example?

@abhijithqb
Copy link

Any workaround for this issue?

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

6 participants