I've been using Petka's url replacement in my apps for a while now and generally it works well. This can potentially speed up a lot of apps for free. The caveats don't sound too bad either. I admit that the fact this was not merged into node (or at least reviewed) when it was first written was quite a surprise for me.

Is this new implementation more or less compliant with the URL Standard? How does it stack up on the URL test suite?

@domenic The goal was not to make a new url parser, just to optimize the existing one. So it should give exactly the same result as the current url url parser - if that means some will fail there then so be it.

perhaps we should take this in the next major semver bump?

@mikeal that's ok with me, what branch do I pr against for major?

@petkaantonov we're still figuring that out :) we have the same problem with a PR for improving util.isObject and didn't entirely resolve it in the last TC meeting. I'll add this one to the agenda though.

I know we don't have any data (and @mikeal really wants a way to make some) but I'd be surprised if this was actually back-compat-breaking in any way.

@domenic this thread is pretty long but I seem to remember that there is an issue with the return value not working util.extends? That will surely cause breaks in a few people's applications.

@mikeal ah yeah you're right same with xtend. I take it back.

@mikeal util._extends is changed to call toJSON() - many internals pass Url object directly to util extend as well. toJSON is not an arbitrary method name, it is used by JSON.stringify as well.

https://github.com/iojs/io.js/pull/933/files#diff-3deb3f32958bb937ae05c6f3e4abbdf5R635

@petkaantonov are you saying it is changed or that it would need to change?

@mikeal It is changed in this PR as I linked

@petkaantonov sorry, in my email that link wasn't there :)

Yea I have a really bad habit of firing comments asap and then editing them later :P

Is there a perf impact elsewhere from changing ._extends?

I am not sure that changing _extend's semantics is a very good idea. It'd be better to change usages of it with URLs throughout the codebase to instead call toJSON() before passing to _extends.

@domenic what do you mean by "throughout the codebase?" the real issue isn't usage in core but usage by applications.

I'm talking purely about _extends here. I agree that in general people will use xtend or similar tools with URLs and this change will break that, which means overall its semver-major. I just am nitpicking @petkaantonov's particular mitigating strategy for handling core.

Is there a perf impact elsewhere from changing ._extends?

I cannot find a benchmark but I would be surprised if a property lookup and type-check would even show up considering the code on the very next line loads all properties of the object in an array.

@domenic I did it this way because util._extends is essentially used as public method I think.

@petkaantonov yes and I think modifying its behavior in the face of toJSON-able objects is not a good change to make.

Well it could always be a hardcoded check for Url instance as well, but I was thinking that if you really defined a toJSON method on an object, you would prefer to use the properties returned by that when using ._extend.

@petkaantonov what's the status of this? Are you OK with @domenic's suggestion of putting getters & setters on all properties for consistency? If so, can we go ahead and get this merged or is there something else holding it up?

@rvagg Yes I am OK with it. I just haven't gotten around to do the work.

@petkaantonov cool, no problems, just let us know if timing sucks too much and we can cut it from 2.0.0 to take the pressure off.

Ill work on this today and see if I can finish this

Making everything accessors has given some unexpected trouble with the resolveObject method, it's gonna take one more day or two

Moved to #1561 which is rebased against master so that it's possible to review it