added option to fix relative urls #186

sontek · 2017-03-09T16:59:18Z

This is rebased from bendytree (#124)

jsf-clabot · 2017-03-09T16:59:19Z

All committers have signed the CLA.

joshwiens · 2017-03-09T17:06:16Z

@sontek - You will need to sign the CLA.

@bebraw @michael-ciniawsky - We are going with this in favor of #124 as a prelude to @john-bai's work in #165 which is waiting on chrome-canary updates to propagate down.

joshwiens · 2017-03-09T17:11:33Z

fixUrls.js

+			.replace(/^'(.*)'$/, function(o,$1){ return $1; });
+
+		//already a full url? no change
+		if (/^(data:|http:\/\/|https:\/\/|file:\/\/\/|\/\/)/i.test(unquotedOrigUrl))


Please check the comment from @john-bai regarding urls with hash fragments #124 (comment)

@romulof fixed this error here: romulof@c695210

Fixed! I added a test to show this change, It doesn't touch any hash URLs now

sontek · 2017-03-09T17:24:27Z

@d3viant0ne Yeah, I signed it but I'm not sure if @bendytree tree has. How do you check that?

bebraw · 2017-03-09T17:30:01Z

Good point. @bendytree probably has to sign too unless his commit is lost from the history (probably something to avoid until we hear back from him).

sontek · 2017-03-09T17:35:17Z

Only thing I don't like about the PR from @romulof is that it moves from taking a currentUrl arg to using window directly. What is your opinion on that? Do you like the change?

bebraw · 2017-03-09T17:37:29Z

The window bit will fair server side most likely. But do people use style-loader on server side? It doesn't make a lot of sense there.

sontek · 2017-03-09T17:56:44Z

Ok, I pulled in the code and updated the tests to mock window.location to work with it but it looks like the code is incomplete. It didn't provide a test to prove it was working but it seems to have broke the existing tests as well.

If someone wants to play with getting the @romulof code working, the branch of it is here:

sontek#1

But its doing things like location.protocol + location.host which won't work because location.protocol returns without // in it.

ekulabuhov · 2017-03-09T18:06:16Z

addStyles.js

@@ -35,6 +36,9 @@ module.exports = function(list, options) {
 	// By default, add <style> tags to the bottom of <head>.
 	if (typeof options.insertAt === "undefined") options.insertAt = "bottom";

+	// Fix urls defaults to false
+	if (typeof options.fixUrls === "undefined") options.fixUrls = false;


There was a discussion in a previous PR about this. It seems like it makes sense to enable this by default. Otherwise we get broken CSS and no advice from Webpack on how to fix it.

Just to be safe and not break previous workarounds we could add few checks:

that sourceMaps are enabled

output.publicPath is not set

ekulabuhov · 2017-03-09T18:20:54Z

@sontek thanks for the PR. Could you also copy the README changes and the unit tests.

I can see a commit that says "Mochaify unit tests" but it looks you've just deleted the contents? 🤔

michael-ciniawsky

See @ekulabuhov @d3viant0ne comments and maybe a better name for fixURL 😛

sontek · 2017-03-09T18:37:14Z

How should hash URLs work?

if you get #bg.jpg do you fix it or just leave it as #bg.jpg?

sontek · 2017-03-09T18:42:44Z

How should double slash URLs work? if you have //a/b.jpg do you add protocol onto it?

sontek · 2017-03-09T18:56:49Z

@ekulabuhov Tests are there now, I haven't moved README in yet because there is discussion of defaulting it to true which means the docs won't be correct.

sontek · 2017-03-09T18:57:14Z

For my questions above:

both // urls and # urls stay untouched.

sontek · 2017-03-09T19:40:14Z

@d3viant0ne I believe I've made all the changes you requested.

@ekulabuhov I've made most of the changes you've requested. Only one I'm not sure of is how to check if publicPath is defined. I have it auto turning fixUrls on if they haven't set fixUrls and they have sourceMap enabled though.

joshwiens · 2017-03-09T21:03:49Z

Definitely want @bebraw to take a look at this one as well before it goes anywhere.

joshwiens · 2017-03-10T06:39:05Z

For reference, @sontek is the only one that needs to sign the CLA. This is a new branch with a single committer, the dead pull request from the user in question will simply be closed.

The comment about the hash fix is related to @john-bai's comment in the dead pull request who has this problem well researched.

As far as using window, this will fail server side which may cause issues in angular-universal or any other server side rendering.

Case in point: #167

bebraw · 2017-03-10T06:45:25Z

README.md

@@ -72,6 +72,10 @@ By default, the style-loader appends `<style>` elements to the end of the `<head

 If defined, the style-loader will re-use a single `<style>` element, instead of adding/removing individual elements for each required module. **Note:** this option is on by default in IE9, which has strict limitations on the number of style tags allowed on a page. You can enable or disable it with the singleton query parameter (`?singleton` or `?-singleton`).

+#### `fixUrls`
+
+If fixUrls and sourceMaps are both enabled, relative urls will be converted to absolute urls right before the css is injected into the page. This resolves [an issue](https://github.com/webpack/style-loader/pull/96) where relative resources fail to load when source maps are enabled.  You can enable it with the fixUrls query parameter (`?fixUrls`).


Based on the description it feels like there could be a better name for the parameter. How about convertToAbsoluteUrls? fixUrls feels too vague and you have to look it up to understand what it fixes exactly.

Is general consensus that convertToAbsoluteUrls is the way we are going? I have no strong opinions about this as long as I get the feature so I can stop using a fork :)

ekulabuhov · 2017-03-10T17:56:43Z

@d3viant0ne on the window debate: style-loader is a runtime loader. For SSR the best way to go - is to capture all CSS statically at build-time. extract-text-plugin seems to be the most common way to do it. Currently style-loader will intentionally throw an exception if used outside the browser.

sontek · 2017-03-11T17:47:20Z

@michael-ciniawsky I believe I've made all the changes you requested as well

joshwiens

I hate to ask this after all the back and forth but given Chore 57 rolled out globally yesterday, would it not be better to just get #165 up to date and landed as the original PR this is based off of is just a stop gap for the above.

Or are we going with this now & #165 in the next major? This at least improves the relative URL issue for the current major release series.

ekulabuhov · 2017-03-11T18:17:57Z

I would prefer #165 because it removes code, decreasing the complexity and chance for bugs creeping in. But let's check if #165 actually works before closing this one.

sontek · 2017-03-11T21:15:03Z

Yeah, my only concern is that this PR comes with a pretty large amount of tests that prove what its doing is correct and will work. It also works with new any version of Chrome. #165 hasn't been proven to work, comes with no tests, and only works with 57+

joshwiens · 2017-03-11T21:47:42Z

Yeah, that's pretty much what I am thinking @sontek. Realistically, #165 is going to take some time to update and properly vet.

This issue has been a pain point for a non-trivial amount of time and this pull request solves a problem in the current 0.x.x version range and allows the chrome version to lag a little bit without requiring everyone to update to what should probably a Major based on the browser requirement as Evergreen doesn't necessarily mean up to date.

We can push in this PR & any other of value to this Major range, then start merging in features / fixes that will be included in the 1.0.0. Currently that includes ...

1.) #165 will need to be updated to account for what is going to change here & then merged.
2.) Upgrade to webpack-defaults
3.) Any viable PR currently open & of value.

1.0.0-beta.0 will then begin release validation and be published on the @beta dist-tag.

As a side note, given the above regarding value to the current major range and the amount of back and forth @sontek has filtered through to get this ready, simply closing this would quite frankly be rude. Which is not something we are going to do.

sontek mentioned this pull request Mar 9, 2017

Add Option To Fix Relative Urls #124

Closed

joshwiens requested review from bebraw, michael-ciniawsky and joshwiens March 9, 2017 17:06

joshwiens requested changes Mar 9, 2017

View reviewed changes

ekulabuhov reviewed Mar 9, 2017

View reviewed changes

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

michael-ciniawsky added type: Feature pr_action: Review labels Mar 9, 2017

michael-ciniawsky self-assigned this Mar 9, 2017

michael-ciniawsky requested changes Mar 9, 2017

View reviewed changes

sontek force-pushed the fix-urls branch 2 times, most recently from 53863ae to 43941b2 Compare March 9, 2017 19:45

added option to fix relative urls

c363407

sontek force-pushed the fix-urls branch from 43941b2 to c363407 Compare March 9, 2017 19:48

add some whitespace

a4b4574

joshwiens approved these changes Mar 9, 2017

View reviewed changes

bebraw reviewed Mar 10, 2017

View reviewed changes

switch option fixUrls to convertToAbsoluteUrls

8ff86d3

joshwiens requested changes Mar 11, 2017

View reviewed changes

joshwiens added this to the 0.14.0 milestone Mar 11, 2017

joshwiens removed this from Feature in Dashboard Mar 11, 2017

joshwiens approved these changes Mar 11, 2017

View reviewed changes

michael-ciniawsky approved these changes Mar 11, 2017

View reviewed changes

michael-ciniawsky merged commit 1ae44f8 into webpack-contrib:master Mar 12, 2017

sontek deleted the fix-urls branch March 12, 2017 00:43

joshwiens pushed a commit that referenced this pull request Mar 15, 2017

fix(fixUrls): add param to fix relative urls (#186)

19959ee

michael-ciniawsky mentioned this pull request Mar 17, 2017

Make urls absolute in blob #96

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added option to fix relative urls #186

added option to fix relative urls #186

sontek commented Mar 9, 2017

jsf-clabot commented Mar 9, 2017 •

edited

joshwiens commented Mar 9, 2017

joshwiens Mar 9, 2017

sontek Mar 9, 2017

sontek commented Mar 9, 2017

bebraw commented Mar 9, 2017

sontek commented Mar 9, 2017

bebraw commented Mar 9, 2017

sontek commented Mar 9, 2017

ekulabuhov Mar 9, 2017

ekulabuhov commented Mar 9, 2017

michael-ciniawsky left a comment

sontek commented Mar 9, 2017

sontek commented Mar 9, 2017

sontek commented Mar 9, 2017

sontek commented Mar 9, 2017

sontek commented Mar 9, 2017

joshwiens commented Mar 9, 2017

joshwiens commented Mar 10, 2017 •

edited

bebraw Mar 10, 2017

sontek Mar 11, 2017

ekulabuhov commented Mar 10, 2017

sontek commented Mar 11, 2017

joshwiens left a comment •

edited

ekulabuhov commented Mar 11, 2017

sontek commented Mar 11, 2017

joshwiens commented Mar 11, 2017 •

edited

added option to fix relative urls #186

added option to fix relative urls #186

Conversation

sontek commented Mar 9, 2017

jsf-clabot commented Mar 9, 2017 • edited

joshwiens commented Mar 9, 2017

joshwiens Mar 9, 2017

Choose a reason for hiding this comment

sontek Mar 9, 2017

Choose a reason for hiding this comment

sontek commented Mar 9, 2017

bebraw commented Mar 9, 2017

sontek commented Mar 9, 2017

bebraw commented Mar 9, 2017

sontek commented Mar 9, 2017

ekulabuhov Mar 9, 2017

Choose a reason for hiding this comment

ekulabuhov commented Mar 9, 2017

michael-ciniawsky left a comment

Choose a reason for hiding this comment

sontek commented Mar 9, 2017

sontek commented Mar 9, 2017

sontek commented Mar 9, 2017

sontek commented Mar 9, 2017

sontek commented Mar 9, 2017

joshwiens commented Mar 9, 2017

joshwiens commented Mar 10, 2017 • edited

bebraw Mar 10, 2017

Choose a reason for hiding this comment

sontek Mar 11, 2017

Choose a reason for hiding this comment

ekulabuhov commented Mar 10, 2017

sontek commented Mar 11, 2017

joshwiens left a comment • edited

Choose a reason for hiding this comment

ekulabuhov commented Mar 11, 2017

sontek commented Mar 11, 2017

joshwiens commented Mar 11, 2017 • edited

jsf-clabot commented Mar 9, 2017 •

edited

joshwiens commented Mar 10, 2017 •

edited

joshwiens left a comment •

edited

joshwiens commented Mar 11, 2017 •

edited