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.
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.
since we use TypeScript, is type check here still necessary?
There was a problem hiding this comment.
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.
yes, i feel parentDepth should be number type. This intermediate step a little confused me. Thanks!
There was a problem hiding this comment.
@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.
|
|
||
| if (i > 0 && shouldTestChild) { | ||
| // should test child | ||
| if (i > 0 && Array.isArray(childType)) { |
There was a problem hiding this comment.
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.
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.
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? |
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
mistercrunch
added
🏷️ bot
SUMMARY
Migrate several utility functions to typescript
TEST PLAN
Pass tsc and test scripts
ADDITIONAL INFORMATION