-
-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle absolute URLs in urlToRequest #79
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This RegExp is very liberal. I would like to be more conservative here since there are only three reasonable variations here:
http://
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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