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

refactor: evaluate values before extracting imports #403

Closed
wants to merge 1 commit into from
Closed

refactor: evaluate values before extracting imports #403

wants to merge 1 commit into from

Conversation

simlrh
Copy link

@simlrh simlrh commented Jan 20, 2017

What kind of change does this PR introduce?
Applies postcss-modules-values before postcss-modules-extract-imports, in order to allow code such as:

@value colors: './colors.css';
.className: { composes blue from colors; }

Did you add tests for your changes?
Yes

@codecov-io
Copy link

codecov-io commented Jan 20, 2017

Codecov Report

Merging #403 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #403      +/-   ##
==========================================
- Coverage    98.4%   98.35%   -0.06%     
==========================================
  Files           9        9              
  Lines         314      304      -10     
  Branches       71       67       -4     
==========================================
- Hits          309      299      -10     
  Misses          5        5
Impacted Files Coverage Δ
lib/processCss.js 97.91% <ø> (ø) ⬆️
lib/css-base.js 96.29% <0%> (-0.85%) ⬇️
lib/compile-exports.js 100% <0%> (ø) ⬆️
lib/loader.js 100% <0%> (ø) ⬆️

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 6da7e90...46418f9. Read the comment docs.

@michael-ciniawsky
Copy link
Member

@simlrh Coud you please close and reopen the PR to trigger the CLA Bot again it should work now 😛 . Would this be a breaking change under any circumstances?

@michael-ciniawsky
Copy link
Member

@simlrh friendly ping 😛

@simlrh simlrh closed this Apr 12, 2017
@simlrh
Copy link
Author

simlrh commented Apr 12, 2017

It's a breaking change in all circumstances, I believe!

Thought this was a different PR. I'm not sure about breaking things. Since you get different behaviour with this than before, it's possible someone is relying on the old behaviour.

@simlrh simlrh reopened this Apr 12, 2017
@jsf-clabot
Copy link

jsf-clabot commented Apr 12, 2017

CLA assistant check
All committers have signed the CLA.

@michael-ciniawsky michael-ciniawsky changed the title Evaluate values before extracting imports refactor: evaluate values before extracting imports Apr 18, 2017
@michael-ciniawsky michael-ciniawsky added this to the 1.0.0 milestone Apr 23, 2017
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 25, 2017

@simlrh @evilebottnawi @bebraw @d3viant0ne Since we intend to remove CSS Modules from css-loader enterily in the next major and refactor them into a PostCSS Plugin used in conjunction with postcss-loader instead, this either lands as minor with the slight risk of breaking things for some users or I need to close it. Could we triage the breakage risk of this again please and make a decision upon releasing as semver minor or not ?

@michael-ciniawsky michael-ciniawsky removed this from the 1.0.0 milestone Apr 25, 2017
@alexander-akait
Copy link
Member

alexander-akait commented Apr 25, 2017

/cc @michael-ciniawsky seems as breaking changes, It would be better to add a temporary option to enable this (before we drop css-modules from css-loader), but only if it frequent usage, if no maybe now be good @simlrh create branch and use own branch until we put everything in its place

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 27, 2017

Closing since we don't intend to support CSS Modules in css-loader within the next major anymore, I will consider this in cssm-loader instead. Thx 😛

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

Successfully merging this pull request may close these issues.

None yet

6 participants