Skip to content
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

Merged
merged 2 commits into from
Mar 24, 2017
Merged

Conversation

japgolly
Copy link
Contributor

Part of the fix to webpack/webpack#4518

@jsf-clabot
Copy link

jsf-clabot commented Mar 22, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@jhnns jhnns left a 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?

@@ -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)) {
Copy link
Member

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?

@@ -13,6 +13,7 @@ ExpectedError.prototype.matches = function(err) {
describe("urlToRequest()", () => {
[
// without root
[["https://google.com"], "https://google.com", "should handle absolute urls"],
Copy link
Member

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

@jhnns
Copy link
Member

jhnns commented Mar 22, 2017

Anyway, thx for the PR 👍

@japgolly
Copy link
Contributor Author

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.

@jhnns jhnns merged commit fface50 into webpack:master Mar 24, 2017
@jhnns
Copy link
Member

jhnns commented Mar 24, 2017

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.

@japgolly
Copy link
Contributor Author

japgolly commented Apr 3, 2017

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

@cornerman cornerman mentioned this pull request Mar 2, 2018
alexander-akait added a commit that referenced this pull request Dec 25, 2018
You need run `isUrlRequest` before run `urlToRequest`.
alexander-akait pushed a commit that referenced this pull request Dec 25, 2018
You need run `isUrlRequest` before run `urlToRequest`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants