Is this really a semver-major? The old behavior looks like a bug, is there really a case when it's desired?

Ah, now we throw an Error immediately when before it was passed to the callback. Seems reasonable.

@ChALkeR hmmm, to think of it, you may be right. Nobody would have preferred connecting to localhost when they actually meant to connect to a different URL. So, I guess this would not break a lot of code out there. I'll remove the major tag.

@thefourtheye Yes, even with the «pass error to callback» → «throw immediately» change it looks more like a bugfix to me. Let's see what the others will say.

@nodejs/collaborators

There are probably applications out there that request('localhost'). They get what they expect for the wrong reason but this will still break them.

@targos That's a good thing to have in the tests. I included them. Thanks :-)

This does come closer to the wep API new URL() to throw on invalid url's, and it's the path of least surprise. Though it may currently may have been undefined behavior, but that doesn't mean it's a bug fix. I'm +1 for the change, but am going to lend towards semver-major, but am open for discussion.

I also think it should be semver-major

+1 semver-major
+1 throw an error immediately
But if urls are invalid, url.parse should throw the error.

Looks like this is seen mostly as semver-major change. @ChALkeR Are you okay with landing this in major and #2966 in master now?

@thefourtheye

Looks like this is seen mostly as semver-major change. @ChALkeR Are you okay with landing this in major and #2966 in master now?

LGTM (as a semver-major) with notes and if tests pass. Btw, don't semver-major entries go directly into master now? If they do, then what is the purpose for landing #2966 in master?

Not sure about #2966. If it gets merged, it would make sense to merge that directly into 4.x branch and backport to 0.12/0.10 if applicable, and put a notice that this behavior is changed in 5.x. But I see no actual need in that, not sure about others. Let's move that discussion to #2966.

@targos
request\(.localhost regex:

min-request-1.4.1.tgz/test.js:29:               request('localhost:' + port, {body: data}, function(err, res, body) {
min-request-1.4.1.tgz/test.js:36:               request('localhost:' + port, {
mitm-servers-1.0.3.tgz/test/test.js:66:    return request('localhost:1234')
rest-stop-framework-1.0.0.tgz/test/server_test.js:17:    chai.request('localhost:3000')
rest-stop-framework-1.0.0.tgz/test/server_test.js:30:    chai.request('localhost:3000')
rest-stop-framework-1.0.0.tgz/test/server_test.js:42:    chai.request('localhost:3000')
rest-stop-framework-1.0.0.tgz/test/server_test.js:55:    chai.request('localhost:3000')
rest-stop-framework-1.0.0.tgz/test/server_test.js:68:    chai.request('localhost:3000')

request\(.127\.0\.0\.1 regex:

cadvisor-to-metric-server-0.0.4.tgz/demo/test.js:92:request('127.0.0.1:8080/api/v1.0/machine', function(err, machineInfo) {
cadvisor-to-metric-server-0.0.4.tgz/demo/test.js:94:    request('127.0.0.1:8080/api/v1.2/docker/' + id, function(err, data) {

don't semver-major entries go directly into master now

@ChALkeR Yes, they do. But if the PR is tagged as semver-major, it will be picked only during a major release cut. Till we release v5, we may want to tell users that this is the expected behavior. That is why #2966 is needed.

@thefourtheye Once again: this change LGTM, and once you decide to land it, it should be landed to master. There is no separate «major» branch, nor there is a reason for delaying landing this till the release.

Also, this change could include #2966 without the note:

 `options` can be an object or a string. If `options` is a string, it is
-automatically parsed with [url.parse()][].
+automatically parsed with [url.parse()][] and it must be a valid complete URL,
+including protocol and complete domain name or IP address.

Ah, also cc @rvagg for semver-major.

cc @nodejs/http

I'm 👍 for this change.

LGTM if the CI is happy

LGTM if CI is green

CI Run: https://ci.nodejs.org/job/node-test-pull-request/624/

The failures seem unrelated to this change. Thanks for the review. Landed at 437930c

It would be better if we get more eyes on #2966 as well. @jasnell Will this be backported to LTS as well?

@thefourtheye .. not as a semver-major

@jasnell oops right...

@thefourtheye can we avoid http{s} in future please and stick to vanilla names in commit simmaries? It makes sorting and grokking changes a bit tricky and http almost always means https anyway.

@rvagg Oh, sorry about that. I'll use only proper subsystem names now on.

I believe this PR introduced a bug.

In the following example code in Node.js 4.0.0 or earlier...

const request = http.get("localhost", res => { });

request.on('error', e => console.error(e.message));

...requires a try catch now in 5.0.0 or later

try {
  const request = http.get("localhost", res => { });
  request.on('error', e => console.error(e.message));
} catch (e) {
  console.error(e.message);
}

I would expect all parse error would be emitted like all the other errors.

Happy to do a pull request if you agree :)

@chalkers Mmm, it's actually closer to the core design philosophy of throwing early on bad inputs. Asynchronous error events are for operational errors, not logic errors.

@bnoordhuis Interesting. Do you have any other examples in the Node.js API that work that way?

I.E.

try {
   //some event emitter
}  catch (e) {
 
}

I ask because it's confusing a lot of students or new users to Node.js. I've been teaching Node.js on Treehouse for over two years and we started on 0.12. If people follow along they tend to have upgraded to 6+ (and we did naively in our interactive coding environment) expecting this would still be compatible.

If there are more examples of this pattern it would be good to know as an educator. I'm also refreshing the Node.js Basics course right now. If this is an unintended consequence of this patch, as I said before, I'd be more than happy to create a pull request resolving the issue.

@chalkers As a rule of thumb:

• passing bad arguments to an API function throws an exception
• passing good arguments that fail at run-time emits an asynchronous 'error' event.

Most APIs in Node.js work this way (although there are shades of gray, of course.) Taking this particular issue as an example:

• a malformed URL throws an exception
• a valid URL with a hostname that doesn't resolve to a DNS record results in an 'error' event

The fs module blatantly deviates from that rule with its 'does this path contain nul bytes?' check - it emits an asynchronous 'error' event even though it's clearly a bad input - but that is the only real outlier I can think of (and I still think that was a stupid move.)

Thanks for clearing that up - I'll be sure to mention the philosophy in the update :)