fix: source maps path on `windows` by alexander-akait · Pull Request #532 · webpack/css-loader

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

 		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 😛

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/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/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](`https://github.com/webpack-contrib/css-loader/issues/698`)) ([c788450](webpack/css-loader@c788450)) #### [0.28.10](webpack/css-loader@v0.28.9...v0.28.10) (2018-02-22) ##### Bug Fixes * **getLocalIdent:** add `rootContext` support (`webpack >= v4.0.0`) ([#681](`https://github.com/webpack-contrib/css-loader/issues/681`)) ([9f876d2](webpack/css-loader@9f876d2)) #### [0.28.9](webpack/css-loader@v0.28.8...v0.28.9) (2018-01-17) ##### Bug Fixes * ignore invalid URLs (`url()`) ([#663](`https://github.com/webpack-contrib/css-loader/issues/663`)) ([d1d8221](webpack/css-loader@d1d8221)) #### [0.28.8](webpack/css-loader@v0.28.7...v0.28.8) (2018-01-05) ##### Bug Fixes * **loader:** correctly check if source map is `undefined` ([#641](`https://github.com/webpack-contrib/css-loader/issues/641`)) ([0dccfa9](webpack/css-loader@0dccfa9)) * proper URL escaping and wrapping (`url()`) ([#627](`https://github.com/webpack-contrib/css-loader/issues/627`)) ([8897d44](webpack/css-loader@8897d44)) #### [0.28.7](webpack/css-loader@v0.28.6...v0.28.7) (2017-08-30) ##### Bug Fixes * pass resolver to `localsLoader` (`options.alias`) ([#601](`https://github.com/webpack/css-loader/issues/601`)) ([8f1b57c](webpack/css-loader@8f1b57c)) #### [0.28.6](webpack/css-loader@v0.28.5...v0.28.6) (2017-08-30) ##### Bug Fixes * add support for aliases starting with `/` (`options.alias`) ([#597](`https://github.com/webpack/css-loader/issues/597`)) ([63567f2](webpack/css-loader@63567f2)) #### [0.28.5](webpack/css-loader@v0.28.4...v0.28.5) (2017-08-17) ##### Bug Fixes * match mutliple dashes (`options.camelCase`) ([#556](`https://github.com/webpack/css-loader/issues/556`)) ([1fee601](webpack/css-loader@1fee601)) * stricter `[@import]` tolerance ([#593](`https://github.com/webpack/css-loader/issues/593`)) ([2e4ec09](webpack/css-loader@2e4ec09)) #### [0.28.4](webpack/css-loader@v0.28.3...v0.28.4) (2017-05-30) ##### Bug Fixes * preserve leading underscore in class names ([#543](`https://github.com/webpack/css-loader/issues/543`)) ([f6673c8](webpack/css-loader@f6673c8)) #### [0.28.3](webpack/css-loader@v0.28.2...v0.28.3) (2017-05-25) ##### Bug Fixes * correct plugin order for CSS Modules ([#534](`https://github.com/webpack/css-loader/issues/534`)) ([b90f492](webpack/css-loader@b90f492)) #### [0.28.2](webpack/css-loader@v0.28.1...v0.28.2) (2017-05-22) ##### Bug Fixes * source maps path on `windows` ([#532](`https://github.com/webpack/css-loader/issues/532`)) ([c3d0d91](webpack/css-loader@c3d0d91)) #### [0.28.1](webpack/css-loader@v0.28.0...v0.28.1) (2017-05-02) ##### Bug Fixes * allow to specify a full hostname as a root URL ([#521](`https://github.com/webpack/css-loader/issues/521`)) ([06d27a1](webpack/css-loader@06d27a1)) * case insensitivity of [@import] ([#514](`https://github.com/webpack/css-loader/issues/514`)) ([de4356b](webpack/css-loader@de4356b)) * don't handle empty [@import] and url() ([#513](`https://github.com/webpack/css-loader/issues/513`)) ([868fc94](webpack/css-loader@868fc94)) * imported variables are replaced in exports if followed by a comma ([#504](`https://github.com/webpack/css-loader/issues/504`)) ([956bad7](webpack/css-loader@956bad7)) * loader now correctly handles `url` with space(s) ([#495](`https://github.com/webpack/css-loader/issues/495`)) ([534ea55](webpack/css-loader@534ea55)) * url with a trailing space is now handled correctly ([#494](`https://github.com/webpack/css-loader/issues/494`)) ([e1ec4f2](webpack/css-loader@e1ec4f2)) * use `btoa` instead `Buffer` ([#501](`https://github.com/webpack/css-loader/issues/501`)) ([fbb0714](webpack/css-loader@fbb0714)) ##### Performance Improvements * generate source maps only when explicitly set ([#478](`https://github.com/webpack/css-loader/issues/478`)) ([b8f5c8f](webpack/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/postcss-loader#224

Closed

includePaths option prepends sass folder path to file locations webpack/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/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

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/sass-loader#460

Closed

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 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

snyk-bot mentioned this pull request May 4, 2020

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

Open

KrisPeachyKen mentioned this pull request Jul 30, 2024

[Snyk] Upgrade css-loader from 0.18.0 to 0.28.11 KrisPeachyKen/ai#2

Open

superadmiral mentioned this pull request Jul 30, 2024

[Snyk] Upgrade css-loader from 0.18.0 to 0.28.11 superadmiral/ai#2

Open

YinghanFeng mentioned this pull request Aug 2, 2024

[Snyk] Upgrade css-loader from 0.9.1 to 0.28.11 YinghanFeng/deco-ide#1

Open

go-benchmark1 mentioned this pull request Aug 16, 2024

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 go-benchmark1/pome#1

Open

Vishesh7797 mentioned this pull request Oct 26, 2024

[Snyk] Upgrade css-loader from 0.26.4 to 0.28.11 Vishesh7797/Sorting-Visualizer#2

Open

Uh oh!

Conversation

alexander-akait commented May 11, 2017

Uh oh!

codecov bot commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented May 12, 2017

Uh oh!

alexander-akait commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented May 12, 2017

Uh oh!

michael-ciniawsky May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-ciniawsky commented May 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshwiens commented May 21, 2017

Uh oh!

joshwiens commented May 21, 2017

Uh oh!

michael-ciniawsky commented May 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 11, 2017 •

edited

Loading

michael-ciniawsky May 12, 2017 •

edited

Loading

alexander-akait May 12, 2017 •

edited

Loading

michael-ciniawsky left a comment •

edited

Loading

michael-ciniawsky May 12, 2017 •

edited

Loading

michael-ciniawsky commented May 12, 2017 •

edited

Loading

michael-ciniawsky commented May 21, 2017 •

edited

Loading