Interpolate objects without prototype

[devgru]

Trying to interpolate objects without prototype (and thus without valueOf and toString) results in error due to isNaN call. It fails when both methods are absent.

I've modified corresponding file to check whether at least one of valueOf or toString are present.

isNaN check has no meaning when both methods are absent because we can say for sure that variable is not a number.

[devgru]

BTW, maybe it makes sense to check not just for presence of valueOf/toString fields but also checking their types, they should be functions.

[mbostock]

Your test tests d3.interpolateObject, which already works fine in this situation:

var a = Object.create(null);
a.value = 0;

var b = Object.create(null);
b.value = 10;

var interpolator = d3.interpolateObject(a, b);
interpolator(0.5); // {value: 0.5}

Your change affects d3.interpolate, so you should test that (perhaps in addition to d3.interpolateObject).

I don’t see why it’s necessary to check for both b.valueOf and b.toString. Shouldn’t it be sufficient to check for b.valueOf before trying to coerce to a number?

export default function(a, b) {
  var t = typeof b, c;
  return b == null || t === "boolean" ? constant(b)
      : (t === "number" ? number
      : t === "string" ? ((c = color(b)) ? (b = c, rgb) : string)
      : b instanceof color ? rgb
      : b instanceof Date ? date
      : Array.isArray(b) ? array
      : !b.valueOf || isNaN(b) ? object
      : number)(a, b);
}

Alternatively, it might be better to have a safe version of isNaN that returns true if coercing to a number throws an error:

function nan(x) {
  try {
    return isNaN(x);
  } catch (error) {
    return true;
  }
}

Then replace isNaN(b) with nan(b).

[mbostock]

I made my proposed edits. Not sure whether the !b.valueOf test is better or worse than catching the error… it’s probably better assuming it detects all the situations where isNaN would throw.

[devgru]

Thanks for fast response, Mike!

Your test tests d3.interpolateObject

Oops, my fail.

Not sure whether the !b.valueOf test is better or worse than catching the error

Looks like my test was just not enough, correct test should check for both field presence and its type being equal to function, as I proposed in additional comment.

Your test covers all cases, but if I remember correctly try-catch approach can affect performance.

isNaN throws only for objects without both toString and valueOf (when trying to get 'primitive' value, see spec) so try-catch can be replaced with typeof b.valueOf != 'function' && typeof b.toString != 'function' || isNaN(b).

Shouldn’t it be sufficient to check for b.valueOf before trying to coerce to a number?

Looks like the answer is no because JS is happy enough with toString for this task:

a = Object.create(null) → {} a.toString = () => '5' → [Function] a - 0 → 5

[mbostock]

Okay. Thanks for checking the spec.

[mbostock]

Please add additional tests and I will merge. Thank you!

[devgru]

Added more tests. There are still some concerns:

• Object coercion seems really weird, object can decide whether it wants to coerce to number or not. Luckily, it is very rare case and it requires custom valueOf/toString methods. IMHO we can say that person implementing them knows what they're doing.
• Custom valueOf/toString methods are getting iterated with for-in, so I had to modify tests accordingly. Feel free to remove this cases if you find them irrelevant.
• I've tried to match your coding style but I'm not sure how much I succeed.

BTW, why not excluding functions at all from interpolate? I can't find good case when I'd want to find empty object instead of a function in my interpolated object.

[devgru]

I suggest that code is ready for release and it should be 1.1.4 because this is a bugfix, not a new feature.

We can discuss all the concerns later, IMHO they are not related to common cases.

What you think?

[mbostock]

I agree there’s not really any sense in converting a function to a blank object. It’s not that this is the desired behavior—there wasn’t really any consideration of interpolating functions in the current design.

I’m not sure it’s particularly useful to interpolate functions, or objects with function properties, but perhaps a more reasonable behavior would be to treat them like we treat non-interpolatable constants (null, undefined, true, false):

export default function(a, b) {
  var t = typeof b, c;
  return b == null || t === "boolean" || t === "function" ? constant(b)
      : (t === "number" ? number
      : t === "string" ? ((c = color(b)) ? (b = c, rgb) : string)
      : b instanceof color ? rgb
      : b instanceof Date ? date
      : Array.isArray(b) ? array
      : typeof b.valueOf !== "function" && typeof b.toString !== "function" || isNaN(b) ? object
      : number)(a, b);
}

[devgru]

Yep, looks like interpolating functions as constants is a good idea.