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

[mgol]

This is a rebase of PR #165 with minor modifications (e.g. a complete removal of fixUrls.js.

Fixes broken relative and protocol-relative url() use in stylesheets when
source maps are enabled. Chrome >=57 and Firefox >=50.1 support loading source
maps when sourceMapURL directive is dynamically added to a new or existing style
element in the DOM. This means that style-loader can simply add the styles and
sourceMapURL directive as text inside a style element and the source map will be
loaded by the browser. Given the simpler insertion method, the relative and
protocol-relative urls in stylesheets will not break.

Fixes #121
Ref #93

What kind of change does this PR introduce?

Bugfix + breaking change.

Did you add tests for your changes?
Not yet.

If relevant, did you update the README?
Yes.

Summary

Currently if source maps are enabled style-loader uses blob URLs instead of <style> tags with inline CSS. This makes the CSS loading asynchronous and causes subsequent JavaScript to often see the state from before applying the CSS, breaking apps (see #121). Another issue is breaking relative URLs (see #93).

Blobs were previously used to make source maps work but recent versions of major browsers have no problems with source maps working for inline <style> tags if a proper source map comment is appended.

Does this PR introduce a breaking change?

Yes. The convertToAbsoluteUrls option is removed as it's no longer needed.

Other information
I can add some tests to verify the source map comment is appended and that it has correct mapping. Is anything else needed to land it?

[mgol]

I set @john-bai as the author of the commit as almost the whole code is his. I can add tests in a separate commit.

[michael-ciniawsky]

@mgol You mentioned Chrome && Firefox. Do you know the status in Safari/Edge ? (General browser support/limitations) since this a breaking change we need to know, how to handle eventual support for folks unable to use this immediately for some reason, or maybe (hopefully) it's widely enough supported, so we can just recommend to modify their setup in one way or the other so it works for the majority 😛

[michael-ciniawsky]

Just one small thing, thx 👍 in advance, this PR is a really nice and needed cleanup :)

[michael-ciniawsky]

Please don't 😛, we use standard-version for release management across webpack-contrib and this is the responsibility of the respective maintainer(s)

[mgol]

Oops. A bad merge, I didn't intend to mess with this field.

[mgol]

Fixed.

[michael-ciniawsky]

Yep no offense I thought so 😛

[michael-ciniawsky]

@mgol Test(s) would be highly appreciated, but you mentioned it's better do it in a separate PR which is ok as long as their will be one 😛 . Otherwise browser support and how strongly this affects current users is the elefant in the room :D

[mgol]

@michael-ciniawsky I did some testing:

1. In Chrome 58 & Firefox 53 the approach from this PR works perfectly. One caveat is Chrome seems to apply the source map incorrectly if the inline style with a sourcemap is attached in the HTML statically instead of being created & appended via JS. This is not a problem for style-loader, though, as it will always do it via JS. I'll report a Chrome issue later.
2. In Safari 10.1 the blob approach works while the <style>-based one doesn't apply source maps.
3. In Edge 15 the <style>-based approach seems to work with some caveats (see the points below). The blob-based approach doesn't apply source maps. So this scenario is opposite to the Safari one. Caveats:
   3.1. It seems to point to a place near the beginning of the file instead of the correct location... but at least it gets the file right.
   3.2. It only works if you open DevTools after the page loads. Otherwise you need to reopen DevTools to see the mappings.
4. In IE 11 source maps are never applied.

I still think the <style>-based approach is the way to go as the blob-based one causes very serious issues. The only issue is that will disable source map support in Safari. If we want to keep such support, this would require imtroducing yet another flag to let people opt into the blob approach. This would increase the complexity of style-loader code even more, though.

Let me know what you think.

EDIT: Reported issues:

1. Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=714101
2. WebKit: https://bugs.webkit.org/show_bug.cgi?id=171111

[michael-ciniawsky]

First thx very much for investigating on this futher 👍 , personally I would prefer a "Best used with Chrome/Firefox' 😛 note/warning in the README instead of a bloated inconvenient 'ducktape' approach in style-loader, but that's not my call to make alone :).

[michael-ciniawsky]

@mgol mind taking a look at #153 aswell 😛 ?

[mgol]

@michael-ciniawsky does #153 influence this PR as well?

[michael-ciniawsky]

@mgol I'm not 💯 about that fact

[lydell]

I’m using this in a project, and it seems to work just fine. Sad to see that there are merge conflicts now, though!

[michael-ciniawsky]

@mgol Could you rebase one more time and change the base branch next please? :)

[mgol]

@michael-ciniawsky Done.

[alexander-akait]

@mgol maybe best create new PR?

[mgol]

@evilebottnawi Why? It's already rebased. Do you want to have clearer conversation or something?

Note that @john-bai is still marked as the author of the commit in this PR as I based my PR on his one.

[mgol]

Also, perhaps some source map tests would be useful to make sure this feature works correctly. Although we won't be able to unit-test that mappings work in the browser.

[teriu]

Any progress on getting this PR merged in?

[kevinzang]

Did this change get released? I was including it via the commit 45ba5c9, but starting today (2018-08-20) yarn install failed because 45ba5c9 is no longer a valid commit (fatal: reference is not a tree). The merge commit (8a17918) seems to also not be valid.

[mdekalka]

Would like to add to @kevinzang comment that downloading the latest version 0.23.1 seems does not include this fix.