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

v4 breaks css-module-loader #1134

Closed
jquense opened this issue Jul 27, 2020 · 13 comments · Fixed by #1150
Closed

v4 breaks css-module-loader #1134

jquense opened this issue Jul 27, 2020 · 13 comments · Fixed by #1150

Comments

@jquense
Copy link
Contributor

jquense commented Jul 27, 2020

Hey @evilebottnawi the change that only adds the ICSS plugins breaks the strategy used by https://github.com/jquense/css-module-loader to convert modules into :import and :export rules which depended on the (admittedly weird) behavior of css-loader always including the icss plugin.

It's not enough to turn on modules tho b/c css-loader will then rehash everything. For this sort of thing to work we need an api to handle downstream import/exports without css-loader trying to process css-modules. This is also true for the new auto default. it's not safe to assume any .module.css file should be processed by css-loader when there are loaders ahead of it.

Would love if we could come up with a first class API for this!

@alexander-akait
Copy link
Member

I don't understand the problem, please clarify

This is also true for the new auto default. it's not safe to assume any .module.css file should be processed by css-loader when there are loaders ahead of it.

It is breaking change, you can disable this behavior

@jquense
Copy link
Contributor Author

jquense commented Jul 27, 2020

css-module-loader converts css to ICSS :export and :import rules, css-loader would process those without also processing @value and classes to create the JS exports. Now it doesn't because the ICSS plugin is only included when modules is true.

css-module-loader is incompatible with css-loader with modules turned on because both loaders would process the file for css modules when it is already done

At the moment there is no way to have a loader above css-loader process css-modules, in v3 there was a hacky way to do it.

@alexander-akait
Copy link
Member

@jquense Maybe you need icss: boolean (false by default), if you specify icss: true, it is unable only icssParser (no css modules plugins), if you set modules: true we enable icss: true by default. Is this what you are looking for? Should be very easy to implement

@jquense
Copy link
Contributor Author

jquense commented Jul 27, 2020

if you set modules: true we enable icss: true by default.

I think that would work, let me get a PR together if that sounds reasonable

@alexander-akait
Copy link
Member

@jquense Go ahead, it is allow fix hacky setup for css-module-loader

@alexander-akait
Copy link
Member

Fixed, now you can add { modules: { auto: false }, icss: true } and all should be work fine, feel free to feelback

@jquense
Copy link
Contributor Author

jquense commented Jul 30, 2020

Realized that the exportOnlyLocals being moved makes SSR hard/impossible for this case as well. Even with the icss options we an't turn on just exports without also including the styles anymore

@alexander-akait
Copy link
Member

hm, interesting use case

@jquense
Copy link
Contributor Author

jquense commented Jul 30, 2020

Not sure what the best approach would be for options here that doesn't break the new behavior? Maybe making icss accept a 'onlyLocals'` value? It duplicates the option, but would work i guess

@alexander-akait
Copy link
Member

alexander-akait commented Jul 30, 2020

I think ideally we should have:

{
  modules: {
    // Maybe best name
    type: 'icss' | 'full'
  }
}

default is full, so you can set modules: { type: 'icss', exportOnlyLocals: true }, It is why I wanted to move this option inside modules.

Unfortunately, we cannot remove this option now, but I propose to do deprecation until many people have used it.

Also you can use other options like namedExport, generate other locals names and etc

@jquense
Copy link
Contributor Author

jquense commented Jul 30, 2020

yeah that makes sense, should have thought through this a bit more first sorry. I'll send a follow up. We can likely just remove icss from the readme, and add a deprecation warning. very unlikely anyone is using this yet

@alexander-akait
Copy link
Member

We can likely just remove icss from the readme, and add a deprecation warning. very unlikely anyone is using this yet

Yes, let's do it

@alexander-akait
Copy link
Member

WIP, other developers have problems with it too

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