url: deleting properties has no effect

[silverwind]

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 = null or uri.prop = '' instead of delete uri.prop.

1.8.1

> uri = url.parse("https://registry.lvh.me:8661/")
> delete uri.protocol
> uri.format()
'//registry.lvh.me:8661/'

2.0.0

> uri = url.parse("https://registry.lvh.me:8661/")
> delete uri.protocol
> uri.format()
'https://registry.lvh.me:8661/'

cc: @petkaantonov @domenic @rvagg @othiym23

[petkaantonov]

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.

[silverwind]

Yeah, delete perf is horrible, I'm all for not supporting it anymore. What do you think about setting configurable = false on all props to stop delete from deleting the getters/setters?

[rvagg]

I'm pretty sure delete url.prop is 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 reports

[silverwind]

I 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: false on all props in the hopes it will throw an error on delete, but that just silently fails. It does return false but I guess few check the return value of delete.

[monsanto]

@silverwind it will throw an error in strict mode (at least the case you linked to, it wont for things on the prototype unfortunately)

[silverwind]

@monsanto thanks, I incidentally just read about that. Won't help as these properties are all on the prototype.

[monsanto]

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...

[petkaantonov]

property definition is absurdly slow even if you have static functions

[silverwind]

Hmm I think Object.observe could catch that deletion and set the prop to null.

[silverwind]

Nope, Object.observe won't work on the prototype. :cry:

[domenic]

It's a breaking release, it's ok to break delete. Just do what browsers do (normal, configurable props on the prototype).

[Fishrock123]

What's the use-case for deleting a prop? Removing that data? That is a terrible way of doing it to begin with.

[chrisdickinson]

@Fishrock123 Yep, removing the data. Using delete probably makes more sense than "assign to false-y" when the aim is to remove a given property from consideration. I'm somewhat ambivalent about removing the ability to delete properties from parsed url objects. At the very least we should note it (loudly and clearly) in the docs.

[rvagg]

@iojs/tc I'd like to hear from other TC members on this, we're about to ship code that will silently break usage like delete url.auth.

I don't believe this to be a simple matter of "assign to null noob, it's faster" and as witnessed by the existence of 2.0-compatible code even in npm @ https://github.com/npm/npm/pull/8163 this is probably not a tiny breakage to code in npm (I've probably done this myself). By shipping this feature we're breaking the "npm compatible platform" claim (ironically literally until https://github.com/npm/npm/pull/8163 is fixed!).

We have to have some respect for cowpaths that already exist and I'm anticipating no small amount of pain coming from this change.

[petkaantonov]

Using delete probably makes more sense than "assign to false-y" when the aim is to remove a given property from consideration.

it's not "assign to falsey" but assigning to null or undefined. Deleting properties is a JavaScript idiosyncracy, in any other language (dynamic or not) you would have object's properties be assigned to whatever is the null value in that language.

[silverwind]

The issue with the configurable getters/setters is that once you delete them, reassignment will make the property a plain property, possibly causing undefined behaviour. I've been looking at using Object.observe again to detect such deletions, but it just seems not possible.

@petkaantonov Do you have an estimate how the perf would be when using plain properties again? These could theoretically be observed to avoid unecessary assignment (jsperf here).

[petkaantonov]

@silverwind the properties are on the prototype, therefore delete on url objects has no effect regardless of their configurability

Edit: see http://www.ecma-international.org/ecma-262/5.1/#sec-8.12.7

[domenic]

I'll try again to make the point that this behaviour is exactly the behaviour of every DOM object ever, and should not be surprising to any JS developer.

[chrisdickinson]

I'm fretting about this a bit. Code has been written that relies on delete removing data from the url object, particularly before it's sent off to some other source. Some of this data may be sensitive, like delete url.parse(<string>).auth, as @rvagg points out. While I'm in favor of eventually defaulting to the new URL parser, non-deletable-properties-and-all, not warning (read: "exploding violently") on potentially leaked or missent info is chilling. Given that there's a tendency to parse strings into url objects, manipulate them, then hand them to APIs that perform requests in userland code, I agree with @rvagg that this is going to cause a lot of pain.

