Prevent accessing property of undefined while retrocycling

[zalmoxisus]

@kolodny, not sure you'll want to accept it, but I didn't want to use a fork only for this change.

For Redux DevTools we've implemented a hack to support ImmutableJS. The idea is pretty simple, we mark custom data in the stringify replacer, then convert it back in the parse reviver. The problem, however, is that the reviver alters the path for references used by jsan and parsing fails. The solution is to skip undefined part of the path. It shouldn't be a problem for other cases, as if we're not at the end of the path and got undefined, it will fail for sure.

[kolodny]

I didn't quite follow the issue with immutable. That said I'm fine with providing an escape hatch along the lines of:

function pathGetter(obj, path, immutableEscapeHatch) {
  if (path !== '$') {
    var paths = getPaths(path);
    for (var i = 0; i < paths.length; i++) {
      path = paths[i].toString().replace(/\\"/g, '"');
      if (immutableEscapeHatch && typeof obj[path] === 'undefined' && i !== paths.length - 1) continue;
      obj = obj[path];
    }
  }
  return obj;
}

and then just call jsan.parse with an options object {immutableEscapeHatch: true}

I would need more convincing to implement it without a hatch as the default

I can make those changes or you can change the PR. I'm also giving you collaborator status for jsan on npm

[zalmoxisus]

@kolodny, sorry for not providing enough information, that's not an issue with immutable but with our implementation.

So, in jsan.stringify replacer we're converting immutable custom types like so:

function replacer(key, value) {
  if (Immutable.List.isList(value)) {
    return { data: value.toArray(), __serializedType__: 'ImmutableList' }; 
  }
  // .. other types
  return value;
}

So, instead of returning the original array (or object or other type) we're returning a new object which contains that array (more details here).

However, after parsing we restore the initial structure back inside the reviver, getting rid of that part of the subtree:

function reviver(key, value) {
  if (
    typeof value === 'object' && value !== null &&
    '__serializedType__' in value && typeof value.data === 'object'
  ) {
    const data = value.data;
    // ...
    return data;
  }
  return value;
}

In other words, if y is an Immutable Map:

{ x: { y: { z: 1 } } }

the object will become:

{ x: { data: { y: { z: 1 } }, __serializedType__: 'ImmutableMap' } }

Since, replacer adds a data key, all references to z will have as a path x.data.y.z instead of x.y.z. So, after we parse with our reviver, pathGetter will not find data key in x object and will throw as y is not a property of undefined.

This happens because we first parse the data, then call retrocycling, which has altered data by the reviver.

This PR just prevents accessing a property of undefined, which is not related to Immutable. We could call it undefinedEscapeHatch, but does it worth extending the API? Would checking if a property is undefined, affect the perf? It shouldn't have edge cases (except the fact that it will make an error not obvious), because if it has no children (i !== paths.length - 1) we return undefined.

[kolodny]

I think I understand, this change will allow things like the following:

var str = `
{
  "a": {
    "b": {}
  },
  "c": {"$jsan": "$.a.no.b"}
}`;

var parsed = jsan.parse(str);
console.log(
  parsed.a.b === parsed.c
);

Which should error. I'm OK with breaking (fixing?) people that are giving bad input to parse, I'm just wondering if there's a cleaner way

[kolodny]

Published to 3.1.6, thanks and sorry for the delay

[zalmoxisus]

@kolodny, no problem. Thanks for merging and all the investigation!