Repository navigation
url: deleting properties has no effect #1591
Description
Activity
- changed the title
[-]url: deleting properties no longer supported[/-][+]url: deleting properties has no effect[/+]on May 2, 2015 there is no real utility in deleting like this,
uri.protocol = null; etc has same effect (and is more efficient to boot), even with plain objects.Yeah,
deleteperf is horrible, I'm all for not supporting it anymore. What do you think about settingconfigurable = falseon all props to stopdeletefrom deleting the getters/setters?Reacted by Brian Greenforest- added a commit that references this issue
on May 2, 2015 - added a commit that references this issue
on May 2, 2015 I'm pretty sure
delete url.propis in fairly high usage in userland, if we can support this then we probably should because otherwise we're going to be dealing with lots of bug reportsI don't think this can be fixed without a fundamental change in the module (and likely a big perf loss).
My first idea was to set
configurable: falseon all props in the hopes it will throw an error ondelete, but that just silently fails. It does returnfalsebut I guess few check the return value ofdelete.@silverwind it will throw an error in strict mode (at least the case you linked to, it wont for things on the prototype unfortunately)
@monsanto thanks, I incidentally just read about that. Won't help as these properties are all on the prototype.
How slow would it be to add the getters on the instance itself for a few releases to help people migrate? Declare the functions themselves outside of the constructor so we don't get into bad closures-and-hidden-classes territory, of course.
Of course it wouldn't help for people who don't use strict mode...
property definition is absurdly slow even if you have static functions
Hmm I think
Object.observecould catch that deletion and set the prop tonull.Nope,
Object.observewon't work on the prototype. 😢39 remaining items
I think it would be better to back it out then. We'll want to re-evaluate if this is worth landing being that there is no way to fix this behavior without using proxies and we still don't know when those will land and what perf impact they'll have when they do. The assumption we were under when we agreed to take the change was that the only breaking change in
clone()behavior, I'm not sure the TC would find this level of breakage acceptable or not so it's better just to put it back through the process.The performance is not based on accesssors (except eager .format() call for .href), everything was made an accessor for consistency, for enabling browser similarity (which seems to be a goal of url module although it's currently nothing like that) and for better sanity, e.g. props can be validated and are reflected in other propd.
Also url is not a dictionary or an associative array but an object instance of Url...
The assumption we were under when we agreed to take the change was that the only breaking change in clone() behavior
I probably did a bad job of explaining the other breaking change I was worried about, but here it is, for posterity:
parsed = url.parse('http://google.com/path/name?q=3') parsed.hostname = 'ok.com' parsed.host = null console.log(url.format(parsed))
Before this
changefeature, this would log'http://ok.com/path/name?q=3', after thischangefeature it will log'http:///path/name?q=3'. That is, the order users change the properties in matters and can induce breaking behavior.@mikeal I think you mean before we decided to make everything an accessor (like in browsers).
In browsers host is just a hostname and port, it doesnt make sense to set hostname and host at the same time. You either set port and hostname separately or you set them together from one string using the host setter.
I have seen the objects returned from
url.parse()be used as if they were regular old object literals quite extensively. As @chrisdickinson points out, this changes the assumptions that might exist for some developers (including myself). I'm +1 on reverting this back until3.0.0so there is no scrambling to get things fixed before dropping the official2.0.0the fact that you can pass the object in to
http.request()even encourages you to think of it as a plain objectquick question: if someone wanted this perf gain in their app today couldn't they just import it from npm and set
url.parseto this version in the npm module? they could even use the--requireflag to make sure it happens before any other modules get loaded and parse some urls.@mikeal That would run into similar issues as
--use-strict, but would be viable for folks to try it out ahead of time. It'd be roughly equivalent to a feature flag, I think.@chrisdickinson ya, that's what I figured. that might be a better path to getting the ecosystem to update than taking more code in to core that is behind a flag we may never take out from behind the flag.
the npm module is completely different than this all accessors version
Good call on the revert. What remains to be researched is why my proposed npm patch fails npm's tests, but otherwise this is resolved. Thanks everyone.
This will probably find its way back to the TC at some point, but as I explain in npm/npm#8163, @isaacs and I don't think the right thing to do is to change npm to match the new
urlinterface. I'm happy to discuss our reasoning, but ultimately @isaacs made the call, and I agreed, that this set of changes is sufficiently problematic to require a lot more discussion and careful planning to land.Interesting...I suppose on the one hand, for any API, as an API user, I shouldn't expect delete to "work" because a property could be on a prototype. On the other hand, I wouldn't expect the shape of the object to change from Node version to version. Urlgeddon! ;-)
BTW nice work @petkaantonov. Do you know if there are techempower benchmarks showing the improvements?
Reacted by Brian Greenforest
Looks like deleting properties no longer has an effect because the properties are getters/setters. Encountered in https://github.com/npm/npm/blob/master/lib/config/nerf-dart.js
The question is, do we want to/can support this with getters/setters at all, or are we fine with it breaking? If we want to break it, we propable need to patch npm to use
uri.prop = nulloruri.prop = ''instead ofdelete uri.prop.1.8.1
2.0.0
cc: @petkaantonov @domenic @rvagg @othiym23