feat: allow removal of original class name #445

joscha · 2017-03-09T11:33:53Z

What kind of change does this PR introduce?
feature/performance

Did you add tests for your changes?
yes

If relevant, did you update the README?
yes

Summary

Does this PR introduce a breaking change?
No

codecov · 2017-03-09T11:35:51Z

Codecov Report

Merging #445 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
+ Coverage    98.4%   98.41%   +<.01%     
==========================================
  Files           9        9              
  Lines         314      315       +1     
  Branches       71       72       +1     
==========================================
+ Hits          309      310       +1     
  Misses          5        5

Impacted Files	Coverage Δ
lib/compile-exports.js	`100% <100%> (ø)`	✅

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...8ce02a0. Read the comment docs.

michael-ciniawsky

👍 Just a few doc related nits. Thx very much for this PR :)

michael-ciniawsky · 2017-03-09T13:37:01Z

README.md

@@ -394,6 +394,15 @@ By default, the exported JSON keys mirror the class names. If you want to cameli
 import { className } from 'file.css';
 ```

+#### Possible options


- #### Possible Options

done, 9574dc0

michael-ciniawsky · 2017-03-09T13:38:10Z

README.md

@@ -394,6 +394,15 @@ By default, the exported JSON keys mirror the class names. If you want to cameli
 import { className } from 'file.css';
 ```

+#### Possible options
+
+|Option|Behaviour|


Move the table directly under the camelCase header please

Behaviour => Description for consistency

done, 9574dc0 and 8ce02a0

ekulabuhov · 2017-03-09T18:37:38Z

package.json

@@ -1,12 +1,16 @@
 {
  "name": "css-loader",
-  "version": "0.26.2",
+  "version": "0.26.4",


This is done automatically. No need to change this file.

@ekulabuhov Good catch 💯 I totally missed it.

In a short while, if seen this a few times now, maybe we should update the PR_TEMPLATE to explicitly mention that npm version is always handled/done by us? cc @bebraw @d3viant0ne

Yeah, open an issue at webpack-defaults so it gets sorted once and for all.

ekulabuhov · 2017-03-09T19:29:49Z

lib/loader.js

-					if(p.indexOf("../") !== 0)
-						p = "./" + p;
-					return "/" + p;
+					return source.split("!").pop();


Is this related?

ekulabuhov

It seems like this PR needs a rebase.

joshwiens · 2017-03-09T20:57:20Z

And please note, it need a rebase not a merge update or conflict resolution via github.

If you aren't comfortable with the rebase comment, give me write access to your fork and I can either do it or walk you through the process

joscha · 2017-03-09T22:35:18Z

And please note, it need a rebase not a merge update or conflict resolution via github.

rebased onto master, please have another look @d3viant0ne @ekulabuhov

ekulabuhov · 2017-03-10T00:16:50Z

👍 Great work!

Would it make more sense to opt in into generating original class names? Something like:
{ camelCase: 'withOriginal' }. I don't think many people depended or actually knew about this buggy behaviour. And in version 1.0 we're going to break it once again for people that switch to only or dashesOnly right now. Any opinions?

joscha · 2017-03-10T00:44:42Z

I don't think many people depended or actually knew about this buggy behaviour.

I know at least two companies that depend on this "feature" 💃 💃 💃 :)

@ekulabuhov I'd really like to get this fix into pre-1.0.0 and the only way to not break existing code is to make the feature opt-out instead of opt-in. I am happy to pick the PR to any 1.0.0 feature branch and make it opt-in to keep the original name, if there is one?

joshwiens · 2017-03-10T03:25:46Z

@joscha - Yeah, this should go in before the next major. Backwards compatible and beneficial to both major versions.

joscha · 2017-03-10T03:29:53Z

@joscha - Yeah, this should go in before the next major. Backwards compatible and beneficial to both major versions.

@d3viant0ne agreed, so I think the current PR is gtg 🏃

joshwiens · 2017-03-10T03:33:49Z

I'll merge this as soon as I finish up with a few other reviews.

joscha · 2017-03-10T07:22:00Z

I made a mistake on this - due to the change from #420 simple classes or ones that were already in camelCase are not exported any more because they are thought to be already on the object. This only happens when one of the new options is enabled. Will write a fix and send in a PR.

joscha · 2017-03-10T07:44:36Z

ever so sorry, the test cases were not wide enough and it wasn't picked up in the review - the fix is here: #448

joscha mentioned this pull request Mar 9, 2017