Given this, my preference is to ship the new URL code behind a feature flag for the duration of v2.0.0, which should be relatively short, given the impending V8 upgrade. If we can't get the code behind a feature flag in time for the v2.0.0 release, we should drop this from the release and get it into next for v3.0.0. Either way, the goal is to buy more time for this feature to address userland breakage, but not at the expense of this release.

[silverwind]

Deferring to 3.0.0 won't make it any better as we have no meaningful way to reach users, except through the changelog. I'd say put it in the list of breaking changes and be done with it. URL-based auth can't be that widespread anymore.

[bnoordhuis]

I agree with @silverwind. We're going to have to bite the bullet sooner or later if we want to ship this feature. I'm not sure if a feature flag would help or if it should default to on or off.

[benjamingr]

I'm pretty sure delete url.prop is in fairly high usage in userland

Any stats or data on that?

[benjamingr]

Here is some research, note this is mostly anecdotal but it should give us a broad idea, it's also partial:

These are the only actual results for delete url.path, out of these there is only one actual project:

• https://github.com/bestng/oauthic-youku
• https://github.com/briancsparks/jmt/blob/master/js/_ev3000/paniniClean.js
• https://github.com/MattiSG/Watai/blob/fd04e318b733100b949b9ed7d339ef28a175c639/src/controller/SuiteLoader.js (has stars)

For url.query:

• https://github.com/tenorviol/node-facebook-sdk/blob/0b485b7c4b1aa5eb3a47d1714c964048d5d40996/lib/facebook.js#L818-L820 (deletes keys off .query)
• https://github.com/anodejs/node-farmjs/blob/2c2b0ef59c54bcb1277d3255a8e1c25035123c36/lib/farmjs.js#L726 (deletes keys off query)
• https://github.com/jmarca/cas_validate/blob/e39a1366d5d6a7d66e5b00e81540790cca4c5979/lib/sax_parser.js#L76-L79 (commented out)
• https://github.com/valanto/jurl/blob/94e753aa17408a82190a05154121c315167569c5/jurl.js#L78 (deletes properties off .query)
• https://github.com/spadgos/getafix/blob/8031653e2b9f8c04dfc1efb9cc81b8ffc2c540cf/src/getafix.js#L121 (deletes properties off .query)
• https://github.com/shepherdwind/beacon/blob/cc154197c7f3f503ae2959233629ee44e8d333a7/routes/index.js#L23 (deletes params off .query)
• https://github.com/steelThread/connect-jsonp/blob/b5ed70d1533106ce41bcfda2e9b74c388724920e/lib/connect-jsonp.js#L57 (deletes a parameter)

Note all these delete a parameter off .query and not .query itself.

Samples for url.auth:

• https://github.com/AndyGrom/node-deploy-client/blob/dead17e734947c6a7719cc7b7549dca21c7c2b7d/lib/AdminModuleProxy.js#L12 (9 stars)

Samples for url.href:

• https://github.com/telehash/telehash-js/blob/d0ea7418b561d812bc813f28a7a49882cc0be6e5/lib/uri.js#L42

Samples for url.host:

• https://github.com/darobin/couth/blob/d7a8adbd2f797b3fa74a1af71ed84ac3bdf7762f/lib/server.js#L19
• https://github.com/MattiSG/Watai/blob/fd04e318b733100b949b9ed7d339ef28a175c639/src/controller/SuiteLoader.js (already listed above for other properties)
• https://github.com/fizker/proxy-server/blob/63d163616b0ed10f62accd79f827344fce393cea/src/server.js#L66
• https://github.com/briancsparks/jmt/blob/62e4e15f2a0e08ccddef00e6a8e96f181eb72b5a/js/_v16/lib/apacheLog.js#L266-L272

