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

fix: allow module config while using autoModules (#281) #292

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

katywings
Copy link
Contributor

This includes a fix for #281

Comparison of the options logic (IS / SHOULD columns):

Case opt autoModules opt modules -- will be module? IS SHOULD
1 styles.css false false
2 ... false true yes yes
3 ... true false
4 ... true true yes yes
5 ... false {} yes yes
6 ... true {} yes
7 styles.module.css false false
8 ... false true yes yes
9 ... true false yes yes
10 ... true true yes yes
11 ... false {} yes yes
12 ... true {} yes yes

I hope I didnt make any errors in the table LMAO

@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #292 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
+ Coverage   92.47%   92.50%   +0.02%     
==========================================
  Files          10       10              
  Lines         319      320       +1     
  Branches      112      113       +1     
==========================================
+ Hits          295      296       +1     
  Misses         23       23              
  Partials        1        1              
Impacted Files Coverage Δ
src/index.js 98.97% <ø> (ø)
src/postcss-loader.js 92.00% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9489eca...5e5c5b7. Read the comment docs.

@intrnl
Copy link

intrnl commented Jun 27, 2020

this vs #291 ?

@katywings
Copy link
Contributor Author

katywings commented Jun 27, 2020

The difference between this two solutions is that #291 requires the consumer to explicitly define autoModules = true if they seek to use autoModules and a custom modules config together. While the fix in this pr doesnt need any consumer changes.

Imho with #291 it would be a little bit weird from a users standpoint:

  • autoModules is by default = true
  • user later also sets modules: { ... } and expects autoModules to still be true
  • but it isnt and so the user then has to set autoModules explicitly to true

@intrnl
Copy link

intrnl commented Jun 27, 2020

Ah, understood

Copy link

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change a lot because it changes the modules flag to be more intended for specifying the naming scheme that is independent of whether autoModules is used or everything should be a module. In my opinion that's the behavior a user would expect without looking at docs 👍

@egoist egoist merged commit f7f3fdb into egoist:master Jul 14, 2020
@github-actions
Copy link

🎉 This PR is included in version 3.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@YIZHUANG
Copy link

@katywings So if i wanna use Css modules and global css together i will have to do:

    postcss({
      modules: true,
      extract: true,
    }),

Doesn't work for me though, pure css files still get transformed into css module

@katywings
Copy link
Contributor Author

@YIZHUANG: Dont set modules: true. If you set modules to true then all files will be interpreted as modules. That was the case before and after this pr, this pr didnt change the behaviour of modules: true because that would have been a breaking change. Please look at the pr description, there is a comparison table with "IS" "SHOULD".

If you think that modules: true should not result in all css files being interpreted as modules, then I recommend you to open a new issue with a change request :).

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

Successfully merging this pull request may close these issues.

6 participants