Prefer Reflect.deleteProperty over the delete keyword in ES2015?

[sindresorhus]

ESLint 1.2.0 came with a new ES2015 rule called prefer-reflect that suggests Reflect where applicable.

The only method I'm unsure about is preferring Reflect.deleteProperty over the delete keyword.

xojs/eslint-config-xo@8b16afc#diff-52091282b1c3f2e056102160d5de3ec2R35

Thoughts?

Also, does it make sense to use Reflect over the other methods, like call, apply, etc?

[arthurvr]

Hmm, this is a difficult one. I think I'd prefer Reflect.deleteProperty. It don't see any strong reason not too, except that delete looks a little nicer, so then I'd rather not have to think about it and don't have any exceptions.

[sindresorhus]

Some discussion on my tweet: https://twitter.com/sindresorhus/status/634680039838117888

[Bloodsucker]

Assuming I would like to use delete functionality somewhere in my code (because it is something I would try to avoid), then I think I would prefer to use Reflect.deleteProperty.

My reason is that I believe delete foo.bar is harder to read than Reflect.deleteProperty('foo', 'bar') since Reflect.deleteProperty seems to me more readable and more explicit(?) than the delete operator. Perhaps I am infected with some java.

However, Reflect.deleteProperty support is almost null so, in that situation, instead I would prefer to use delete operator (because I have no choice).

[rauschma]

One thing makes me lean towards Reflect.deleteProperty(): its return value is easier to understand than delete’s.

[markogresak]

I have mixed feeling about this. I agree that Reflect.deleteProperty is easier to understand.
As already mentioned, current support is bad. When I was writing a benchmark to compare performances, the v8 option --harmony-reflect wasn't working (no deleteProperty) so I had to use a polyfill.

In my benchmarks, polyfilled Reflect.deleteProperty was way slower, since it's just a wrapper around delete operator. I've also added tests with splice, although this applies only to arrays, since objects don't have a splice function.
I'm not sure how realistic the results in my benchmark are, but whatever I tried, it resulted in delete being the fastest.

I say don't add this until there is support in the latest browsers. We wouldn't want to force xo users to write code which requires a polyfill to run.

[Qix-]

I'm for delete. It's analogous to other languages and has been around long enough I don't need to look up documentation. As well, Reflect.deleteProperty is a method call rather than an operator, which means delete is probably faster (though that's speculation at best - @markogresak seems to concur, though, that delete is fastest).

Unless Reflect.deleteProperty can force-delete non-configurable properties (which would be awesome), I'd say stick with delete.

[Qix-]

@rauschma: Unless I'm mistaken, delete and Reflect.deleteProperty return the same thing for all intents and purposes.

From the Reflect.deleteProperty MDN page:

Return value
A Boolean indicating whether or not the property was successfully deleted.

From the delete operator MDN page:

Return Value
Throws in strict mode if the property is an own non-configurable property (returns false in non-strict). Returns true in all other cases.

Which means I'd almost prefer delete since if I'm doing something wrong and trying to delete something that can't be deleted, the logic following the statement probably isn't correct. In strict mode (which all of my code is, personally, as should yours be!) that means my program will error out.

Unless I check all of my Reflect.deleteProperty's and handle them in some specific way (adding more code to probably just throw an Error anyway), delete seems like the clear winner.

---

'use strict';

var obj = {};
Object.defineProperty(obj, 'foo', {
  value: 1234
});

delete obj.foo;

/private/tmp/strictdelete/index.js:8
delete obj.foo;
       ^
TypeError: Cannot delete property 'foo' of #<Object>
    at Object.<anonymous> (/private/tmp/strictdelete/index.js:8:8)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
    at startup (node.js:129:16)
    at node.js:814:3

---

I'm a fail as fast as possible kind of guy, and delete does just that. Deleting a property that cannot be deleted is exceptional behavior; I shouldn't have to check for it - the code leading up to the delete should be fixed and corrected 👯

[keithamus]

o/ @sindresorhus & co. I'm the one that suggested and subsequently wrote the prefer-reflect rule - maybe I could add the reason for adding it.

