This is still semver-major, correct?

no there should not be breaking changes

Let's CI this, also npm tests should pass.

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/650/

make test-npm is happy.

I'd be fine landing this in a patch release.

@petkaantonov can you outline what's changed from the original implementation?

The parsing has been rewritten (.parse()).

Some minor changes in formatting (.format())

Some micro optimization in resolution (.resolveObject()).

Needs a rebase because of 19ffb5c

rebased

Thanks, got a few linter errors concerning this rule: http://eslint.org/docs/rules/comma-spacing

lib/url.js
   31:22  error  There should be no space before ','  comma-spacing
   32:22  error  There should be no space before ','  comma-spacing
   33:14  error  There should be no space before ','  comma-spacing
   33:28  error  There should be no space before ','  comma-spacing
  228:48  error  There should be no space before ','  comma-spacing
  636:40  error  There should be no space before ','  comma-spacing
  941:48  error  There should be no space before ','  comma-spacing
  950:46  error  There should be no space before ','  comma-spacing

Those are not spaces but comments like 0x3A/*':'*/, 0x3F/*'?'*/, ...

Yeah, it's kind of bullshit that this gets interpreted as a space.

should we open issue in eslint?

Already been done: eslint/eslint#2408

Probably best to disable the comma-spacing linter rule until that one is resolved.

How about this ??

const _protocolCharacters = makeAsciiTable([
  [0x61, 0x7A], /*a-z*/
  [0x41, 0x5A], /*A-Z*/
  0x2E, /*'.'*/ 0x2B, /*'+'*/ 0x2D /*'-'*/
]);

@yosuke-furukawa that's just confusing. also doesn't work in cases like

if (!containsCharacter2(search, 0x23 /*'#'*/, -1))

Hm... I think this is not so weird.

if (!containsCharacter2(search, 0x23, /*'#'*/ -1))

But +1 for suppress the warnings if you want.

@yosuke-furukawa would you suggest setting the setting to 1 or 0? Both seem to make make lint pass.

1 is warn, 0 is silence. 1 is better. When we will re-consider the eslint rules, we can detect the rule violation easily.

@petkaantonov the PR should pass eslint now. I'd advice changing lines like

  0x2E /*'.'*/, 0x2B /*'+'*/, 0x2D /*'-'*/

to

  0x2E/*'.'*/, 0x2B/*'+'*/, 0x2D/*'-'*/

because I assume eslint will still warn on these after the fix.

done

@petkaantonov thanks. May I ask to add a small test for the delete uri.prop case (#1591)?

One additional bit to note, the IANA registry for URI schemes lists a few more permanent schemes that require the slash (coap, for instance, http://tools.ietf.org/html/rfc7252) that are currently not listed in the _slashProtocols. Definitely do not need to add those in here but we'll likely want to revisit and add them in later.

@petkaantonov thank you for persisting with this btw. The improvements look very good on an initial review

It'll be great to see this finally land. Great work @petkaantonov

Hey everybody - what's the status, what's keeping us from merging this? :)

There are still a few outstanding comments, including the request to port the tests from the previous PR. Plus, it needs to be rebased, since it no longer merges cleanly.

I am really lacking the time at the moment

reference to original: #1561

No worries @petkaantonov
I was just checking in because Techempower is gearing up to run the next benchmark suite and I remember the low node numbers being part of your motivation to work on this in the first place.
We also landed significant speed ups for the express+jade app, so the next round should yield pretty good numbers.

Out of interest, is this still going places?

@ronkorving it nowadays live in #2303.

Ah thanks, better close this one then I guess?

I think its left open in case petka has time to resume.

Going to cloes this, Petka said he's likely not going to resume, the changes live in another PR and this can be reopend anyway if anyone disagrees.