Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(partition): Flame and icicle chart #965
feat(partition): Flame and icicle chart #965
Changes from 22 commits
59eab63
cfb07d7
c961de2
4ebfc69
fc839b2
f0107ed
81132d3
329d618
5ec7718
4e39c41
39e55f8
3f17149
93dcf25
3985768
5774b8d
4df838b
6692738
934fa7b
04e1663
7c1ad36
b33bb86
15dbefd
cc9edc2
eefbbd7
d6a3893
0aa4781
ea378fb
49f2813
1fa0580
f9e51f9
a57b697
5e9559b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a clean up on the PR, but seriously looking at that
getRootArrayNode
function, it doesn't return strictly anArrayNode
, theSTATISTICS_KEY
andSORT_INDEX_KEY
are not available there.It's possible that for the root note these values are ignored in the current code, but that doesn't mean that we can easily remember that fact when refactoring/reworking on that file. It can leads to bug if we believe that also the root node is of type
ArrayNode
Fixing the type here can go in two direction:
Exclude<ArrayNode, parent
This task doesn't need to be handled in this PR, but it's just an example of why ts ignoring or putting as can deal to issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good points!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pondering... the possible issue with points 1 and 3 is that it'll no longer be possible to model the entire tree as having homogeneous type; I'm sure it's solvable with type guards and such, with some more work.
With option 2, it'd relax the type for all uses, and I wouldn't want to water it up.
I think adding the missing properties to this root node would be a compact, local solution, but it'd still need the
as
assertion, because the[PARENT_KEY]
prop demands anArrayNode
.So my current plan is to see what I can do locally (eg. just adding the missing props but leaving in place the
as
) and continue exploring and likely, discussing, outside this PRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK the current solution is, use
Omit
to excludePARENT_KEY
in the assignment:This way, there's still a need for
as
, because TS can't type-bootstrap a self-referential object, but it's way better, because should anything inArrayNode
change, thebootstrap
object must reflect that. So the only part where the type checker is asked to trust the human is the presence ofPARENT_KEY
which is a local, single-line assignment right thereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f9e51f9