GeezerVison

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

@nodejs/url

It looks like this results in many other tests failing?

Hi @Trott I'm trying to pinpoint the reason behind why the C++ code does not work.

Hi @Trott I'm trying to pinpoint the reason behind why the C++ code does not work.

I'm not terribly familiar with that code or frankly C++ in general, plus I'm kinda occupied with other things right now, but a quick git blame on the relevant part of the file and a git shortlog on the file suggest that @TimothyGu or @addaleax might be people who can provide some direction/guidance. Neither of them are as active on the project as they once were, but they're both still around.

So, the problem was that when the parser saw more than 4 . character, it just forgot about the input being a ipv4 and just continued parsing the input as a domain. Because of this particular line:

bool is_ipv4;
ParseIPv4Host(decoded.c_str(), decoded.length(), &is_ipv4);
if (is_ipv4)
  return;

Another big issue is around this line. Since, if parse number fails, it should also return a FAILURE, but before it should define is_ipv4 = true.

int64_t n = ParseNumber(mark, pointer);
if (n < 0)
  return;

@aduh95 can you re-run the failed test?

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

Microscopic nit-pick on the commit message. Can be ignored (someone can fix it while landing if they feel strongly about it), but FYI for future stuff: First word should be an imperative verb (so an action). Instead of "should validate...", just "validate....".

Thank you @Trott. You're absolutely right! I'll definitely take this feedback into consideration in my next commit. BTW, one of the test always goes to timeout. Is this a common problem or related to this pull request?

one of the test always goes to timeout. Is this a common problem or related to this pull request?

I haven't looked to see what test you are talking about specifically, but test reliability is definitely an issue we struggle with, so likely a common problem.

Landed in 87d0d7a

Thanks for the contribution! 🎉

Thanks @Trott!

@anonrig @aduh95 @targos @Trott This is not the right fix.
It now throws on new URL('http://256.256.256.256.com')
Sorry I don't have the time to open an issue.

This PR changed quite a lot between the time I reviewed it and the time it landed, and #42915 (comment) looks like a blocker to me. I think we should revert.