The main reason is that its more an explicit call - where delete can have any identifier passed to it, Reflect.deleteProperty has 2 arguments (object and key) and so becomes a more explicit invocation. delete can throw for any number of reasons - for example it can throw ReferenceErrors for e.g. IsSuperReference, and throws if it couldn't delete a property if in strict mode. Reflect's impl does not do any of this - it'll just return true if it deleted it, or false if not. Some commenters have argued that throwing is better - I'd argue that delete is overloaded and can throw for multiple causes which becomes a problem if you do actually intend to behave differently on some of those errors (namely, if the property didn't delete).

[Qix-]

@keithamus but Reflect.deleteProperty just returns true/false (unless MDN is wrong).

If you want to act upon what exactly happened when trying to delete, wouldn't you want the specificity of delete? I can't help but to feel your final statement was contradictory.

[joakimbeng]

I personally prefer the delete keyword over Reflect.deleteProperty. The former is to me a part of the language, and quite a clear one, whereas the latter just looks like calling a function with side-effects, which I don't like.

As for the call and apply I prefer using the Reflect versions because they're more terse.

[keithamus]

@Qix- sorry I didnt really articulate myself properly toward the end of that. Let me rectify that:

Reflect.deleteProperty's possible states are:

• Throw TypeError if target isn't an Object (which, btw, is consistent with every Reflect method - every one throws a TypeError if target isn't right)
• Return true if it deleted
• Return false if it didn't.

deletes possible states are:

