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

Only load cssnano if we're going to use it. #472

Merged
merged 1 commit into from
Mar 30, 2017
Merged

Only load cssnano if we're going to use it. #472

merged 1 commit into from
Mar 30, 2017

Conversation

jwalton
Copy link
Contributor

@jwalton jwalton commented Mar 29, 2017

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
N/A

If relevant, did you update the README?
N/A

Summary

css-loader relies on cssnano, which relies on postcss-filter-plugins, which relies on uniqid, which relies on macaddress.

macaddress has a global leak in v0.2.8. They accidentally create a global variable named addresses. This is a problem, because if you use this pacakge, and you supply --check-leaks to mocha, mocha will spaz out about the fact that there's this extra global variable.

The good news is, this is fixed in macaddress#master. The bad news is, they haven't published the fix for 9 months.

uniqid relies on macaddress (and, obnoxiously, calls it at top-level-scope), so I'd like to harass them into switching from macaddress to some other package, but they were made aware of this issue six months ago, and they still haven't done anything.

I've raised an issue with postcss-filter-plugins, but they had a PR that would have fixed this and the closed it without merging it because they're going to deprecate postcss-filter-plugins and remove it from cssnano entirely.

So, this is a slightly awful fix which makes it so we don't even require cssnano in css-loader unless we plan to do some minimization. Since that's the default, it should fix this problem for most people. Note that node will cache the required cssnano, so if you are using minimization, this won't cause any noticeable performance problems (and I suppose it will very marginally speed things up for people who aren't doing minimization).

Does this PR introduce a breaking change?
No.

@jsf-clabot
Copy link

jsf-clabot commented Mar 29, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Mar 29, 2017

Codecov Report

Merging #472 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #472   +/-   ##
======================================
  Coverage    98.3%   98.3%           
======================================
  Files          10      10           
  Lines         354     354           
  Branches       79      79           
======================================
  Hits          348     348           
  Misses          6       6
Impacted Files Coverage Δ
lib/processCss.js 97.91% <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 c8db489...95abb5e. Read the comment docs.

@michael-ciniawsky
Copy link
Member

@jwalton Well, in terms of css-loader the logic will definitely change in the future and this change is fine for now 😛, for all the other concerns regarding dependency madness and subsequent incompabilities + sloppy maintenance in nearly all of them I keep it by just saying that without any further investigation, it starts pissing me off by just hearing it :D

@michael-ciniawsky
Copy link
Member

cc @bebraw @d3viant0ne semver patch please 😛

@michael-ciniawsky michael-ciniawsky merged commit 76eb7f6 into webpack-contrib:master Mar 30, 2017
@jwalton
Copy link
Contributor Author

jwalton commented Mar 30, 2017

@michael-ciniawsky You are a scholar and a gentleman, sir. :)

@michael-ciniawsky
Copy link
Member

@jwalton Released in v0.28.0

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.

3 participants