Fix corrupted inheritance when deleting a key

Returning `hasValue` blindly without checking if `entry[0] === key` but after erasing those two references causes problems when inheritance is involved, either with `Object.create` or simply via `Constructor.prototype`.

This should not happen:

```js
var wm = new WeakMap;
var key = {};

wm.set(key, {});

// this should not affect `key` status
wm.delete(Object.create(key));

return wm.get(key); // shenanigans
```


Moreover, it would be probably not as fast but surely simpler to just rely on `this.has(key)` instead of repeating same check all over, example:

```js
if (typeof WeakMap === 'undefined') {
  (function() {
    var defineProperty = Object.defineProperty;
    var counter = Date.now() % 1e9;

    var WeakMap = function() {
      this.name = '__st' + (Math.random() * 1e9 >>> 0) + (counter++ + '__');
    };

    WeakMap.prototype = {
      set: function(key, value) {
        if (this.has(key)) {
          key[this.name][1] = value;
        } else {
          defineProperty(key, this.name, {value: [key, value], writable: true});
        }
        return this;
      },
      get: function(key) {
        return this.has(key) ? key[this.name][1] : void 0;
      },
      delete: function(key) {
        return this.has(key) && !(key[this.name].length = 0);
      },
      has: function(key) {
        var entry = key[this.name];
        if (!entry) return false;
        return entry[0] === key;
      }
    };

    window.WeakMap = WeakMap;
  })();
}
```

However, regardless specs talk about internal `key, value` slots this solution does not really have any concrete advantage on using an `Array` if not avoiding `delete` to make the object slower but that method is usually not frequently used with `WeakMap` ( reason these are demanded is that you can forget about `keys`, right ? ;-) )

Accordingly, I wonder why you didn't go for something more like [the following](https://gist.github.com/WebReflection/5991636) where also there is a standard `clear()` method which will leak like a charm in this polyfill too.

**leaking** on `.clear()` would be indeed a note you should put after you have implemented the method because all previously set `keys` cannot possibly be cleaned up but _at least you won't pollute the global scope with a broken polyfill that does not allow to invoke a legit `.clear()`_

The utterable in the constructor would be eventually another thing this polyfill needs, but I start wondering why is this even needed for `CustomElements` since [the version I've polyfilled](https://github.com/WebReflection/document-register-element) seems to work pretty well without bringing in `WeakMap` at all.

I'd love to understand why you need to ship this file, thanks for any sort of outcome.
This commit is contained in:
Andrea Giammarchi
2014-10-23 11:27:01 +01:00
parent caef618f37
commit c3f6ac623f

View File

@@ -29,10 +29,9 @@ if (typeof WeakMap === 'undefined') {
},
delete: function(key) {
var entry = key[this.name];
if (!entry) return false;
var hasValue = entry[0] === key;
if (!entry || entry[0] !== key) return false;
entry[0] = entry[1] = undefined;
return hasValue;
return true;
},
has: function(key) {
var entry = key[this.name];