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: don't handle empty @import and url() #513

Merged
merged 2 commits into from
Apr 26, 2017

Conversation

alexander-akait
Copy link
Member

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

yes

If relevant, did you update the README?

not required

Summary

Ref: #463

Other information

not required

@codecov
Copy link

codecov bot commented Apr 23, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage    98.6%   98.61%   +<.01%     
==========================================
  Files          10       10              
  Lines         359      361       +2     
  Branches       81       82       +1     
==========================================
+ Hits          354      356       +2     
  Misses          5        5
Impacted Files Coverage Δ
lib/processCss.js 98.01% <100%> (+0.02%) ⬆️

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 b90d8dd...639bef1. Read the comment docs.

@alexander-akait
Copy link
Member Author

alexander-akait commented Apr 23, 2017

/cc @michael-ciniawsky I think we don't throw error on empty url and import because we don't know why it is used. Example i can create class with empty .myclass { background: url('') } and then change value on js.

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 23, 2017

I will take a look at this soon 😛

@michael-ciniawsky michael-ciniawsky changed the title Fixed: don't handle empty import and url [#463]. fix: don't handle empty @import and url() Apr 23, 2017
@michael-ciniawsky
Copy link
Member

Fixes #463

if(!loaderUtils.isUrlRequest(url, root)) {
return url;
if(!origUrl.replace(/\s/g, '').length || !loaderUtils.isUrlRequest(url, root)) {
return origUrl;
Copy link
Member

@michael-ciniawsky michael-ciniawsky Apr 23, 2017

Choose a reason for hiding this comment

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

return url ? Otherwise the former fix (trim(" ")) would be reverted, if you return the untrimmed origUrl, maybe better just return url.trim(" ") and rm origUrl (no-var)😛

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky i think we don't change original value of url (example background: url(' ') many spaces 😄 ) should be background: url(' ') after processing, if we return trimmed, we get background: url('') output (as now). I would not call it a bug, but this is a non-standard behavior that in theory we should not do.

Copy link
Member

@michael-ciniawsky michael-ciniawsky Apr 23, 2017

Choose a reason for hiding this comment

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

What is the difference between url(' ') && url(' ') ? 🙃

Copy link
Member Author

@alexander-akait alexander-akait Apr 23, 2017

Choose a reason for hiding this comment

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

@michael-ciniawsky no difference 😄 but in theory i can have js script which get value url and check on , I do not know why i need to do this and for what, but I can do it 😄 Just think that it's worth avoiding implicit manipulation that in the future can lead to unexpected problems

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @michael-ciniawsky ping 😄

Copy link
Member

Choose a reason for hiding this comment

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

@evilebottnawi I'm not 💯 on this yet, mainly because of the code noise, even if small 😛 is justifiable, compared to what we want to achieve here. var origUrl in particular, the trim() story, Can't we solve this otherwise ? Is the distinction between url('') <=> url(' ') really really worthwhile? Besides that not handling empty @import/url() of course which needs to land :D

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky about url i think best solution if url empty (also with spaces and newlines and etc) do nothing at all, it is good practice don't change css ast, if we do not need it, but here in fact we do not need it. The same applies to import. We can never know why the user did this, but if he did it maybe he really needed it, and by changing something we can break his logic. If there are a lot of issues with this, then we can just throw warning, why warning? because it is not fatal, all works as expected 😄

Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to trim() and have the cleanest code possible + comment and wait if someone needs the \s preserved and fix it when we have a usecase. I personally doubt that someone will ever need that, but in 3+ months I look at the code and wonder what the fuss was all about 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky ok let's trim 😄

@alexander-akait
Copy link
Member Author

@michael-ciniawsky done, just see which tests changed and what the user has and what will get at the output 639bef1#diff-aba9b3cb66ffd4aba9e67a4ad4d38376

@@ -160,9 +163,9 @@ module.exports = function processCss(inputSource, inputMap, options, callback) {
mode: options.mode,
rewriteUrl: function(global, url) {
if(parserOptions.url){
url = url.trim(" ");
url = url.trim();
Copy link
Member

Choose a reason for hiding this comment

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

Indent off? 🙃 😛

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 26, 2017

just see which tests changed and what the user has and what will get at the output 639bef1#diff-aba9b3cb66ffd4aba9e67a4ad4d38376

Is there something you're worried about besides the url(' ') => url('') case? Otherwise LTGM

@alexander-akait
Copy link
Member Author

@michael-ciniawsky nothing 😄

@michael-ciniawsky michael-ciniawsky merged commit 868fc94 into webpack-contrib:master Apr 26, 2017
@alexander-akait alexander-akait deleted the issue-463 branch April 26, 2017 20:28
@michael-ciniawsky michael-ciniawsky modified the milestone: 0.29.0 May 22, 2017
eliperelman referenced this pull request in neutrinojs/neutrino Jul 10, 2018
This Pull Request updates dependency [css-loader](https://github.com/webpack-contrib/css-loader) from `^0.28.11` to `^1.0.0`



<details>
<summary>Release Notes</summary>

### [`v1.0.0`](https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md#&#8203;100httpsgithubcomwebpack-contribcss-loadercomparev02811v100-2018-07-06)
[Compare Source](webpack-contrib/css-loader@v0.28.11...v1.0.0)
##### BREAKING CHANGES

* remove `minimize` option, use [`postcss-loader`](https://github.com/postcss/postcss-loader) with [`cssnano`](https://github.com/cssnano/cssnano) or use [`optimize-cssnano-plugin`](https://github.com/intervolga/optimize-cssnano-plugin) plugin
* remove `module` option, use `modules` option instead
* remove `camelcase` option, use `camelCase` option instead
* remove `root` option, use [`postcss-loader`](https://github.com/postcss/postcss-loader) with [`postcss-url`](https://github.com/postcss/postcss-url) plugin
* remove `alias` option, use [`resolve.alias`](https://webpack.js.org/configuration/resolve/) feature or use [`postcss-loader`](https://github.com/postcss/postcss-loader) with [`postcss-url`](https://github.com/postcss/postcss-url) plugin
* update `postcss` to `6` version
* minimum require `nodejs` version is `6.9`
* minimum require `webpack` version is `4`
#### [0.28.11](webpack-contrib/css-loader@v0.28.10...v0.28.11) (2018-03-16)
##### Bug Fixes

* **lib/processCss:** don't check `mode` for `url` handling (`options.modules`) ([#&#8203;698](`https://github.com/webpack-contrib/css-loader/issues/698`)) ([c788450](webpack-contrib/css-loader@c788450))
#### [0.28.10](webpack-contrib/css-loader@v0.28.9...v0.28.10) (2018-02-22)
##### Bug Fixes

* **getLocalIdent:** add `rootContext` support (`webpack >= v4.0.0`) ([#&#8203;681](`https://github.com/webpack-contrib/css-loader/issues/681`)) ([9f876d2](webpack-contrib/css-loader@9f876d2))
#### [0.28.9](webpack-contrib/css-loader@v0.28.8...v0.28.9) (2018-01-17)
##### Bug Fixes

* ignore invalid URLs (`url()`) ([#&#8203;663](`https://github.com/webpack-contrib/css-loader/issues/663`)) ([d1d8221](webpack-contrib/css-loader@d1d8221))
#### [0.28.8](webpack-contrib/css-loader@v0.28.7...v0.28.8) (2018-01-05)
##### Bug Fixes

* **loader:** correctly check if source map is `undefined` ([#&#8203;641](`https://github.com/webpack-contrib/css-loader/issues/641`)) ([0dccfa9](webpack-contrib/css-loader@0dccfa9))
* proper URL escaping and wrapping (`url()`) ([#&#8203;627](`https://github.com/webpack-contrib/css-loader/issues/627`)) ([8897d44](webpack-contrib/css-loader@8897d44))
#### [0.28.7](webpack-contrib/css-loader@v0.28.6...v0.28.7) (2017-08-30)
##### Bug Fixes

* pass resolver to `localsLoader` (`options.alias`)  ([#&#8203;601](`https://github.com/webpack/css-loader/issues/601`)) ([8f1b57c](webpack-contrib/css-loader@8f1b57c))
#### [0.28.6](webpack-contrib/css-loader@v0.28.5...v0.28.6) (2017-08-30)
##### Bug Fixes

* add support for aliases starting with `/` (`options.alias`) ([#&#8203;597](`https://github.com/webpack/css-loader/issues/597`)) ([63567f2](webpack-contrib/css-loader@63567f2))
#### [0.28.5](webpack-contrib/css-loader@v0.28.4...v0.28.5) (2017-08-17)
##### Bug Fixes

* match mutliple dashes (`options.camelCase`) ([#&#8203;556](`https://github.com/webpack/css-loader/issues/556`)) ([1fee601](webpack-contrib/css-loader@1fee601))
* stricter `[@import]` tolerance ([#&#8203;593](`https://github.com/webpack/css-loader/issues/593`)) ([2e4ec09](webpack-contrib/css-loader@2e4ec09))
#### [0.28.4](webpack-contrib/css-loader@v0.28.3...v0.28.4) (2017-05-30)
##### Bug Fixes

* preserve leading underscore in class names ([#&#8203;543](`https://github.com/webpack/css-loader/issues/543`)) ([f6673c8](webpack-contrib/css-loader@f6673c8))
#### [0.28.3](webpack-contrib/css-loader@v0.28.2...v0.28.3) (2017-05-25)
##### Bug Fixes

* correct plugin order for CSS Modules ([#&#8203;534](`https://github.com/webpack/css-loader/issues/534`)) ([b90f492](webpack-contrib/css-loader@b90f492))
#### [0.28.2](webpack-contrib/css-loader@v0.28.1...v0.28.2) (2017-05-22)
##### Bug Fixes

* source maps path on `windows` ([#&#8203;532](`https://github.com/webpack/css-loader/issues/532`)) ([c3d0d91](webpack-contrib/css-loader@c3d0d91))
#### [0.28.1](webpack-contrib/css-loader@v0.28.0...v0.28.1) (2017-05-02)
##### Bug Fixes

* allow to specify a full hostname as a root URL ([#&#8203;521](`https://github.com/webpack/css-loader/issues/521`)) ([06d27a1](webpack-contrib/css-loader@06d27a1))
* case insensitivity of [@&#8203;import] ([#&#8203;514](`https://github.com/webpack/css-loader/issues/514`)) ([de4356b](webpack-contrib/css-loader@de4356b))
* don't handle empty [@&#8203;import] and url() ([#&#8203;513](`https://github.com/webpack/css-loader/issues/513`)) ([868fc94](webpack-contrib/css-loader@868fc94))
* imported variables are replaced in exports if followed by a comma ([#&#8203;504](`https://github.com/webpack/css-loader/issues/504`)) ([956bad7](webpack-contrib/css-loader@956bad7))
* loader now correctly handles `url` with space(s) ([#&#8203;495](`https://github.com/webpack/css-loader/issues/495`)) ([534ea55](webpack-contrib/css-loader@534ea55))
* url with a trailing space is now handled correctly ([#&#8203;494](`https://github.com/webpack/css-loader/issues/494`)) ([e1ec4f2](webpack-contrib/css-loader@e1ec4f2))
* use `btoa` instead `Buffer` ([#&#8203;501](`https://github.com/webpack/css-loader/issues/501`)) ([fbb0714](webpack-contrib/css-loader@fbb0714))
##### Performance Improvements

* generate source maps only when explicitly set ([#&#8203;478](`https://github.com/webpack/css-loader/issues/478`)) ([b8f5c8f](webpack-contrib/css-loader@b8f5c8f))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants