-
Notifications
You must be signed in to change notification settings - Fork 23
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
chore: Convert to TS - services charts, user, orgUploadToken, image #3440
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3440 +/- ##
==========================================
- Coverage 99.10% 99.10% -0.01%
==========================================
Files 805 804 -1
Lines 14134 14125 -9
Branches 3968 3961 -7
==========================================
- Hits 14007 13998 -9
Misses 118 118
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3440 +/- ##
==========================================
- Coverage 99.10% 99.10% -0.01%
==========================================
Files 805 804 -1
Lines 14134 14125 -9
Branches 3961 3968 +7
==========================================
- Hits 14007 13998 -9
Misses 118 118
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3440 +/- ##
==========================================
- Coverage 99.10% 99.10% -0.01%
==========================================
Files 805 804 -1
Lines 14134 14125 -9
Branches 3968 3961 -7
==========================================
- Hits 14007 13998 -9
Misses 118 118
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3440 +/- ##
==========================================
- Coverage 99.10% 99.10% -0.01%
==========================================
Files 805 804 -1
Lines 14134 14125 -9
Branches 3961 3968 +7
==========================================
- Hits 14007 13998 -9
Misses 118 118
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will decrease total bundle size by 6.14MB (-34.7%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will decrease total bundle size by 6.14MB (-34.7%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
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.
When we're converting data-fetching/interacting services over to TS, we want to utilize Zod schemas so that we can validate the API response and ensure that we receive the correct data. As well, when the parsing is successful, we get the correct types back and through inference Query will assign those types to the data field etc. from useQuery
.
We have some notion docs that you can take a peak at that walk through this here
Would you mind taking a second pass at these data related hooks and update them to use Zod schemas to validate their responses? If you have any questions around this, feel free to reach out, and we can chat about it.
src/services/pathContents/branch/file/usePrefetchBranchFileEntry.test.tsx
Show resolved
Hide resolved
src/services/pathContents/branch/file/usePrefetchBranchFileEntry.ts
Outdated
Show resolved
Hide resolved
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.
Couple of comments to take a peak at.
src/services/user/useOwner.ts
Outdated
return Promise.reject({ | ||
status: 404, | ||
data: {}, | ||
dev: 'useOwner - 404 failed to parse', | ||
} satisfies NetworkErrorObject) |
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.
Can we use the new rejectNetworkError
helper function?
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.
fixed - was gonna make a sweep on this so you caught it before then - sorry for sloppy state then
thanks for reviewing - it was a mistake to try to do the zod stuff for some of these complicated / high-use hooks in this same PR that already had a bunch of other stuff going on so I'll pull those out here - codecov/engineering-team#2857 Okay so with that this pared down PR is ready again for review! |
@@ -1,30 +0,0 @@ | |||
import { useQuery } from '@tanstack/react-query' |
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.
these files are not used - removed
const selectedOrg = input?.selectedOrg | ||
mutationFn: (input: { formData?: unknown; selectedOrg?: string }) => { | ||
const formData = input.formData | ||
const selectedOrg = input.selectedOrg | ||
|
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.
I'll add zod schema validation in here - codecov/engineering-team#2857
@@ -61,7 +61,7 @@ export function useUpdateProfile({ provider }) { | |||
` | |||
|
|||
return useMutation({ | |||
mutationFn: ({ name, email }) => { | |||
mutationFn: ({ name, email }: { name: string; email: 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.
I'll add zod schema validation here codecov/engineering-team#2857
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
Convert a batch of files to ts
Closes codecov/engineering-team#2721