Skip to content

Commit

Permalink
Set#delete and Map#delete should return a boolean indicating success.
Browse files Browse the repository at this point in the history 
Fixes #298.
  • Loading branch information
@ljharb
ljharb committed Oct 26, 2014
1 parent 8a77333 commit b90f0a0
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 21 deletions.
3 changes: 1 addition & 2 deletions es6-shim.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1891,8 +1891,7 @@
'delete': function (key) {
var fkey;
if (this._storage && (fkey = fastkey(key)) !== null) {
delete this._storage[fkey];
return;
return delete this._storage[fkey];

This comment has been minimized.

Sign in to view

Copy link
@Yaffle

Yaffle Oct 27, 2014

Contributor

and the question appears there: why not to just return true; ?
delete always returns true

var x = {};
console.log(delete x["test"]);

This comment has been minimized.

Sign in to view

Copy link
@ljharb

ljharb Oct 27, 2014

Author Collaborator

When a property has been defined as non-writable, delete will return false. Since delete already returns a boolean, and it's the one we want, why would we hardcode behavior that could potentially be incorrect?

This comment has been minimized.

Sign in to view

Copy link
@Yaffle

Yaffle Oct 28, 2014

Contributor

Because it is hard to undestand the logic, I do not use delete often, and I do not know, that it returns true always, (except non-writable), so return true; will be more easy to read for me in Map.prototype.delete, also for Set.prototype.delete es6-shim's logic is incorrect:

var set = new Set();
console.log(set.delete("0")); // should be `false` according to spec, seems

This comment has been minimized.

Sign in to view

Copy link
@ljharb

ljharb Oct 28, 2014

Author Collaborator

delete foo.bar will return false only if bar in foo is true after the deletion is attempted. Trying to delete a nonexistent key should return true, because the key doesn't exist after the deletion - which is all that matters.

This comment has been minimized.

Sign in to view

Copy link
@Yaffle

Yaffle Oct 28, 2014

Contributor

so, you should tell es community to change the spec for Set.prototype.delete and Map.prototype.delete

This comment has been minimized.

Sign in to view

Copy link
@ljharb

ljharb Oct 28, 2014

Author Collaborator

lol why would I do that? Any other behavior would be insane. delete returns true when it ends up not present - it returns false when it remains.

This comment has been minimized.

Sign in to view

Copy link
@Yaffle

Yaffle Oct 28, 2014

Contributor

@ljharb , did you read spec for Set.prototype.delete and Map.prototype.delete ?
did you run my example with es6-shim and without shim in Chrome or Firefox or IE?

var set = new Set();
console.log(set.delete("0")); // false, with es6-shim: true
var map = new Map();
console.log(map.delete("0")); // false, with es6-shim: false
var object = Object.create(null);
console.log(delete object["0"]); // true, with es6-shim: true

This comment has been minimized.

Sign in to view

Copy link
@ljharb

ljharb Oct 28, 2014

Author Collaborator

Ah, I didn't understand the point you were making. https://people.mozilla.org/~jorendorff/es6-draft.html#sec-set.prototype.delete and https://people.mozilla.org/~jorendorff/es6-draft.html#sec-map.prototype.delete both do indicate that a Set/Map deletion should return false if it's not in the Set/Map. Will fix.

This comment has been minimized.

Sign in to view

Copy link
@ljharb

ljharb Oct 28, 2014

Author Collaborator

Thanks for being persistent. Fixed in b3dbd35
}
ensureMap(this);
return this['[[SetData]]']['delete'](key);
Expand Down
42 changes: 23 additions & 19 deletions test/collections.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ describe('Collections', function () {

// -0 and +0 should be the same key (Map uses SameValueZero)
expect(map.has(-0)).to.equal(true);
map['delete'](+0);
expect(map['delete'](+0)).to.equal(true);
testMapping(-0, {});
expect(map.has(+0)).to.equal(true);

Expand Down Expand Up @@ -204,7 +204,7 @@ describe('Collections', function () {
expect(typeof map.keys).to.equal('function');
expect(typeof map.values).to.equal('function');
expect(map.size).to.equal(3);
map['delete']('a');
expect(map['delete']('a')).to.equal(true);
expect(map.size).to.equal(2);
});

Expand Down Expand Up @@ -307,7 +307,7 @@ describe('Collections', function () {
map.set('0', 42);
map.forEach(function (value, key) {
if (key === '0') {
map['delete']('0');
expect(map['delete']('0')).to.equal(true);
map.set('4', 'a value');
} else if (key === '4') {
hasSeenFour = true;
Expand All @@ -318,12 +318,14 @@ describe('Collections', function () {

it('does not visit keys deleted before a visit', function () {
var hasVisitedC = false;
var hasDeletedC = false;
map.forEach(function (value, key) {
if (key === 'c') {
hasVisitedC = true;
}
if (!hasVisitedC) {
map['delete']('c');
if (!hasVisitedC && !hasDeletedC) {
hasDeletedC = map['delete']('c');
expect(hasDeletedC).to.equal(true);
}
});
expect(hasVisitedC).to.equal(false);
Expand All @@ -338,7 +340,7 @@ describe('Collections', function () {
var foundMap = {};
map.forEach(function (value, key) {
foundMap[key] = value;
map['delete'](key);
expect(map['delete'](key)).to.equal(true);
});
expect(foundMap).to.eql(expectedMap);
});
Expand Down Expand Up @@ -382,9 +384,9 @@ describe('Collections', function () {
var keys = [];
var iterator = map.keys();
keys.push(iterator.next().value);
map['delete']('a');
map['delete']('b');
map['delete']('c');
expect(map['delete']('a')).to.equal(true);
expect(map['delete']('b')).to.equal(true);
expect(map['delete']('c')).to.equal(true);
map.set('e');
keys.push(iterator.next().value);
keys.push(iterator.next().value);
Expand All @@ -405,9 +407,9 @@ describe('Collections', function () {
var keys = [];
var iterator = set.keys();
keys.push(iterator.next().value);
set['delete']('a');
set['delete']('b');
set['delete']('c');
expect(set['delete']('a')).to.equal(true);
expect(set['delete']('b')).to.equal(true);
expect(set['delete']('c')).to.equal(true);
set.add('e');
keys.push(iterator.next().value);
keys.push(iterator.next().value);
Expand All @@ -426,7 +428,7 @@ describe('Collections', function () {
expect(set.has(key)).to.equal(false);
set.add(key);
expect(set.has(key)).to.equal(true);
set['delete'](key);
expect(set['delete'](key)).to.equal(true);
expect(set.has(key)).to.equal(false);
set.add(key); // add it back
};
Expand Down Expand Up @@ -515,7 +517,7 @@ describe('Collections', function () {

// -0 and +0 should be the same key (Set uses SameValueZero)
expect(set.has(-0)).to.equal(true);
set['delete'](+0);
expect(set['delete'](+0)).to.equal(true);
testSet(-0);
expect(set.has(+0)).to.equal(true);

Expand Down Expand Up @@ -692,7 +694,7 @@ describe('Collections', function () {
set.add('0');
set.forEach(function (value, key) {
if (key === '0') {
set['delete']('0');
expect(set['delete']('0')).to.equal(true);
set.add('4');
} else if (key === '4') {
hasSeenFour = true;
Expand All @@ -707,7 +709,7 @@ describe('Collections', function () {
set.add('0');
set.forEach(function (value, key) {
if (key === '0') {
set['delete']('0');
expect(set['delete']('0')).to.equal(true);
set.add('4');
} else if (key === '4') {
hasSeenFour = true;
Expand All @@ -718,12 +720,14 @@ describe('Collections', function () {

it('does not visit keys deleted before a visit', function () {
var hasVisitedC = false;
var hasDeletedC = false;
set.forEach(function (value, key) {
if (key === 'c') {
hasVisitedC = true;
}
if (!hasVisitedC) {
set['delete']('c');
if (!hasVisitedC && !hasDeletedC) {
hasDeletedC = set['delete']('c');
expect(hasDeletedC).to.equal(true);
}
});
expect(hasVisitedC).to.equal(false);
Expand All @@ -738,7 +742,7 @@ describe('Collections', function () {
var foundSet = {};
set.forEach(function (value, key) {
foundSet[key] = value;
set['delete'](key);
expect(set['delete'](key)).to.equal(true);
});
expect(foundSet).to.eql(expectedSet);
});
Expand Down

0 comments on commit b90f0a0

Please sign in to comment.