Handle absolute URLs in urlToRequest #79
Conversation
|
|
There was a problem hiding this comment.
I'm not quite sure if this change makes sense.
While it is correct that urls with protocols should not be touched, it makes no sense to transforms urls with protocols into "module requests" (that's what this function is doing). Because "module requests" in webpack cannot be resolved via remote protocols. A module request should be file path.
@sokra what do you think?
lib/urlToRequest.js
Outdated
| @@ -30,6 +30,9 @@ function urlToRequest(url, root) { | |||
| default: | |||
| throw new Error("Unexpected parameters to loader-utils 'urlToRequest': url = " + url + ", root = " + root + "."); | |||
| } | |||
| } else if(/^[a-z][a-z0-9+\-.]*:\/\//.test(url)) { | |||
There was a problem hiding this comment.
This RegExp is very liberal. I would like to be more conservative here since there are only three reasonable variations here:
http:https://https:////(protocol agnostic)
I can't think of a use-case where other protocols are useful.
So could you change that RegExp to /^(https?:)?\/\//i?
test/urlToRequest.test.js
Outdated
| @@ -13,6 +13,7 @@ ExpectedError.prototype.matches = function(err) { | |||
| describe("urlToRequest()", () => { | |||
| [ | |||
| // without root | |||
| [["https://google.com"], "https://google.com", "should handle absolute urls"], | |||
There was a problem hiding this comment.
Please also add the test cases for the other variations
|
Anyway, thx for the PR 👍 |
|
I'm new to webpack but there seems to be lots of debate and confusion about external urls being used (CDNs) and referenced (source maps). I don't know what the answers there are going to be but at least this PR will remove this issue from the equation. My view on this is that either URLs are acceptable in some places (which this PR will fix), or they aren't in which case this PR has no effect :) Oh and I've updated the PR to address the review feedback. |
|
I think, this change makes sense anyway. Because rewriting these URLs is clearly not what we want. Using external URLs in webpack really makes no sense. Because the purpose of webpack is to bundle multiple files into fewer files. If you want to load stuff from CDNs, that's just fine. It just shouldn't be handled by webpack. |
Your comment here was part of my motivation in writing a new tool. :) I now use webpack just for bundling and my new tool (webtamp) to then run afterwards and take a higher view with regards to stuff like CDNs. If you're interested or have similar needs, you can find it here: https://github.com/japgolly/webtamp |
You need run `isUrlRequest` before run `urlToRequest`.
Part of the fix to webpack/webpack#4518