feat: re-type vx/scale with new functionalities

[kristw]

💥 Breaking Changes

• Deprecate rangeRound field in the input of scaleLinear(), scaleLog(), scalePoint(), scalePower(), scaleSqrt(), scaleTime() and scaleUtc().

Instead of

scaleLinear({ rangeRound: xxx })

Do this instead

scaleLinear({ range: xxx, round: true });

• Deprecate ticks and tickFormat in the input of scaleQuantize(). It was not really doing anything anyway as both scale.ticks() and scale.tickFormat() do not mutate the scale.

• Remove scale.type field that was attached to the d3 scales.

🚀 Enhancements

New functions

• createScale(): A factory function for all scales

import { createScale } from 'vx/scale';

const myScale = createScale({ type: 'linear', domain: [1, 2] });

• inferScaleType(): Given a D3 scale, return a string that tells its type.

New fields for the scale configs

scale type	new fields
scaleLinear	interpolate, zero
scaleLog	interpolate
scalePower	interpolate, zero
scaleSqrt	interpolate, zero
scaleSymlog	clamp, nice, zero
scaleTime	clamp, interpolate, nice*
scaleUtc	clamp, interpolate, nice*

• zero: Adjust domain (if necessary) to include 0
• interpolate: Take a string of color interpolator names or parameter object and set the interpolator function for the scale.

{ 
  interpolate?: ScaleInterpolate | ScaleInterpolateParams;
}

type ScaleInterpolate =
  | 'rgb'
  | 'lab'
  | 'hcl'
  | 'hsl'
  | 'hsl-long'
  | 'hcl-long'
  | 'cubehelix'
  | 'cubehelix-long';

interface ScaleInterpolateParams {
  type: 'rgb' | 'cubehelix' | 'cubehelix-long';
  gamma?: number;
}

• nice*: This is an existing field, but for time scales, it now accepts more than boolean.

{
  nice?: boolean | number | NiceTime | { interval: NiceTime; step: number };
}

type NiceTime = 'second' | 'minute' | 'hour' | 'day' | 'week' | 'month' | 'year';

so you can specify nicing to round by every 15 minutes like this

scaleTime({
  nice: { interval: 'minute', step: 15 }
});

Other improvement

• Stronger type-safety overall with new types for all scales and scale configs.

• Updated updateScale(scale, config) with type-safety. No more any. Also type-check to make sure the scale and config are compatible.

• New type exports

📝 Documentation

• Improve documentation of the fields in scale configs.

🐛 Bug Fix

• N/A

🏠 Internal

• Rewrite individual scale factory with composition of shared operators. This ensure order of operators and simplified code.
• Add 100+ unit tests to make this vx/scale package has 100% test coverage.

[hshoff]

I know it's WIP, just leaving some general comments reading through

[hshoff]

Since these are mutable functions, is call order significant here? Does applyInterpolate() have to be called before applyRound()? If so, we might want to think of a way to enforce this. I don't expect it to a problem as it is lib code, but could be an easily introduced bug that could slip through a review.

[kristw]

Thank you for the comments. I think the order of operations is a valid point. I could set up a proxy function and only allow a single apply call with list of operations through this proxy to ensure order. Not sure if that is an overkill.

Since these are mutable functions, is call order significant here?

The ones that really have to be in order are domain > nice > zero.

Does applyInterpolate() have to be called before applyRound()?

Not necessary. They should be mutually exclusive. applyInterpolate() is in fact applyColorInterpolate. (This is a new functionality for vx. I could also drop it. Previously I don't think vx was accepting interpolate.)
Users can either do rounding which will set interpolateRound

when a scale output is number.

{ round: true }

or

when a scale output is color and they want to set color interpolation to HCL

{ interpolate: 'hcl' }

It is still not super clean and the typing may allow something like this to be specified. Although both effect cannot happen at the same time.

{ interpolate: 'hcl', round: true }

Alternatives

1. I could drop the interpolate feature for now.
2. Stricter typing to allow only either { interpolate } or { round }. Can be complicated.
3. Make { round: true } become { interpolate: 'round' }.

[hshoff]

One idea instead of proxy (can copy & paste in browser console):

const log = (str) => (scale, config) => console.log(str);

const operations = {
  domain: log('domain'),
  nice: log('nice'),
  interpolate: log('interpolate'),
  round: (scale, config) => config.interpolate ? console.warn(`can't round and interpolate`) : console.log('round'),
  zero: log('zero')
};

const order = Object.keys(operations);
const intersection = (a,b) => a.filter(v => b.includes(v));

function scaleOperator(...ops) {
  const orderedOps = intersection(order, ops);
  return (scale, config) => {
    return orderedOps.forEach(op => operations[op](scale, config))
  }
}

// order doesn't matter
const applyOperator1 = scaleOperator('round', 'zero', 'domain', 'nice');
const applyOperator2 = scaleOperator('round', 'zero', 'domain', 'interpolate');

// in createScale({ scale, config })
applyOperator1(/**scale*/ {}, /**config*/ {});
applyOperator2(/**scale*/ {}, /**config*/ { round: true, interpolate: true });

output 1: 
=> domain
=> nice
=> round
=> zero

output 2: 
=> domain
=> interpolate
=> WARN: can't round and interpolate
=> zero

[kristw]

I refactor with this idea. The individual scale files now look very clean.

[kristw]

@hshoff @williaster One question I would love thoughts here is about rangeRound.

{ rangeRound: xxx } 

is basically syntactic sugar for

{ range: xxx, round: true }

which makes sense for when it was D3 function

// for continuous scales
scale.rangeRound(xxx) = scale.range(xxx).interpolate(interpolateRound);
// for point or band scales
scale.rangeRound(xxx) = scale.range(xxx).round(true);

but when we are dealing with a config as input, should we provide single way to define round? Otherwise there could be weird combinations. We could add more strict typing to prevent, but that introduces additional complexity to the implementation.

// all of these will pass the current typescript type check unless we add more union types
{ rangeRound: xxx, round: false }
{ range: xxx, rangeRound: xxx }
{ range: xxx, rangeRound: xxx, round: false }

I am proposing we drop support for rangeRound field in the config, what do you think?

[kristw]

Another question is the addition of type field to scales.
Do we want to keep this?

// @ts-ignore
scale.type = 'log';

[williaster]

Another question is the addition of type field to scales.
Do we want to keep this?

These are definitely not ideal, I believe they were added at one point to enable checking type, but I couldn't fine any references with search. @hshoff do you remember where they were used?

If they are no longer used I think we should remove them, and even if they are it'd be great to find another approach.

[williaster]

I am proposing we drop support for rangeRound field in the config, what do you think?

This makes sense to me based on all of the considerations. If the types are clear about this, then the docs should pick them up and also be clear. If we get issues from users who are confused about it from d3 api, we could reconsider from there?

[hshoff]

I am proposing we drop support for rangeRound field in the config, what do you think?

+1

@hshoff do you remember where they were used?

My guess is I was going to use them in Legends or Axis before the TypeScript re-write. Probably ok to remove them as a breaking change.

[hshoff]

👏

[williaster]

nice solution

[williaster]

Had a couple of small suggestions, mostly just impressed! Amazing job 💯

[williaster]

this will be amazing for XYChart 🤩

[williaster]

this is amazing

[kristw]

Got this package to 100% test coverage. Happy reviewing!

[hshoff]

🍜 🍔 🥪 🟢

[williaster]

💯