refactor: support force disabling sourcemaps and setting sourceRoot #153

kherock · 2016-12-01T05:24:04Z

What kind of change does this PR introduce?

feature

Summary

SourceMaps have so far been a real pain to track down the source of issues with, and the way CSS loaders handle them seem to be all over the place. Most of these stem from the sourceRoot property being set and then integrated into the actual source paths somewhere down the line. This is especially problematic if webpack isn't being run in the current working directory (as I am). As long as it's blank throughout though, things seem to work out.

However, once the maps arrive at style-loader, they have no root, and without one they get put under the (no domain) section in Chrome's Sources view. The ExtractTextPlugin handles these fine since the paths are set via moduleFilenameTemplate. I could write an intermediate loader just to set sourceRoot to webpack:///, but it would be nice if I could do that through style-loader's config, which is the goal of this PR.

Does this PR introduce a breaking change?

Nope!

jsf-clabot · 2016-12-01T05:24:04Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

michael-ciniawsky · 2017-03-13T19:26:58Z

@rockmacaca Can you please Close and reopen the PR to trigger the CLA Bot and rebase against current master ? 😛

joshwiens · 2017-03-15T05:53:51Z

Before anyone considers / begins to review this pull request, it needs to be rebased with current master and it has to be rebase

kherock · 2017-03-15T06:00:19Z

I'll rebase when I have a chance later this week!

kherock · 2017-03-23T23:27:14Z

@michael-ciniawsky this is ready to be looked at! Sorry I didn't mention it earlier

michael-ciniawsky

@rockmacaca 👍 See the comment for discussion && Docs and Tests then 😛

michael-ciniawsky · 2017-03-24T05:01:05Z

addStyles.js

