-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
refactor(frontend): move utils to TypeScript #9820
refactor(frontend): move utils to TypeScript #9820
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9820 +/- ##
==========================================
+ Coverage 66.16% 70.89% +4.72%
==========================================
Files 585 587 +2
Lines 30427 30455 +28
Branches 3152 3168 +16
==========================================
+ Hits 20133 21590 +1457
+ Misses 10113 8753 -1360
+ Partials 181 112 -69
Continue to review full report at Codecov.
|
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 for the typing! Only one major comment about not needing to export types in most (all?) of these files.
Also, @graceguo-supercat would you mind taking a look since you've worked on most of the dashboard util functions recently? Specifically if the types for isValidChild
look good
Codecov Report
@@ Coverage Diff @@
## master #9820 +/- ##
==========================================
- Coverage 66.16% 64.60% -1.56%
==========================================
Files 585 537 -48
Lines 30427 29412 -1015
Branches 3152 2883 -269
==========================================
- Hits 20133 19003 -1130
- Misses 10113 10230 +117
+ Partials 181 179 -2
Continue to review full report at Codecov.
|
C extends keyof ParentMaxDepthLookup[P] = any, | ||
D extends ParentMaxDepthLookup[P][C] = any | ||
>(child: IsValidChildProps): child is ValidChild<P, C, D> { | ||
const { parentType, childType, parentDepth } = child; | ||
if (!parentType || !childType || typeof parentDepth !== 'number') { |
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.
since we use TypeScript, is type check here still necessary?
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.
It depends, I aimed to keep the behavior the same, taking in an unknown and returning a known type.
This entire function could theoretically be completely removed, using a combo of keyof and generics can statically check most of this:
interface ValidChild<
P extends keyof ParentMaxDepthLookup,
C extends keyof ParentMaxDepthLookup[P],
D extends ParentMaxDepthLookup[P][C]
> {
parentType: P;
childType: C;
parentDepth: D;
}
as an intermediate step, I could set parentDepth
to be number and remove this check.
thoughts? 💭
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.
yes, i feel parentDepth
should be number
type. This intermediate step a little confused me. Thanks!
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.
@graceguo-supercat I removed the generics which were causing confusion.
The request to make parentDepth
a number
and remove the typeof
check causes a cascade affect where two other test suites fail
Test Suites: 2 failed, 186 passed, 188 total
Tests: 17 failed, 4 skipped, 1040 passed, 1061 total
Snapshots: 12 passed, 12 total
Time: 67.338s, estimated 78s
specifically getDropPosition
and isValidChild
both expect the number check.
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.
narrowing parentType
and childType
did turn up a bug in the test suite though.
|
||
if (i > 0 && shouldTestChild) { | ||
// should test child | ||
if (i > 0 && Array.isArray(childType)) { |
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.
Currently type narrowing using Array.isArray
is limited to when the it is inside an if
condition
if (typeof parentType !== 'string') | ||
throw TypeError('parent must be string'); |
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.
Type narrowing needed due to mixed string
and string[]
in the array, this should never throw
it(`(${exampleIdx})${getIndentation( | ||
childDepth, | ||
)}${parentType} (depth ${parentDepth}) > ${childType} ❌`, () => { | ||
expect( | ||
isValidChild({ | ||
parentDepth, | ||
parentType, | ||
childType, | ||
childType: childType[0], |
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 wasn't quite testing what it said it was testing, it was seeing if an array matches the string literal at a given level.
This fixes the tests to compare strings to strings.
@ChristianMurphy thank you so much!! I feel this shows the actual significance of converting js to Typescript: find the potential logic flows in our code. Do you think this PR is ready to merge? |
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.
LGTM
I think so ✔️ |
* refactor(frontend): move utils to typescript (apache#9101) * refactor(frontend): don't export interfaces * test(frontend): update types and test for isValidChild
SUMMARY
Migrate several utility functions to typescript
TEST PLAN
Pass tsc and test scripts
ADDITIONAL INFORMATION