No one deletes .slashes or .protocol.

For anyone who is not used to GH code search results - this is very few matches/uses. Far less for example than the unhanldedRejection hook that was added only in io 1.4

[Fishrock123]

Maybe @chrisdickinson can get some stats on it? I'm not against the change but it is pretty short notice, if that matters?

[evanlucas]

No one deletes .slashes or .protocol.

@benjamingr looks like npm actually deletes .protocol.

https://github.com/npm/npm/blob/master/lib/config/nerf-dart.js#L16

[benjamingr]

@evanlucas yes like I said that is very partial data - only checking GH code search and cases where the parsed URL is called URL.

[silverwind]

It's unlikely the variable would be called url as that's usually the module itself. Maybe uri or parsed will give better results.

[silverwind]

Quick search for auth deletion using above variable names:

• 6 non-fork repos using delete parsed.auth
• 4 non-fork repos using delete uri.auth

[chrisdickinson]

as we have no meaningful way to reach users

We do! We have GitHub and pull requests. Delaying 'till 3.0.0 buys us time to make those PRs and get the ecosystem in a state where breakage is minimized, or at least the appropriate maintainers are made aware of the change.

URL-based auth can't be that widespread anymore.

Sure, but what about oauth tokens in GET params, for example?

[chrisdickinson]

Anec-data-ly: my local npm cache only has two repos that delete props from url.parse results: npm and relateurl. This may not be exhaustive: my delete handling isn't quite what I'd like it to be, TBH – I hadn't focussed on it until this came up!

[silverwind]

You could theoretically move to plain properties and print a deprecation warning through the delete event on Object.observe. But that would likely cost tons of perf.

[Fishrock123]

Would it be possible to use Object.observe to correct the situation if delete was used? i.e. update properly, show deprecation and re-define the property?

[silverwind]

Fishrock: Nope, observe events don't seem to fire on getters/setters, only plain properties.

[domenic]

We shouldn't be using Object.observe anyway. It's a non-standard API and as such may be removed in future V8 versions.

[silverwind]

Or rather, they do seem to fire:

> o = {}; Object.defineProperty(o, "a", { get: function(){return 1}, configurable:true})
> Object.observe(o,console.log)
> delete o.a
[ { type: 'delete', object: {}, name: 'a' } ]

But something about the implementation did not work out last time I tried, probably because the accessors are on the prototype.

[othiym23]

Deleting properties is a JavaScript idiosyncracy, in any other language (dynamic or not) you would have object's properties be assigned to whatever is the null value in that language.

False. I picked up the habit of using delete from Perl and Ruby, where it's the only way to remove both the key and the value from an associative array (which, to be clear, is also true in JavaScript, it's just that until ES2015, the only associative arrays built into JS are objects, which everyone uses interchangeably). What is an idiosyncrasy is the consequences for the JIT, because of how hidden classes work.

this behaviour is exactly the behaviour of every DOM object ever, and should not be surprising to any JS developer.

It may not be surprising if you're a browser developer. It is surprising to me as a back-end developer, because this is a change from behavior that's worked since the beginning of Node.

My point: I have an issue with some of the language in this thread. Improving the performance of url.parse is a worthwhile goal and this is a good change, but don't soft-pedal the significance of the change. Also, implying that only bad developers use a core idiom of the language is unhelpful.

As it stands, the project has a few choices:

1. wait while I land @silverwind's PR (npm/npm#8163) and then roll out a new release for testing. The fact that we didn't notice this problem until now indicates that there's a hole in npm's tests, and that leads me to want to take more deliberation around getting this change hammered on for a little while, and extremely hesitant to roll it out as a hotfix.
2. follow a path similar to the one suggested by @chrisdickinson and put it behind a flag
3. ship iojs@2.0.0 with a broken npm

