spiermar / d3-flame-graph

A D3.js plugin that produces flame graphs from hierarchical data.
Apache License 2.0
880 stars 119 forks source link

TypeScript: confusion regarding StackFrame type #187

Open nickgirardo opened 3 years ago

nickgirardo commented 3 years ago

Describe the bug The type for StackFrame, as described in here in index.d.ts is

interface StackFrame {
    name: string;
    value: number;
    children: StackFrame[];
};

This looks fine, however your sample JSON file does not conform to this type. Notice how in the type definition children is an array of StackFrame. In the example file, children is omitted if a frame has no children. To conform to stacks.json, the type of Stack frame would be more like

interface StackFrame {
    name: string;
    value: number;
    children?: StackFrame[];
};

While this difference is minor, it is still important. For one example of how this matters, consider the following tooltip:

const tip = defaultFlamegraphTooltip()
    .html((d: { data: StackFrame }) => `name: ${d.data.name}, children: ${d.data.children.length}`);

In this example I have a tooltip which states the name of an item and how many immediate children it has. TypeScript does not complain about this, however if this is ran on a JSON file with children omitted it will produce a runtime error when a item with no children is moused over: d.data.children is undefined. Because TypeScript thinks StackFrame will always have an array of children it did not stop us from writing this broken code.

The following trivial example file

{
    "name": "root",
    "value": 9999,
    "children": []
}

will not produce this error, however

{
    "name": "root",
    "value": 9999
}

which is identical aside from omitting the children property (as in your example files) will.

To Reproduce If the above is not sufficient and you would like me to write out steps to reproduce just comment and I'll get them set up for you

Expected behavior I would expect one of two things to happen:

  1. The example files conform to the type expressed in index.d.ts, meaning that the empty children properties are not omitted.
  2. The type for StackFrame is changed such that children is marked as optional. I believe this might constitute a major/ breaking change as it may cause previously compiling projects to be no longer valid.

Desktop (please complete the following information):

spiermar commented 2 years ago

Hey @nickgirardo Thanks for the note. I generally don't use a TypeScript compiler, so frequently miss updating the ts definition. Feel free to send a PR with the changes for both open issues. Agree with both and happy to merge.