type tests

[Fil]

• tests salvaged from ts options #1024
• fix valueof crash

[mbostock]

My expectation was that the nullish (or falsey) check should happen earlier, not here in map. At this point we should know that values is non-null and an array or array-like.

[Fil]

I don't know. Yes it was in valueof in #1024, but with the current code we need the test on lines 12, 14 and 16. And it felt repetitive to do:

  return valueType === "string"
    ? !data
      ? data
      : maybeTypedMap(data, field(value), type) // 12
    : valueType === "function"
    ? !data
      ? data
      : maybeTypedMap(data, value, type) // 14
    : valueType === "number" || value instanceof Date || valueType === "boolean"
    ? !data
      ? data
      : map(data, constant(value), type) // 16
    : value && typeof value.transform === "function"
    ? maybeTypedArrayify(value.transform(data), type)
    : maybeTypedArrayify(value, type);

I noticed that line 16 could be optimized as:
new (type ?? Array)(arrayify(data).length).fill(value)
or maybe even:
new (type ?? typeof value === "number" ? Float64Array : Array)(arrayify(data).length).fill(value)

(Both versions pass all tests.)

[mbostock]

Can we move the data == null test first to avoid repeating it?

[mbostock]

Maybe this?

export function valueof(data, value, type) {
  if (typeof value?.transform === "function") return maybeTypedArrayify(value.transform(data), type);
  if (data == null) return data;
  const valueType = typeof value;
  return valueType === "string"
    ? maybeTypedMap(data, field(value), type)
    : valueType === "function"
    ? maybeTypedMap(data, value, type)
    : valueType === "number" || value instanceof Date || valueType === "boolean"
    ? map(data, constant(value), type)
    : maybeTypedArrayify(value, type);
}

[Fil]

nope… if value is an array, we don't care if data is nullish. But from this variant we can test data==null in maybeTypedMap, which (contrary to map) is not used anywhere else.

[mbostock]

if value is an array, we don't care if data is nullish

That’s not a requirement, and I don’t think that was intentional historically. The only time data should be nullish is for decorations—marks that are not data-driven, such as a frame. Decoration marks never have channels associated with them. And marks without data by definition don’t have an index, so even if they did theoretically have a channel, it wouldn’t be meaningful because there would be no index by which to lookup values in the channel. So, it may have been true that valueof returned the array in the valueof(null, array) case, but I don’t think we need to maintain that behavior. And we should add tests for it in any case.

Edit: Sigh. 😮‍💨 I forgot about raster and contour marks, which also utilize null data to compute “unindexed” channels when the value is expressed as a continuous function to be sampled. So I guess we have basically codified that valueof(null, array) and valueof(null, transform) should respect the value argument even though the data is nullish. So… disregard my comment. Everything is hard today it seems!

[mbostock]

What if we instead turned test/options.js into test/options.ts. Would we still need this file or could we rely on type validation of the other tests we are already running?

Edit: I guess this file is needed to assert when type errors should be generated? But that code that should not generate errors (the “valid input data” examples below) should just be part of test/options.ts like normal?

Also, should we name this src/options.test-d.ts or some such? I saw there are tools like tsd that specialize in this sort of thing. But I’d prefer to not add another dependency if tsc can do the checking for us.

[Fil]

Yes, we used tsd in the #1024 branch, but it's simpler to use tsc.

The "valid input data" were there to contrast with the ones below that expect ts errors, but they were not useful.

I've consolidated the tests (most of them were redundant).