@@ -171,12 +171,17 @@ function attachTagAttrs(element, attrs) {
 function addStyle(obj, options) {
 	var styleElement, update, remove;

+	if (obj.sourceMap && typeof options.sourceRoot === "string") {


@rockmacaca This is not mandatory, more of a triage 😛 , but would it be a breaking change, if we change the signature of sourceMap to sourceMap: Boolean || { sourceRoot: '...' } instead of adding a new option sourceRoot ? Shouldn't be, but needs to be verified

Yeah, I would rather go with this than to add yet another field to the API. Better reuse this time.

if (obj.sourceMap && !obj.sourceMap.sourceRoot.match(/webpack:\/\/\//) { obj.sourceMap.sourceRoot = 'webpack:///' + obj.sourceMap.sourceRoot }

@rockmacaca Something in that direction is what I mean by 'can we avoid adding an extra option'? 🙃 If not possible and the user needs control over this, then it can't be helped 😛

joshwiens

Needs docs & tests but beyond that and the change in the other comment should be g2g

michael-ciniawsky · 2017-04-18T11:33:14Z

@rockmacaca According to Tobias sourceRoot should always be empty to work with webpack, could you elaborate again on why you want to modify it ? 😛

michael-ciniawsky · 2017-04-25T00:47:42Z

@rockmacaca friendly ping 😛

kherock · 2017-04-25T11:39:16Z

Sorry, I've been putting this off for a bit too long -

Yes, sourceRoot is expected to be empty as maps get passed through webpack. The last time I checked, I believe css-loader typically used in conjunction will take care of setting the webpack:// prefix on maps, but I am choosing to use my own set of loaders. If you're curious what it is, it's just

sass-loader --> postcss-loader --> raw-loader (modified with sourcemap support) --> style-loader

For production builds, I have style-loader swapped with ExtractTextPlugin (which doesn't have the problem this PR addresses since it uses the moduleFilenameTemplate function). As the paths shouldn't be prefixed with anything by the time they arrive at style-loader, they appear in the (no domain) section. I'm making the PR here since it shouldn't be the responsibility of any intermediate loader to correctly set the prefix.

michael-ciniawsky · 2017-04-25T12:23:25Z

Agreeing upon that and postcss-loader v2.0.0 (especially for Angular folks) will export a module directly (no raw-loader) in the near future, could you post an usage example of how you would to use this feature purposed in the PR and could we fix it without introducing an extra option for it (based on a pattern etc)? 😛

kherock · 2017-04-25T12:46:56Z

Sure. This is a rule I have for my plain CSS files:

{ use: ['style-loader?sourceRoot=webpack:///', 'css-raw-loader'], include: path.resolve(__dirname, './app') }

css-raw-loader is literally just a copy of raw-loader but it attaches a sourcemap.

I like being able to just use the inline query string format, but your suggestion of having a signature of
sourceMap: Boolean || { sourceRoot: '...' } would also work. To be honest, the original sourceMap option I added should probably be removed entirely. I only added that while I was still figuring out how webpack's loader lifecycle worked. Since the entire sourcemap is still included in the bundle and all it is accomplishing is hiding it from the browser's sources inspector, it isn't really useful.

michael-ciniawsky

@rockmacaca Please see the comment :)

michael-ciniawsky · 2017-04-25T13:45:24Z

addStyles.js

@@ -171,12 +171,17 @@ function attachTagAttrs(element, attrs) {
 function addStyle(obj, options) {
 	var styleElement, update, remove;

+	if (obj.sourceMap && typeof options.sourceRoot === "string") {


if (obj.sourceMap && !obj.sourceMap.sourceRoot.match(/webpack:\/\/\//) { obj.sourceMap.sourceRoot = 'webpack:///' + obj.sourceMap.sourceRoot }

michael-ciniawsky · 2017-04-25T13:47:13Z

addStyles.js

@@ -171,12 +171,17 @@ function attachTagAttrs(element, attrs) {
 function addStyle(obj, options) {
 	var styleElement, update, remove;

+	if (obj.sourceMap && typeof options.sourceRoot === "string") {


@rockmacaca Something in that direction is what I mean by 'can we avoid adding an extra option'? 🙃 If not possible and the user needs control over this, then it can't be helped 😛

michael-ciniawsky · 2017-04-28T01:45:22Z

@rockmacaca How should we proceed with this ?

kherock · 2017-04-28T04:27:24Z

I'll let you know in a bit, I think I'm just going to remove the boolean sourceMap option. This PR is pretty tiny so I can rebase on top of #219 if that's getting merged soon.

I also want to investigate how it interacts with css-loader so I could possibly set some default values without breaking existing setups.

michael-ciniawsky · 2017-04-28T05:54:53Z

Yep no rush, so when #219 lands, this will still be a needed? Tbh I haven't had much time recently to take a deeper look at it, so sry in advance if this is bit naively asked 😛

kherock · 2017-04-28T06:12:10Z

Ok, I just realized what that PR does and it completely fixes the reason I added the sourceMap option, since I use relative paths in my CSS. I still want to be able to customize sourceRoot though. I'll amend this sometime soon and I'll just resolve any conflict that arises once 219 is taken care of.

michael-ciniawsky · 2018-01-08T06:13:09Z

Sry but I have to decline this, since source maps will be inlined in style-loader >= v1.0.0, this PR isn't needed/won't work in v1.0.0.

kherock · 2018-01-08T07:04:40Z

That's all right, I haven't been too concerned about it. It's actually pretty simple to create my own loader that handles prefixing the paths.

In the meantime, are there plans to publish a beta or RC for v1.0 to npm anytime soon?

michael-ciniawsky · 2018-01-08T07:15:26Z

Yes v1.0.0-alpha will be released in ~1-2 days, maybe today if nothing else pops up on my schedule :)

kherock force-pushed the master branch from 2d013e1 to 2c3cb1b Compare December 23, 2016 09:52

kherock force-pushed the master branch 3 times, most recently from b3b11e6 to 3e961e3 Compare February 24, 2017 02:09

michael-ciniawsky added type: Feature pr: refactor pr_action: Review labels Mar 13, 2017

michael-ciniawsky requested review from bebraw, ekulabuhov and joshwiens March 13, 2017 19:27

michael-ciniawsky added this to Feature in Dashboard Mar 13, 2017

joshwiens force-pushed the master branch from afd5db9 to 2630c0f Compare March 15, 2017 04:22

kherock force-pushed the master branch 2 times, most recently from 20ba6b3 to 5e9b5c2 Compare March 21, 2017 06:22

Support force disabling sourcemaps and setting sourceRoot

07f6276

kherock force-pushed the master branch from 5e9b5c2 to 07f6276 Compare March 21, 2017 06:22

michael-ciniawsky approved these changes Mar 24, 2017

View reviewed changes

michael-ciniawsky added pr: docs and removed pr: refactor pr_action: Review labels Mar 24, 2017

joshwiens reviewed Mar 28, 2017

View reviewed changes

michael-ciniawsky added pr: refactor semver: Minor labels Apr 18, 2017

michael-ciniawsky changed the title ~~Support force disabling sourcemaps and setting sourceRoot~~ refactor: support force disabling sourcemaps and setting sourceRoot Apr 18, 2017

michael-ciniawsky moved this from Feature to Refactor in Dashboard Apr 18, 2017

michael-ciniawsky mentioned this pull request Apr 21, 2017

refactor: use annotation comments for source maps (options.sourceMap) #219

Merged

michael-ciniawsky added type: Refactor priority: 2 (urgent) and removed pr: refactor labels Apr 23, 2017

michael-ciniawsky reviewed Apr 25, 2017

View reviewed changes

michael-ciniawsky mentioned this pull request Apr 25, 2017

feat: support hot module replacement (HMR) webpack-contrib/extract-text-webpack-plugin#457

Closed

joshwiens removed the status: refactor label May 1, 2017

michael-ciniawsky added this to the 1.0.0 milestone May 8, 2017

michael-ciniawsky added pr_action: Rebase status: Blocked and removed priority: 2 (urgent) labels Sep 8, 2017

michael-ciniawsky closed this Jan 8, 2018

michael-ciniawsky added status: Declined and removed pr_action: Rebase status: Blocked labels Jan 8, 2018

michael-ciniawsky removed this from the 1.0.0 milestone Jan 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: support force disabling sourcemaps and setting sourceRoot #153

refactor: support force disabling sourcemaps and setting sourceRoot #153

kherock commented Dec 1, 2016

jsf-clabot commented Dec 1, 2016 •

edited

michael-ciniawsky commented Mar 13, 2017

joshwiens commented Mar 15, 2017

kherock commented Mar 15, 2017

kherock commented Mar 23, 2017

michael-ciniawsky left a comment

michael-ciniawsky Mar 24, 2017

bebraw Mar 24, 2017

michael-ciniawsky Apr 25, 2017 •

edited

michael-ciniawsky Apr 25, 2017

joshwiens left a comment

michael-ciniawsky commented Apr 18, 2017

michael-ciniawsky commented Apr 25, 2017

kherock commented Apr 25, 2017 •

edited

michael-ciniawsky commented Apr 25, 2017

kherock commented Apr 25, 2017 •

edited

michael-ciniawsky left a comment

michael-ciniawsky Apr 25, 2017 •

edited

michael-ciniawsky Apr 25, 2017

michael-ciniawsky commented Apr 28, 2017

kherock commented Apr 28, 2017 •

edited

michael-ciniawsky commented Apr 28, 2017

kherock commented Apr 28, 2017 •

edited

michael-ciniawsky commented Jan 8, 2018

kherock commented Jan 8, 2018

michael-ciniawsky commented Jan 8, 2018

refactor: support force disabling sourcemaps and setting sourceRoot #153

refactor: support force disabling sourcemaps and setting sourceRoot #153

Conversation

kherock commented Dec 1, 2016

jsf-clabot commented Dec 1, 2016 • edited

michael-ciniawsky commented Mar 13, 2017

joshwiens commented Mar 15, 2017

kherock commented Mar 15, 2017

kherock commented Mar 23, 2017

michael-ciniawsky left a comment

Choose a reason for hiding this comment

michael-ciniawsky Mar 24, 2017

Choose a reason for hiding this comment

bebraw Mar 24, 2017

Choose a reason for hiding this comment

michael-ciniawsky Apr 25, 2017 • edited

Choose a reason for hiding this comment

michael-ciniawsky Apr 25, 2017

Choose a reason for hiding this comment

joshwiens left a comment

Choose a reason for hiding this comment

michael-ciniawsky commented Apr 18, 2017

michael-ciniawsky commented Apr 25, 2017

kherock commented Apr 25, 2017 • edited

michael-ciniawsky commented Apr 25, 2017

kherock commented Apr 25, 2017 • edited

michael-ciniawsky left a comment

Choose a reason for hiding this comment

michael-ciniawsky Apr 25, 2017 • edited

Choose a reason for hiding this comment

michael-ciniawsky Apr 25, 2017

Choose a reason for hiding this comment

michael-ciniawsky commented Apr 28, 2017

kherock commented Apr 28, 2017 • edited

michael-ciniawsky commented Apr 28, 2017

kherock commented Apr 28, 2017 • edited

michael-ciniawsky commented Jan 8, 2018

kherock commented Jan 8, 2018

michael-ciniawsky commented Jan 8, 2018

jsf-clabot commented Dec 1, 2016 •

edited

michael-ciniawsky Apr 25, 2017 •

edited

kherock commented Apr 25, 2017 •

edited

kherock commented Apr 25, 2017 •

edited

michael-ciniawsky Apr 25, 2017 •

edited

kherock commented Apr 28, 2017 •

edited

kherock commented Apr 28, 2017 •

edited