fix: source maps path on `windows` #532

alexander-akait · 2017-05-11T20:25:29Z

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?

yes

If relevant, did you update the README?

Summary

#529

Does this PR introduce a breaking change?

No.

Other information

We need more tests and good review on this.

codecov · 2017-05-11T20:28:01Z

Codecov Report

Merging #532 into master will increase coverage by 0.29%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
+ Coverage   98.36%   98.65%   +0.29%     
==========================================
  Files          10       10              
  Lines         366      371       +5     
  Branches       87       89       +2     
==========================================
+ Hits          360      366       +6     
+ Misses          6        5       -1

Impacted Files	Coverage Δ
lib/processCss.js	`98.03% <0%> (ø)`	⬆️
lib/loader.js	`100% <100%> (+1.38%)`	⬆️

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 fe4cf7a...92fc8d9. Read the comment docs.

michael-ciniawsky

Still node >= v0.12 in theory

michael-ciniawsky · 2017-05-11T20:33:38Z

lib/loader.js

+			}
+
+			if (map.sources) {
+				map.sources = map.sources.map((source) => source.replace(/\\/g, '/'));


.map((source) => ...) => .map(function(source) { return ... })

@michael-ciniawsky oh yep, i don't support old node.js in my project 😄

Neither do I 😛 and I'm really in favour make the switch as quickly as possible (node < 4.0.0 is dead), but if we 'break' things all kind of conservatives will come out of their rat holes and complain like crazy about it :D 'breaks app IN PRODUCTION' 'USE SEMVER!!!` etc...

michael-ciniawsky · 2017-05-12T02:24:33Z

https://github.com/postcss/postcss/blob/master/lib/map-generator.es6#L188-L189

But it seems to work on the relative file path (L187) only atm and maybe unrelated 😛

alexander-akait · 2017-05-12T15:29:19Z

/cc @michael-ciniawsky

michael-ciniawsky · 2017-05-12T15:31:02Z

lib/loader.js

@@ -30,8 +39,8 @@ module.exports = function(content, map) {

 	processCss(content, map, {
 		mode: moduleMode ? "local" : "global",
-		from: loaderUtils.getRemainingRequest(this),
-		to: loaderUtils.getCurrentRequest(this),
+		from: loaderUtils.getRemainingRequest(this).split("!").pop(),


What is the output of loaderUtils.getRemainingRequest(this).split("!").pop()? If I remember right it should be the absolute file path and therefore we could replaced it with this.resourcePath instead?

@michael-ciniawsky we can have remainingRequest https://github.com/webpack/loader-utils/blob/master/lib/getRemainingRequest.js#L4

Best get file from getRemainingRequest it is more safe

kk also not really related to this PR, just pop into my eyes

Look into resourcePath, yup. I can't remember the exact API, though. Maybe this needs some guidelines.

@bebraw why? what is bad in use loaderUtils.getRemainingRequest(this).split("!").pop(), also #532 (comment)

@evilebottnawi I'm not entirely sure. At least test-wise the current implementation passes. It's fine to refactor later. 👍

michael-ciniawsky · 2017-05-12T15:31:46Z

lib/loader.js

-		from: loaderUtils.getRemainingRequest(this),
-		to: loaderUtils.getCurrentRequest(this),
+		from: loaderUtils.getRemainingRequest(this).split("!").pop(),
+		to: loaderUtils.getCurrentRequest(this).split("!").pop(),


Should be equal to this.resourcePath aswell

michael-ciniawsky

Rubberstamp LTGM, I have no windows machine to test this, but looks 👍 land it if you think it's good to go

alexander-akait · 2017-05-12T15:50:51Z

@michael-ciniawsky if we enable appveyor i can rewrite all tests for compatibility with windows (now tests written only for linux based os)

michael-ciniawsky · 2017-05-12T15:51:29Z

lib/loader.js

+
+			if (map.sources) {
+				map.sources = map.sources.map(function (source) {
+					return source.replace(/\\/g, '/');


Test with path.sep? But I think it's unnecessary...

@michael-ciniawsky yep, we should already changed \ to /

@michael-ciniawsky This is a source map check. Can we assume forward slash there due to browser environment?

michael-ciniawsky · 2017-05-12T15:54:05Z

Yep appveyor would be a good idea, want is need to enable it besides appveyor.yml ? For Org access ping bebraw or d3viant0ne 😛

bebraw · 2017-05-13T06:09:34Z

@d3viant0ne Yeah, fair point. Better get Tobias' approval first. 👍

michael-ciniawsky · 2017-05-21T23:10:41Z

@evilebottnawi @d3viant0ne @bebraw I think we should merge this meanwhile, despite the fact that current implementation will hopefully vanish soon 😛 , it's definitely a bug in the current css-loader

joshwiens · 2017-05-21T23:54:43Z

@evilebottnawi Open an issue in defaults for appveyor and we can get started on a org wide config as a part of defaults.

joshwiens · 2017-05-21T23:57:06Z

As far as this particular PR goes, there are a few open review comments. Patches are fine for the current major version so @bebraw @evilebottnawi @michael-ciniawsky if everyone is satisfied with this, throw a thumbs up on the comment and we'll get this landed.

michael-ciniawsky · 2017-05-22T00:21:27Z

AppVeyor Tracking Issue #54

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#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`) ([#698](`webpack-contrib/css-loader#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`) ([#681](`webpack-contrib/css-loader#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()`) ([#663](`webpack-contrib/css-loader#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` ([#641](`webpack-contrib/css-loader#641)) ([0dccfa9](webpack-contrib/css-loader@0dccfa9)) * proper URL escaping and wrapping (`url()`) ([#627](`webpack-contrib/css-loader#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`) ([#601](`webpack-contrib/css-loader#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`) ([#597](`webpack-contrib/css-loader#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`) ([#556](`webpack-contrib/css-loader#556)) ([1fee601](webpack-contrib/css-loader@1fee601)) * stricter `[@import]` tolerance ([#593](`webpack-contrib/css-loader#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 ([#543](`webpack-contrib/css-loader#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 ([#534](`webpack-contrib/css-loader#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` ([#532](`webpack-contrib/css-loader#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 ([#521](`webpack-contrib/css-loader#521)) ([06d27a1](webpack-contrib/css-loader@06d27a1)) * case insensitivity of [@import] ([#514](`webpack-contrib/css-loader#514)) ([de4356b](webpack-contrib/css-loader@de4356b)) * don't handle empty [@import] and url() ([#513](`webpack-contrib/css-loader#513)) ([868fc94](webpack-contrib/css-loader@868fc94)) * imported variables are replaced in exports if followed by a comma ([#504](`webpack-contrib/css-loader#504)) ([956bad7](webpack-contrib/css-loader@956bad7)) * loader now correctly handles `url` with space(s) ([#495](`webpack-contrib/css-loader#495)) ([534ea55](webpack-contrib/css-loader@534ea55)) * url with a trailing space is now handled correctly ([#494](`webpack-contrib/css-loader#494)) ([e1ec4f2](webpack-contrib/css-loader@e1ec4f2)) * use `btoa` instead `Buffer` ([#501](`webpack-contrib/css-loader#501)) ([fbb0714](webpack-contrib/css-loader@fbb0714)) ##### Performance Improvements * generate source maps only when explicitly set ([#478](`webpack-contrib/css-loader#478)) ([b8f5c8f](webpack-contrib/css-loader@b8f5c8f)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).

alexander-akait added flag: Needs Tests pr_action: Blocked pr_action: Review type: Bug labels May 11, 2017

michael-ciniawsky reviewed May 11, 2017

View reviewed changes

michael-ciniawsky assigned michael-ciniawsky and alexander-akait May 11, 2017

michael-ciniawsky added this to the 0.29.0 milestone May 11, 2017

michael-ciniawsky changed the title ~~fix: source maps path on windows.~~ fix: source maps path on windows May 11, 2017

This was referenced May 11, 2017

[2.0.3] source map urls are a off (map.sources) webpack-contrib/postcss-loader#224

Closed

includePaths option prepends sass folder path to file locations webpack-contrib/sass-loader#425

Closed

alexander-akait force-pushed the issue-529 branch 4 times, most recently from 1863a86 to 86332ba Compare May 12, 2017 13:29

fix: source maps path on windows.

3608136

alexander-akait force-pushed the issue-529 branch from 86332ba to 3608136 Compare May 12, 2017 15:09

alexander-akait mentioned this pull request May 12, 2017

source-map resource urls coming out strange webpack-contrib/style-loader#231

Closed

fix: source maps path on windows.

92fc8d9

alexander-akait force-pushed the issue-529 branch from 73d9876 to 92fc8d9 Compare May 12, 2017 15:26

alexander-akait removed flag: Needs Tests pr_action: Blocked labels May 12, 2017

michael-ciniawsky reviewed May 12, 2017

View reviewed changes

michael-ciniawsky approved these changes May 12, 2017

View reviewed changes

michael-ciniawsky reviewed May 12, 2017

View reviewed changes

joshwiens approved these changes May 13, 2017

View reviewed changes

alexander-akait requested a review from sokra May 13, 2017 11:41

michael-ciniawsky mentioned this pull request May 21, 2017

sass-loader creating broken paths in sourcemap webpack-contrib/sass-loader#460

Closed

michael-ciniawsky added this to Bug in Dashboard May 21, 2017

michael-ciniawsky merged commit c3d0d91 into master May 22, 2017

michael-ciniawsky deleted the issue-529 branch May 22, 2017 00:21

michael-ciniawsky removed the pr_action: Merge ready label May 22, 2017

michael-ciniawsky removed this from Bug in Dashboard May 22, 2017

michael-ciniawsky modified the milestone: 0.29.0 May 22, 2017

michael-ciniawsky mentioned this pull request May 22, 2017

source-map resource urls coming out strange #529

Closed

snyk-bot mentioned this pull request Aug 16, 2019

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 DriveClutch/sellpy-dockerfiles#4

Closed

snyk-bot mentioned this pull request Jan 8, 2020

[Snyk] Upgrade css-loader from 0.17.0 to 0.28.11 timscharfenort/kibana#3

Open

This was referenced Jan 22, 2020

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 anyulled/Learn-Redux-Starter-Files#1

Open

[Snyk] Upgrade css-loader from 0.24.0 to 0.28.11 anyulled/react-skeleton#2

Closed

This was referenced Feb 23, 2020

[Snyk] Upgrade css-loader from 0.26.4 to 0.28.11 saurabharch/stitch-examples#2

Open

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 saurabharch/guesstimate-app#3

Open

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 saurabharch/CodeGenius#2

Open

snyk-bot mentioned this pull request Mar 7, 2020

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 yowainwright/react-workshop#4

Closed

This was referenced Apr 2, 2020

[Snyk] Upgrade css-loader from 0.26.4 to 0.28.11 fiverr/rollout_dashboard#48

Open

[Snyk] Upgrade css-loader from 0.19.0 to 0.28.11 rangle/react-day-picker#2

Open

snyk-bot mentioned this pull request Apr 24, 2020

[Snyk] Upgrade css-loader from 0.16.0 to 0.28.11 walmartlabs/bolt-standard-flux#3

Open

This was referenced May 4, 2020

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 Barnstorm-Online/votetracker#1

Open

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 React-Junkie/react-coffee-and-friends#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: source maps path on `windows` #532

fix: source maps path on `windows` #532

alexander-akait commented May 11, 2017

codecov bot commented May 11, 2017 •

edited

michael-ciniawsky left a comment

michael-ciniawsky May 11, 2017

alexander-akait May 12, 2017

michael-ciniawsky May 12, 2017 •

edited

michael-ciniawsky commented May 12, 2017

alexander-akait commented May 12, 2017

michael-ciniawsky May 12, 2017

alexander-akait May 12, 2017

alexander-akait May 12, 2017

michael-ciniawsky May 12, 2017

bebraw May 12, 2017

alexander-akait May 12, 2017 •

edited

bebraw May 13, 2017

michael-ciniawsky May 12, 2017

michael-ciniawsky left a comment •

edited

alexander-akait commented May 12, 2017

michael-ciniawsky May 12, 2017 •

edited

alexander-akait May 12, 2017

bebraw May 12, 2017

michael-ciniawsky commented May 12, 2017 •

edited

bebraw commented May 13, 2017

michael-ciniawsky commented May 21, 2017 •

edited

joshwiens commented May 21, 2017

joshwiens commented May 21, 2017

michael-ciniawsky commented May 22, 2017

fix: source maps path on windows #532

fix: source maps path on windows #532

Conversation

alexander-akait commented May 11, 2017

codecov bot commented May 11, 2017 • edited

Codecov Report

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky May 12, 2017 • edited

Choose a reason for hiding this comment

michael-ciniawsky commented May 12, 2017

alexander-akait commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait May 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky left a comment • edited

Choose a reason for hiding this comment

alexander-akait commented May 12, 2017

michael-ciniawsky May 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky commented May 12, 2017 • edited

bebraw commented May 13, 2017

michael-ciniawsky commented May 21, 2017 • edited

joshwiens commented May 21, 2017

joshwiens commented May 21, 2017

michael-ciniawsky commented May 22, 2017

fix: source maps path on `windows` #532

fix: source maps path on `windows` #532

codecov bot commented May 11, 2017 •

edited

michael-ciniawsky May 12, 2017 •

edited

alexander-akait May 12, 2017 •

edited

michael-ciniawsky left a comment •

edited

michael-ciniawsky May 12, 2017 •

edited

michael-ciniawsky commented May 12, 2017 •

edited

michael-ciniawsky commented May 21, 2017 •

edited