Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScript: confusion regarding StackFrame type #187

Open
nickgirardo opened this issue Sep 12, 2021 · 1 comment
Open

TypeScript: confusion regarding StackFrame type #187

nickgirardo opened this issue Sep 12, 2021 · 1 comment
Labels

Comments

@nickgirardo
Copy link
Contributor

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):

  • OS: Linux
  • Browser: Firefox
  • Version: 90.0
@spiermar
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants