Convert @finos/perspective-viewer-d3fc to TypeScript

[brochington]

This PR is as a foundational first step to adding types to the perspective-viewer-d3fc package. Adding full typing to perspective-viewer-d3fc is a very large and complex task, and the resulting PR would be...massive. The aim of this PR is offer a starting ground for adding better types in the future.

In this PR:

• All .js files in the src/js have been given a .ts extension.
• Immediate typing issues (red squiggles) have been addressed.

Very minimal changes to the original functional code have been made, and no change in behavior should be noticeable.

[texodus]

Thanks for the PR!

Draft PRs are for work that is under development but not ready to be merged. Why is this PR in draft? Are you planning on finishing this work or ...?

Adding full typing to perspective-viewer-d3fc is a very large and complex task, and the resulting PR would be...massive.

Hence why it has not been done yet, I'm not interested in a perpetually half-typed package.

[texodus]

Thanks for the PR!

Individual chart imports are broken, this definitely needs to be fixed (see notes below). This really needs a test please as well.

There is a lot of typing in this PR that isn't checked (e.g. removing or breaking it does not introduce errors) and/or has non-obvious ellision of safety in other places when written incorrectly - GetSetReturn and the interface/return type disconnect are two major ones. I am willing to merge this without complete fix for these, but I think a clear answer beyond:

Adding full typing to perspective-viewer-d3fc is a very large and complex task

What can I expect about the end state of this work? Is there a stage 2 that is going to address some of the issues below, if so which ones and when? Or is this the end state of TypeScript support in this package?

[texodus]

💀

[texodus]

Wrong type SVGAElement

[texodus]

The CLI output for this is ugly, lacks formatting, line break, colors, and VS Code doesn't identify the error links in the built-in terminal. If you just use execSync() instead of exec, the sub shell will get the parent process pipes.

[brochington]

Good call! Will use execSync.

[texodus]

Wrong base type

[texodus]

Wrong type SVGAElement

[texodus]

This type adds value especially in the IDE, but it is excessively noisy at the function definition:

<T extends string | undefined = undefined>(
   ...args: T[]
): GetSetReturn<T, string, Obj>

and the interface

<T extends string | undefined = undefined>(
   ...args: T[]
): GetSetReturn<T, string, Obj>

and the function body (this pattern is repeated quite a lot in this file and would'v ebeen easier to type if we'd just removed args and replaced it with the known type):

if (!args.length) {
    return value as GetSetReturn<T, string, Obj>;
}

value = args[0];
return obj as GetSetReturn<T, string, Obj>;

The 2nd cast is not present in most of the code in this file, but would actually be necessary with strictNullChecks due to the values for these closure-scoped objects initializing to null.

Due to these casts (and the lack of "strict"), it is very easy to accidentally screw up the definition of one of these GetSetReturn functions as there is virtually no internal type checking on these members. Doing so introduces no errors, it just makes the type checking at the call site worse (tsc can no longer determines whether the call is a getter or setter).

This is IMO a particular pain point of this rewrite, a lot of verbosity is added here for something that we can't really enforce at compile time (with this tsconfig at least).

[ada-x64]

d3 seems to do this with function overloads instead of a unified type, see e.g. d3-shape.d.ts. Since we're writing and immediately implementing our function, this may mean we need to rewrite the component into explicit getter/setter tuples (usually pairs, but a triple in the code below), or perhaps just add a separate .d.ts beside the original js file.
EDIT: It can be a single function implementation as long as it covers all the type constraints.

    /**
     * Returns the current data comparator, which defaults to null.
     */
    sort(): ((a: Datum, b: Datum) => number) | null;
    /**
     * Sets the data comparator to the specified function and returns this pie generator.
     *
     * If both the data comparator and the value comparator are null, then arcs are positioned in the original input order.
     * Otherwise, the data is sorted according to the data comparator, and the resulting order is used. Setting the data comparator implicitly sets the value comparator to null.
     *
     * Sorting does not affect the order of the generated arc array which is always in the same order as the input data array; it merely affects the computed angles of each arc.
     * The first arc starts at the start angle and the last arc ends at the end angle.
     *
     * @param comparator A compare function takes two arguments a and b, each elements from the input data array.
     * If the arc for a should be before the arc for b, then the comparator must return a number less than zero;
     * if the arc for a should be after the arc for b, then the comparator must return a number greater than zero;
     * returning zero means that the relative order of a and b is unspecified.
     */
    sort(comparator: (a: Datum, b: Datum) => number): this;
    /**
     * Sets the data comparator to null and returns this pie generator.
     *
     * If both the data comparator and the value comparator are null, then arcs are positioned in the original input order.
     *
     * @param comparator null, to set the pie generator to use the original input order or use the sortValues comparator, if any.
     */
    sort(comparator: null): this;

(Side note, looks like the d3 authors are on the tau side of the circle constant debate... https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/d3-shape/index.d.ts#L72)

[brochington]

I need to play around a little bit more with it, but so far to it looks like method overloading is a much better option than using GetSetReturn. (thanks @ada-x64!).

interface AxisFactory {
    (data: any[]): any;
    memoValue(): Axis;
    memoValue(a: Axis): this;

    excludeType(): () => AxisTypeValues;
    excludeType(nextExcludeType?: AxisTypeValues): this;
    // ... everything else
}

const AxisFactory = (settings: Settings): AxisFactory => {
      const _factory = (data): AxisFactoryContent = { ... };

      _factory.memoValue = (nextMemoValue?: Axis): Axis | AxisFactory => {
        if (!nextMemoValue) {
            return memoValue;
        }

        memoValue = nextMemoValue;
        return _factory;
    };

    _factory.excludeType = (nextExcludeType?: AxisTypeValues) => {
        if (!nextExcludeType) {
            return excludeType;
        }

        excludeType = nextExcludeType;
        return _factory;
    };
}

[texodus]

We should really make an effort to re-use the types from the packages where they are defined if we want to benefit from type-safety. These are defined in perspective though I do see a bug already that they don't define null and allow mixed-types per column.

[texodus]

" check the perspective types enum.

[texodus]

TreeMap is a custom chart exclusive to Perspective, TreeData is the internal tree node type. It's not an external type it just lacks a constructor.

[texodus]

This pattern is used quite a bit in this rewrite - casting the return type as any to avoid having to match the dynamically bound methods to the interface signature. However, this means the types of the internal object and interface may differ completely, and there are a bunch of subtle inconsistencies as a result, especially around null-ness.

[ada-x64]

Overall I'm pretty pleased with this refactor. I think there are several rough edges and places where types are still missing or improperly inferred, but I think this is worth merging as a first step.
Please take a look at the changes I've suggested. I think it would be fine to add these as additional PRs, since the goal with this PR is to at least get typescript working and out the door.

[ada-x64]

This is missing the columns property which sets column styles. I noticed that xy-scatter's overrideSymbols didn't have the Settings type on its settings property - this may be why.
These settings are the plugin-internal settings. Some of them get serialized, some don't. There's the EXCLUDED_SETTINGS const in plugin.ts which tells you which aren't serialized. (It's most of them.)

[brochington]

Good catch. What do you think the type of settings.columns should be? Thanks for pointing out EXCLUDED_SETTINGS.

[ada-x64]

Record<string, Record<string, unknown>>

This could probably be narrowed so the inner Record is an object containing the specific types of plugin configurations. For example:

type ColumnConfig = {
    symbol: Record<string, string>
}
type ColumnSettings = Record<string, ColumnConfig>

let example = {"Region": {symbol: {"West": "star"}}}

This particular configuration setting is undergoing changes atm so don't worry about fitting it too closely

[brochington]

Awesome, I've updated the settings.columns type.

[ada-x64]

This could be very useful, but it's not used anywhere but ohlcCandle.
Also the proper types for the function are as follows:

    (
    	// obtained from the results of `d3.select(this._container))` in `plugin._draw` 
        // resolves to d3.Selection<HTMLElement, unknown, null, undefined>,
        container: ReturnType<typeof d3.select<HTMLElement, unknown>>,
        settings: Settings
    ): void;

The container type here would cascade down and fill out all the container types throughout the codebase. They would probably need to replace the datum parameter with whatever datum they're using, but this seems to be the general form.

EDIT: I see why this isn't used - the current code just creates a function and plops a property on it. TS can't really handle that. I know the intention is to keep the code as close to the original as possible, but I think there's a pattern we could use to integrate this. This is similar to what ohlcCandle does, and I think we can do it for the other charts as well without much change. See this playground

type ChartFactory = (
    container: {},
    settings: {},
) => void;

interface Chart {
    this: ChartFactory;
    plugin?: {
        name: string;
        // etc
    };
}

// pretend this is a module
let x = (): Chart => {
    let factory: ChartFactory = (container, settings) => {};
    let chart = factory as unknown as Chart;
    chart.plugin = {name: "hey"};
    return chart;
}

console.log(x(), x().plugin);

[brochington]

Good catch on the container arg type! I've updated the Chart type and chart functions.

The ChartFactory/Chart pattern described above could be done, but I'm not 100% it needs to be (at least at the moment). There is some checking of the chart types where the constructors are being used:

// src/ts/charts/charts.ts
const chartClasses: Chart[] = [
    barChart,
    columnChart,
    lineChart,
    xyLine,
    areaChart,
    yScatter,
    xyScatter,
    ohlc,
    candlestick,
    treemap,
    sunburst,
    heatmap,
];

So even though the type isn't defined on the functions themselves, they are "checked" on the chartClasses array type constraint. Is this enough for now? I dunno, curious what others think.

[texodus]

Is this enough for now? I dunno, curious what others think.

As I alluded to in the first comment, this work needs to eventually be all-or-nothing. Whether this is "enough" is the wrong question to ask - the real question is, what standard do you want to set for the developers who contribute after you? If someone else changes this code in ways that reduces type safety but the contributor thinks its enough and you don't, what do I merge? For a project of this scale, the only fair answer is that we encode our standards in tests, lint, CI, type-checking as much as we possibly can.

As I mentioned in my subling comment wrt to the constructor type of SunburstSeries, while this PR adds a lot to the present developer experience, I am concerned that it is highly susceptible to bit-rot. Take this thread for example - Chart constraint only produces errors on this line, not in the definition of e.g. a mis-written barChart function; within that file, you can rename plugin (as it is marked nullable ? in the type), add mystery fields, and do all sort of "broken" things without causing an error.

A good type system needs to prevent future developers from modifying the code incorrectly, not just provide intellisense for what is already written.

[texodus]

Ok but ...then why catch the error? Printing and exiting with code 1 is the default behavior for an unhandled exception in this function. This only sticks out to me because we do this verbatim in perspecitve-viewer without issue.

[texodus]

These can also be Date or boolean

[texodus]

Looks good!

Much better with the removal of GetSetReturn :)