• Throwing a ReferenceError is the reference IsSuperReference (I can't really tell what this actually means tbh)
• If in strict mode
  • Throwing a TypeError if the delete operation returns false.
• if not in strict mode
  • Return false if delete operation returns false

That's just the states if the reference is a property reference (e.g. obj.foo) - delete has another half dozen states for all other types of references.

Now if you just call delete obj.foo over Reflect.deleteProperty(obj, 'foo') - it's hard to argue any benefits of the latter. Of course, just running these statements, you're just shifting the responsibility of all of the error catching onto the user/consumer. But if you actually want to do something useful with the return value, it becomes slightly more difficult - lets say we want to console.warn instead (contrived I know, but work with me here 😉):

if (typeof target !== 'object') {
  console.warn('Target should be an object, try again');
  return;
}
var deleted = Reflect.deleteProperty(target, key);
if (!deleted) {
  console.warn('Oops, didnt delete!');
}

"use strict";
if (typeof target !== 'object') {
  console.warn('Target should be an object, try again');
  return;
}
try {
  delete target[key];
} catch(e) {
  if (e.constructor.name === 'ReferenceError') {
    console.warn('Hmm, something weird happened with super references or some stuff');
  } else if (e.constructor.name === 'TypeError') {
    console.warn('Oops, didnt delete');
  }
}

// no-strict
if (typeof target !== 'object') {
  console.warn('Target should be an object, try again');
  return;
}
try {
  var deleted = delete target[key];
  if (deleted) {
    console.warn('Oops, didnt delete');
  }
} catch(e) {
  if (e.constructor.name === 'ReferenceError') {
    console.warn('Hmm, something weird happened with super references or some stuff');
  }
}

---

The point of the above is that despite Reflect.deleteProperty returning less information, it's actually easier to use and reason about. I say this as a capable developer, who has read the delete spec probably 100 times, and still can't make sense of some of it. Developers who haven't read the spec could reasonably grok the entirety of Reflect.deleteProperty with a few minutes of experimentation, I fear not for delete.

So you could stipulate a rule like so:

Use delete if you dont care about the results/errors, or Reflect.deleteProperty if you want a sane API for reasoning about what just got deleted.

But at that point you're mixing and matching two APIs that do the same thing, but with different return values, and may as well stick with one - so do you never care about the deleted property (and so use delete) or do you sometimes care (and so should probably go with Reflect.deleteProperty as its easier to use)?

[Qix-]

@keithamus You bring up good points, but the way I see it is that when I call delete xyz.blah I expect it to work. In strict mode, it will throw if it doesn't delete - which is what I want since it more than likely signifies a logic error or bug. Reflect requires, now, that I check the return value.

'use strict';
delete xyz.foo; // throws; must mean there's a bug in my code somewhere

whereas

if (!Reflect.deleteProperty(xyz, 'foo')) {
  throw new Error('Welp, there\'s a bug, Jim.)';
}

Further, delete is an operator, which means it's part of the language. It's cleaner and more suited for the job, and from what I'd expect, it's probably loads faster than a function call and string comparison. I'd like to see a use case where checking the return value of deleteProperty is the best possible solution over throwing an error due to program logic.

[h2non]

Interesting topic. I think this should be treated only as a preference, never defining a big constraint about imposing one or other operation, mostly because, according to MDN, Reflect.deleteProperty is just a traditional delete operation wrapped as a function.
Indeed, taking a look to SpiderMonkey source code (seems like V8 doesn't support it yet), the C++ routine actually calls to DeleteProperty(), like a traditional delete statement.
I believe that Reflect.deleteProperty shouldn't be treated with any specific preference by default, unless it's explicitly defined by the user. Also, Reflect API is not too much supported today in JS engines.

From other side, removing members from objects has evident performance downgrades in V8 since it enforces the VM to break hidden classes benefits. Maybe that could be noticed to the user as well.
Generally speaking, optimizations should be treated with coherency, for the 98% of the use cases is not significant, but for massively used libraries can be a considerable and easy to apply performance improvement. Personally, I'd tend to avoid using the delete operator.

Here're some performance tests:
http://jsperf.com/delete-vs-undefined-vs-null/12
http://jsperf.com/delete-performance/4

[Qix-]

Side note: Glad to see JS perf back up.

---

@h2non The first test does functionally different things. Just pointing that out.

[h2non]

@Qix- Take a look to the different test suite revisions, for instance this one

[Qix-]

@h2non granted, still pointing out that obj.prop = undefined or obj.prop = null is not analogous to delete obj.prop. It's not a fair comparison if you're trying to remove a property from an object, only if you're trying to get !obj.prop to return true. 'prop' in obj is still going to return true after an obj.prop = undefined.

Also, those tests are flawed; they should be using the After section to reset the variable, not contaminating test results via making function calls.

[h2non]

@Qix- That's the point, since technically in ES there's no way to strictly remove own non-inherited properties without using the delete operator (or now Reflect.deleteProperty).

So the goal here is how to achieve a similar approach without using that operator, and therefore without losing the benefit of hidden classes in V8 (taking into account the nature of C++, objects are usually internally mapped into classes which cannot vary at runtime), which is one significant design performance feature of the engine. Setting "removed" members to undefined, in certain scenarios might be enough, and then you can simply ignore those fields which are === undefined, which is analog and mimics non existent properties behavior. The unique point is that the property will remain as property, for instance when reflecting via Object.keys().

Another common approach is remove properties by clone, omitting those fields to delete during the copy, but in most cases the performance cost of copying the whole object has not too much performance gain, actually.

Obviously, we're covering very specific optimization details only justifiable in particular scenarios, so you've to consider the tradeoffs of each approach, also based on the specific use case, but in my experience, in most cases the significant part of an object is its value, not its key, so the key could remain, but pointing to nothing.

[Qix-]

Doing a for (var k in obj) or Object.keys() or even Object.getOwnPropertyNames, as you stated, will not act as expected, and adding an extra check of the value is hardly ideal. Plus, undefined might be a perfectly valid value logic-wise (i.e. when working with lookup tables).

[barneycarroll]

So the reasoning seems to be about a desire to force complexity over long-established conventions for the sake of specificity. I don't have an especially strong opinion on this. However, in case it helps others make a decision: delete is very weird in practice. I thought I remembered unexpected behaviour, so I fired up the Chrome console and tried a few operations:

delete window              // false
delete this                // true
this === window            // true
delete this                // true
delete localStorage        // true
delete localStorage.length // true
typeof localStorage        // 'object'
typeof localStorage.length // 'number'
delete Function            // true
typeof Function            // 'undefined'

[Qix-]

@barneycarroll were you in strict mode? ;)

[barneycarroll]

@qix this === window // true no sir

[Qix-]

@barneycarroll that's why. Most of those would result in delete throwing an Error. That's my point; code that uses Reflect.deleteProperty is code that expects it to work. Those will never (and should never) work - code that expects them to is erroneous.

---

Even if you had a delete wrapper, or a function that took an argument and called delete on it, if you're passing a faulty argument to that wrapper/function, the logic leading up to that call is probably erroneous as well. You should know what you're passing to delete. In theory, Reflect.deleteProperty sounds like a better choice, but when you actually consider the use-cases of delete vs those of Reflect.deleteProperty, therein lays the most pragmatic solution.

[sindresorhus]

Thanks for the feedback all. Personally, I'll stick with using the delete keyword and Reflect for the rest.