I don't really think 3 is a valid option, as it will break npm for all users of private registries, and possibly lead to the creation of broken npm configuration files for those users.

[silverwind]

@domenic isn't it on track for ES7?

@Fishrock123 here's the non-working case with the prototype getter (paste into repl, no 'delete' event):

function O () {}
Object.defineProperty(O.prototype, "a", { get: function(){}, configurable:true})
o = new O()
Object.observe(o,console.log)
delete o.a

[monsanto]

I think we should ship the new url module behind a flag, and move the getters from the prototype to the instance so we can warn on deletes. This gives devs the opportunity to see if their applications would silently fail with the new module. When we release for real we can move the getters back to the prototype.

As for "this is a breaking release, so it is OK" arguments: it's one thing to intentionally break something, it's another to find something broke unexpectedly at the absolute last minute. And to make things worse, this is a silent failure, and it even affects code that we ship with! And io.js already has a (undeserved?) reputation for being unstable compared to Node. And we are spending time arguing about this stuff when we should have shipped by now.

Is the new url implementation holding back progress in other ways? Is it so critical that it be in this release? I don't think we have anything to lose by backing out and putting a debug version behind a flag.

[domenic]

@silverwind I very much doubt it will get two implementations in shipping browsers in time for the ES2016 deadline. It should be considered as non-standard as any other future ES proposal.

We shouldn't try flags; we don't have the infrastructure, and building it will waste even more time. (What are we at now, 2 weeks until next V8 release/3.0.0?) If we're not going to make this breaking change in this release, then we need to roll back and do it in 3.0.0.

There's no way to warn on delete without a Proxy, which isn't in V8 yet.

[monsanto]

@domenic Throwing an exception on delete in strict mode is one type of warning, and @silverwind just showed that you can use Object.observe to warn for properties not on the prototype. Whether or not Object.observe is non-standard is irrelevant since this would just be for debugging purposes.

[mikeal]

How much work is it to put this behind a flag as opposed to just backing it out?

[silverwind]

Just throwing this out there: If we can detect the delete, we can just assign null and be done. No userside change required.

[benjamingr]

@silverwind we can detect the delete with a proxy but those are considered unstable and under a flag atm, and are also not really ES6 compliant. A proxy has a hook for this deleteProperty. This isn't really practical (like the Object.observe) part because of the performance overhead.

I honestly think that making users just change delete url.foo to url.foo = null isn't too bad. This is a major version update after all.

[mikeal]

We can detect a delete with Object.observe(), or at least it would appear so reading this http://www.html5rocks.com/en/tutorials/es7/observe/

We should just set it to null when observing deletes.

We should not require user intervention or break npm.

[domenic]

Object.observe is nonstandard and slows down any object it touches. If we introduce that we might as well just roll back since the whole point of this change was performance.

[monsanto]

I feel like there is a lot of crosstalk here. Using Object.observe requires putting the getters on the instance + observing every instance which would defeat the purpose of a superfast implementation. It would only be of use on a debug version behind a flag. Setting null automatically is a non-starter.

[othiym23]

npm/npm#8163 does not pass npm's full test suite, even on iojs@1.8.1. I'm going to keep poking at this, but it's Sunday and I'm probably not going to spend more than another hour or two poking at this on my day off.

Unfortunately, this is not a trivial update for npm, and it's going to take some time to get this fixed and to make sure that whatever the full solution does doesn't break npm under older versions of Node.

[mikeal]

@domenic because we'd have at least a full release cycle using the rest of this new code and testing it in the wild before asking users to change behavior in order to improve the performance.

[chrisdickinson]

Notably, Object.observe is asynchronous, so beyond the perf concerns, I don't think it'd actually fix the problem.

I think at this point we should back out the change and introduce it behind a flag on the v2.x line, making it a candidate for v3 or v4. I agree with @domenic that we shouldn't block the v2.0.0 release for this to land with a feature flag.

[mikeal]

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.