Add support for removing original classnames when using camelCase=true/'dashes' #440

Closed

michael-ciniawsky requested changes Mar 9, 2017

View reviewed changes

michael-ciniawsky added modules labels Mar 9, 2017

michael-ciniawsky self-assigned this Mar 9, 2017

michael-ciniawsky requested review from bebraw, joshwiens and ekulabuhov March 9, 2017 13:42

michael-ciniawsky added the pr: review label Mar 9, 2017

bebraw approved these changes Mar 9, 2017

View reviewed changes

joshwiens force-pushed the master branch from c372543 to 6da7e90 Compare March 9, 2017 16:15

ekulabuhov reviewed Mar 9, 2017

View reviewed changes

lib/loader.js Outdated

if(p.indexOf("../") !== 0)

p = "./" + p;

return "/" + p;

return source.split("!").pop();

Copy link

Member

ekulabuhov Mar 9, 2017

Choose a reason for hiding this comment

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

Is this related?

ekulabuhov suggested changes Mar 9, 2017

View reviewed changes

joscha and others added 3 commits March 10, 2017 09:32

feat: allow removal of original class name

4381a97

add comment for 1.0.0

ebc0e7c

docs: move table

9574dc0

joscha force-pushed the joscha/remove-original-class-name branch from 5679c8c to 9574dc0 Compare March 9, 2017 22:33

docs: consistency

8ce02a0

michael-ciniawsky approved these changes Mar 10, 2017

View reviewed changes

michael-ciniawsky added pr: accepted and removed pr: docs labels Mar 10, 2017

joshwiens approved these changes Mar 10, 2017

View reviewed changes

joshwiens merged commit 3f78361 into webpack-contrib:master Mar 10, 2017

joscha deleted the joscha/remove-original-class-name branch March 10, 2017 05:33

joscha mentioned this pull request Mar 10, 2017

fix: incorrect handling of non-mutated class names #448

Merged

evisong mentioned this pull request Mar 14, 2017

Support camelCase=only|dashesOnly css-modules/css-modules-require-hook#89

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow removal of original class name #445

feat: allow removal of original class name #445

joscha commented Mar 9, 2017 •

edited

Loading

codecov bot commented Mar 9, 2017 •

edited

Loading

michael-ciniawsky left a comment

michael-ciniawsky Mar 9, 2017

joscha Mar 9, 2017

michael-ciniawsky Mar 9, 2017

joscha Mar 9, 2017 •

edited

Loading

ekulabuhov Mar 9, 2017

michael-ciniawsky Mar 9, 2017 •

edited

Loading

bebraw Mar 9, 2017

ekulabuhov Mar 9, 2017

ekulabuhov left a comment

joshwiens commented Mar 9, 2017

joscha commented Mar 9, 2017

ekulabuhov commented Mar 10, 2017

joscha commented Mar 10, 2017

joshwiens commented Mar 10, 2017

joscha commented Mar 10, 2017

joshwiens commented Mar 10, 2017

joscha commented Mar 10, 2017

joscha commented Mar 10, 2017

feat: allow removal of original class name #445

feat: allow removal of original class name #445

Conversation

joscha commented Mar 9, 2017 • edited Loading

codecov bot commented Mar 9, 2017 • edited Loading

Codecov Report

michael-ciniawsky left a comment

Choose a reason for hiding this comment

michael-ciniawsky Mar 9, 2017

Choose a reason for hiding this comment

joscha Mar 9, 2017

Choose a reason for hiding this comment

michael-ciniawsky Mar 9, 2017

Choose a reason for hiding this comment

joscha Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

ekulabuhov Mar 9, 2017

Choose a reason for hiding this comment

michael-ciniawsky Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

bebraw Mar 9, 2017

Choose a reason for hiding this comment

ekulabuhov Mar 9, 2017

Choose a reason for hiding this comment

ekulabuhov left a comment

Choose a reason for hiding this comment

joshwiens commented Mar 9, 2017

joscha commented Mar 9, 2017

ekulabuhov commented Mar 10, 2017

joscha commented Mar 10, 2017

joshwiens commented Mar 10, 2017

joscha commented Mar 10, 2017

joshwiens commented Mar 10, 2017

joscha commented Mar 10, 2017

joscha commented Mar 10, 2017

joscha commented Mar 9, 2017 •

edited

Loading

codecov bot commented Mar 9, 2017 •

edited

Loading

joscha Mar 9, 2017 •

edited

Loading

michael-ciniawsky Mar 9, 2017 •

edited

Loading