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

Refactoring: No Implicit Any #290

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Refactoring: No Implicit Any #290

merged 1 commit into from
Oct 31, 2023

Conversation

rokotyan
Copy link
Contributor

@rokotyan rokotyan commented Oct 17, 2023

Enabling noImplicitAny checks in tsconfig. Overall that should improve the code quality by allowing to catch potential errors while writing code.

@rokotyan rokotyan requested a review from reb-dev October 17, 2023 20:18
@rokotyan rokotyan marked this pull request as draft October 17, 2023 20:18
@rokotyan
Copy link
Contributor Author

@reb-dev Can you please check the following TS errors in Nested Donut?
SCR-20231018-mtrv

My initial thought was that we need to pass an array to HierarchyRectangularNode but that generates even more errors.
SCR-20231018-mtnh

@rokotyan rokotyan marked this pull request as ready for review October 18, 2023 21:27
@reb-dev
Copy link
Collaborator

reb-dev commented Oct 19, 2023

@rokotyan Yeah so this is due to a discrepancy between the original data type (returned from d3's hierarchy layout function), and the type we use for Nested Donut (NestedDonutSegment<Datum>). The change occurs after the call to partitionData.each(). Basically,

Before:

typeof node == HierarchyRectangularNode<InternMap<string, number[]>>
typeof node.data == [
    key: string,
    values: InternMap<[string, number[]]> | number[]
]

After:

typeof node == HierarchyRectangularNode<NestedDonutSegmentDatum<Datum>>
typeof node.data == NestedDonutSegmentDatum<Datum>

So no need to change any existing types, it is the logic inside of function _getHierarchyData that needs to be reworked. Maybe creating a separate array of type NestedDonutSegment<Datum>>[] and adding the new nodes to it instead of modifying them in place would help?

@rokotyan
Copy link
Contributor Author

@reb-dev Thanks for the explanation, I understand the problem.

Will you be able to look into it? I've tried to quickly understand data transformations that are happening there but got stuck and didn't want to spend too much time on this. We can probably override the type of node inside the each loop to the correct one like this:

partitionData.each(node => {
  const n = node as unknown as ...
  node.data = {
    key: n.data[0],
    ...

And I would also leave a comment in the code that we're mutating the original data.

@reb-dev
Copy link
Collaborator

reb-dev commented Oct 26, 2023

@rokotyan I've added the changes (and added some comments as well) in a separate PR so I didn't block myself from merging this one. Do you want to merge it and rebase then I'll approve?

@rokotyan
Copy link
Contributor Author

@reb-dev Yeah, let's do it, thanks! Merging now.

@rokotyan
Copy link
Contributor Author

@reb-dev Actually, you'll need to merge it :)

@reb-dev reb-dev merged commit 18e3065 into main Oct 31, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants