Map#set polyfill is not correct

[brunoscopelliti]

Ciao Andrea,

The way Map#set is polyfilled doesn't seem correct:

set: function set(value) {
  var i = k.indexOf(value);
  v[i < 0 ? k.push(value) - 1 : i] = value;
}

https://github.com/WebReflection/domdiff/blob/master/index.js#L79-L82

I know that you've published https://github.com/WebReflection/poorlyfills, where it's implemented correctly. Probably this is just a disalignment.

Thank you.

[WebReflection]

afaik the Map is internal and it's ad-hoc for this code.

What issue are you having with this library, exactly?

[brunoscopelliti]

I've not a specific issue, but I suppose that's because I've not tested in a browser where native Map is not available.

This is how Rel is defined:

var Rel = typeof Map === 'undefined' ? function () {
  var k = [],
      v = [];
  return {
    // ...
  };
} : Map;

The problem with the polyfilled implementation is that it stores the key both as key both as value, that is:

m = new PolyfilledMap(); 
m.set("key", 42);

m.has("key"); // true, ok

m.get("key"); // “key”, that’s for sure different from what happens with native Map

Rel is used only in two different places.

In the first place, the applyDiff method (https://github.com/WebReflection/domdiff/blob/master/index.js#L217-L250), it’s used to set just a flag:

live.set(futureNodes[futureStart], 1);

We’re not interested in the specific value… in fact later we just check that the key exists:

live.has(currentNodes[currentStart])

In this case the fact that the Map#set is not spec-compliant doesn’t cause any trouble.

In the second case, the HS method (https://github.com/WebReflection/domdiff/blob/master/index.js#L86-L151), it's used to store an index.

And this is a case where it's used:

var keymap = new Rel();
for (var _i = currentStart; _i < currentEnd; _i++) {
  keymap.set(currentNodes[_i], _i);
}

When later we read this value, it seems we’re interested in an index (not a Node):

var idxInOld = keymap.get(futureNodes[_i2]);

In fact we store this value at a specific index in an array tresh[k] = idxInOld;, and later on we use it for a particular condition:

while (tresh[k] > currentEnd) {
--k;
}

Honestly I’ve not understood what k is, and why it needs to be decremented, but when tresh[k] is a Node instead that an actual number (this happen using the polyfilled Map#set) that condition is never true... and maybe this could be cause of bugs.

Since for the most common diff operations we don’t use the smartDiff algorithm it might be possible that this cause actual problems on browsers which don't support natively Map, that have been unnoticed.

What do you think?

[WebReflection]

Uhm ... I guess that's the curse of skipping code coverage, and I've been blind so far.

You are right, this is how Rel should be

function Rel() {
  var k = [],
      v = [];
  return {
    has: function has(key) {
      return -1 < k.indexOf(key);
    },
    get: function get(key) {
      return v[k.indexOf(key)];
    },
    set: function set(key, value) {
      var i = k.indexOf(key);
      v[i < 0 ? k.push(key) - 1 : i] = value;
    }
  };
}

Luckily enough, nobody uses browsers without Map these days, but in case you are interested, since all I am doing every single time is to use the same few LOC to monkey patch environments, I have created @ungap and planning to put there the most essential, 100% code covered (no Istanbul skips), most needed/common poly.

Map, and Set, will be likely next there.

[brunoscopelliti]

Glad it helped. Thank you!