Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

url: deleting properties has no effect #1591

Closed
silverwind opened this issue May 2, 2015 · 63 comments
Closed

url: deleting properties has no effect #1591

silverwind opened this issue May 2, 2015 · 63 comments
Labels
url Issues and PRs related to the legacy built-in url module.
Milestone

Comments

@silverwind
Copy link
Contributor

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

@silverwind silverwind added the url Issues and PRs related to the legacy built-in url module. label May 2, 2015
@silverwind silverwind changed the title url: deleting properties no longer supported url: deleting properties has no effect May 2, 2015
@petkaantonov
Copy link
Contributor

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
Copy link
Contributor Author

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?

silverwind added a commit to silverwind/npm that referenced this issue May 2, 2015
Deleting the properties will not work anymore in io.js 2.0 as the
properties are now getters/setters and deleting them will delete the
getters/setters themselves from the instance. Nulling the properties has
the same effect, while also bringing a performance gain over delete.

Related: nodejs/node#1591
@silverwind silverwind added this to the 2.0.0 milestone May 2, 2015
silverwind added a commit to silverwind/npm that referenced this issue May 2, 2015
Deleting uri properties will not work anymore in io.js 2.0.0 as the
properties are now getters/setters and deleting them will delete the
getters/setters themselves from the instance. Nulling the properties
has the same effect, while also bringing a performance gain over delete.

Related: nodejs/node#1591
@rvagg
Copy link
Member

rvagg commented May 2, 2015

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
Copy link
Contributor Author

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
Copy link
Contributor

monsanto commented May 2, 2015

@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
Copy link
Contributor Author

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

@monsanto
Copy link
Contributor

monsanto commented May 2, 2015

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
Copy link
Contributor

property definition is absurdly slow even if you have static functions

@silverwind
Copy link
Contributor Author

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

@silverwind
Copy link
Contributor Author

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

@domenic
Copy link
Contributor

domenic commented May 2, 2015

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

@Fishrock123
Copy link
Contributor

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

@chrisdickinson
Copy link
Contributor

@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
Copy link
Member

rvagg commented May 3, 2015

@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 @ npm/npm#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 npm/npm#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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor

domenic commented May 3, 2015

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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

Any stats or data on that?

@benjamingr
Copy link
Member

@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
Copy link
Contributor

mikeal commented May 3, 2015

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
Copy link
Contributor

domenic commented May 3, 2015

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
Copy link
Contributor

monsanto commented May 3, 2015

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
Copy link
Contributor

othiym23 commented May 3, 2015

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
Copy link
Contributor

mikeal commented May 3, 2015

@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
Copy link
Contributor

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
Copy link
Contributor

mikeal commented May 3, 2015

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.

@petkaantonov
Copy link
Contributor

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

@chrisdickinson
Copy link
Contributor

@mikeal:

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 this changefeature it will log 'http:///path/name?q=3'. That is, the order users change the properties in matters and can induce breaking behavior.

@petkaantonov
Copy link
Contributor

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

@jcrugzz
Copy link

jcrugzz commented May 3, 2015

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 until 3.0.0 so there is no scrambling to get things fixed before dropping the official 2.0.0

@rvagg
Copy link
Member

rvagg commented May 3, 2015

the fact that you can pass the object in to http.request() even encourages you to think of it as a plain object

@mikeal
Copy link
Contributor

mikeal commented May 3, 2015

quick question: if someone wanted this perf gain in their app today couldn't they just import it from npm and set url.parse to this version in the npm module? they could even use the --require flag to make sure it happens before any other modules get loaded and parse some urls.

@jcrugzz
Copy link

jcrugzz commented May 3, 2015

@rvagg exactly, my mind was trained to think of it as such.

@chrisdickinson
Copy link
Contributor

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

@mikeal
Copy link
Contributor

mikeal commented May 3, 2015

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

@petkaantonov
Copy link
Contributor

the npm module is completely different than this all accessors version

@silverwind
Copy link
Contributor Author

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.

@othiym23
Copy link
Contributor

othiym23 commented May 5, 2015

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

@gx0r
Copy link

gx0r commented May 5, 2015

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

No branches or pull requests