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: do not export duplicate keys #420

Merged

Conversation

joscha
Copy link
Contributor

@joscha joscha commented Feb 14, 2017

What kind of change does this PR introduce?

Bugfix, it makes sure that keys that are the same in different case styles (e.g. dashed, camelCase, dashedCamelCase) are not added to the resulting output when ?camelCase is enabled.

E.g. a class name .bla would be bla as a default, but also bla in camelCase whereas .my-bla would yield my-bla and myBla. Currently the constructed document for the locals contains two entries, like this:

{
  "bla": "_1L-rnCOXCE_7H94L5XT4uB",
  "bla": "_1L-rnCOXCE_7H94L5XT4uB",
}

if camelCase is enabled.

Did you add tests for your changes?

No, because the test helper uses JSON deserialization, which removes the dupe.

Summary

When using CSS modules with Typescript for example, you encounter something like this:

ERROR in /Users/joscha/dev/web-ui/src/ui/button/button.css.ts
(11,2): error TS2300: Duplicate identifier '"defaultFontStack"'.

ERROR in /Users/joscha/dev/web-ui/src/ui/button/button.css.ts
(13,2): error TS2300: Duplicate identifier '"button"'.

Also, the resulting output in the bundle is 50% bigger than it has to be if only class names are used that are className === camelCase(className).

Does this PR introduce a breaking change?
No

@codecov
Copy link

codecov bot commented Feb 14, 2017

Codecov Report

Merging #420 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   98.32%   98.36%   +0.03%     
==========================================
  Files           9        9              
  Lines         299      306       +7     
  Branches       67       69       +2     
==========================================
+ Hits          294      301       +7     
  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 e02e7a2...be83084. Read the comment docs.

@joscha
Copy link
Contributor Author

joscha commented Feb 14, 2017

can't find the place to sign the CLA

@joscha joscha closed this Feb 14, 2017
@joscha joscha reopened this Feb 14, 2017
@joscha
Copy link
Contributor Author

joscha commented Feb 14, 2017

still no email from the CLA bot :(

@michael-ciniawsky michael-ciniawsky added this to Bug in Dashboard Mar 6, 2017
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

👍

@michael-ciniawsky
Copy link
Member

@joscha Please close and reopen the PR once again for the CLA, it should work now :)

@bebraw
Copy link
Contributor

bebraw commented Mar 6, 2017

I think you have to close/open the PR again as the CLA bot was fixed about ten days ago.

@jsf-clabot
Copy link

jsf-clabot commented Mar 6, 2017

CLA assistant check
All committers have signed the CLA.

@joscha
Copy link
Contributor Author

joscha commented Mar 6, 2017

@michael-ciniawsky @bebraw done

Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

You can nest two ternary operators for better readability and the same end result

@joscha
Copy link
Contributor Author

joscha commented Mar 7, 2017

@d3viant0ne I tried keeping the diff as small as possible - made some changes so it is more readable now, please take another look.

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

👍 @d3viant0ne @bebraw Please Review :)

Copy link
Contributor

@bebraw bebraw left a comment

Choose a reason for hiding this comment

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

This needs tests so we don't break it again.

@joscha
Copy link
Contributor Author

joscha commented Mar 7, 2017

@bebraw sure, I will need to change the current tooling a tiny bit for that because right now it is all based on JSON.parse which removes the duplication before we can test it. If that's okay I am happy to add a test for it.

@michael-ciniawsky
Copy link
Member

@joscha From my side, just move on :)

@bebraw
Copy link
Contributor

bebraw commented Mar 8, 2017

@joscha Ok, cool. Safer with tests. 👍

@joscha joscha force-pushed the joscha/do-not-export-duplicate-keys branch 2 times, most recently from 614b66e to 5229a89 Compare March 8, 2017 09:59
@joscha
Copy link
Contributor Author

joscha commented Mar 8, 2017

@bebraw @michael-ciniawsky I added two tests however, there is something really strange happening with the README.md - somehow the line endings get converted and any git config core.autocrlf true, etc. couldn't change it so the README.md doesn't show as changed. Do you have any idea how to fix this, as currently the diff contains the line ending changes.

EDIT: seems to be fixed by #416

@joscha joscha force-pushed the joscha/do-not-export-duplicate-keys branch from 5229a89 to efe9171 Compare March 8, 2017 10:30
@joscha
Copy link
Contributor Author

joscha commented Mar 8, 2017

The line endings in README seem to be fixed by #416 - for now you can just squash my changes.

@joscha joscha force-pushed the joscha/do-not-export-duplicate-keys branch from efe9171 to be83084 Compare March 8, 2017 10:46
@joscha
Copy link
Contributor Author

joscha commented Mar 8, 2017

I squashed all commits into one, so should be good to go now 👍

@bebraw
Copy link
Contributor

bebraw commented Mar 8, 2017

Nice work. I'll let either Michael or Joshua to merge and publish. 👍

@joshwiens joshwiens merged commit 7dfedc7 into webpack-contrib:master Mar 8, 2017
@joscha joscha deleted the joscha/do-not-export-duplicate-keys branch March 8, 2017 13:53
joshwiens pushed a commit that referenced this pull request Mar 8, 2017
joshwiens pushed a commit that referenced this pull request Mar 9, 2017
@michael-ciniawsky michael-ciniawsky removed this from Bug in Dashboard Mar 21, 2017
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

5 participants