Improvements to calcExtents

[mhkeller]

Currently, the second argument to calcExtents is an array of objects defining a field and accessor. It might make more sense for this to be an object like

{
  x: d => d.x,
  y: d => d.y
}

Because the output of this function is a similarly structured object, it makes a bit more sense that the input matches the output. Also I think it would make it examples like the SmallMultiple wrapper be a bit easier where you wouldn't have to loop through the input array:

<LayerCake
  padding={{ top: 2, right: 6, bottom: 2, left: 6 }}
  x={extentGetters.find(d => d.field === 'x').accessor}
  y={extentGetters.find(d => d.field === 'y').accessor}
  {data}
  xDomain={$xDomain}
  yDomain={$yDomain}
>
  <Svg>
    <Line
      stroke={'#000'}
    />
  </Svg>
</LayerCake>

Would become:

<LayerCake
  padding={{ top: 2, right: 6, bottom: 2, left: 6 }}
  x={extentGetters.x.accessor}
  y={extentGetters.x.accessor}
  {data}
  xDomain={$xDomain}
  yDomain={$yDomain}
>
  <Svg>
    <Line
      stroke={'#000'}
    />
  </Svg>
</LayerCake>

There was maybe a reason why I made the input an array, though, so I'll look into this. It would also be a breaking change.

[mhkeller]

This function currently looks at the first row for any undefined, NaN or null values. It might also be better to simply skip these values instead of breaking on them, similar to some d3 functions. I want to look at the a few examples and how both d3 and tidy.js handle them.

[mhkeller]

Any thoughts welcome @jtrim-ons @jurb

[jtrim-ons]

The object-based API you're suggesting looks like an improvement to me. I've had a go at implementing it: https://github.com/jtrim-ons/layercake/blob/new-api-for-calcExtents/src/lib/calcExtents.js . I've written it so that the current array-based API still works.

I've changed the behaviour of the function in a few subtle ways, partly just to simplify the code. Some of these might not be good!

• Instead of [Infinity, -Infinity], the function returns [null, null] if there are no non-{null,undefined,NaN} values
• If there are no fields, the function returns {} instead of null
• If an accessor function returns an array, all values returned by the accessor are checked in the min and max calculations

I also fixed a bug which I think is definitely worth fixing even if you don't use any of my code in the end! If the accessor function returns an array that is part of the datum, the current version of calcExtents modifies the first item of data in place. e.g. the following test case changes the first element of data to the value of expected.

        args: [[
            { x: [-4, 0], y: [1, 6] }, { x: [-5, 1], y: [2, 7]}, { x: [-3, 2], y: [3, 8] }, { x: [-2, 3], y: [4, 9] }, { x: [-1, 4], y: [5, 10] }
        ], [{ field: 'x', accessor: d => d.x }, { field: 'y', accessor: d => d.y }]],
        expected: { x: [-5, 4], y: [1, 10] }
    }

I've added another test function to check for this.

[mhkeller]

Thanks for putting the thought into this and putting this together so quickly. I think that's a good approach! Is there a good argument for keeping the current array-based API? If the return value changes from null to {} that would already be a "breaking change."

In the library, I would probably change activeGetters to be a similarly structured object:

layercake/src/LayerCake.svelte

Line 171 in bc13c6c

const activeGetters_d = derived([_x, _y, _z, _r], ([$x, $y, $z, $r]) => {

Some backstory, I used for-loops in the original function since they're a bit faster than .forEach but it definitely made things a bit cryptic :). The svelte compiler may do this kind of optimization now, though? Not sure.

[jtrim-ons]

Some responses in no particular order...

• That makes sense about for loops. I wasn't thinking about speed at all, and the inlined for-loop style might well be faster. I don't have time to benchmark now unfortunately.
• Keeping the array-based API: I'm not sure! I was just thinking it might be nice to keep old code working. I guess one alternative would be to throw an error saying that the API has changed so people can look up the new way?
• Of the three breaking changes in my bullet points above, the first two probably aren't that useful. The third one might be a bit useful?
• The activeGetters idea seems good to me.
• Probably best not to trust my opinions on any of this!

[mhkeller]

I think that sounds good! And on the breaking change, I remembered that anything that changes how it deals with NaN and things will be a breaking change. Do you want to start on a pull request for the calcExtent file and I can slowly work on integrating and tweaking things?

[mhkeller]

Thanks for all the work on this @jtrim-ons! This is now available in 5.0.0!

[mhkeller]

I finished updating the examples, the site and the REPL examples. Let me know if I missed anything or if some changes don't stick – sometimes the REPL doesn't always save right.