-
Notifications
You must be signed in to change notification settings - Fork 2
Update to TS 5 #1875
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
Update to TS 5 #1875
Conversation
… of update to TS5
also removed obsolete @typescript-eslint/no-implicit-any-catch disable comments and changed catch clause types from `any` to `unknown`.
suppressImplicitAnyIndexErrors flag and address errors
tsconfig.json
Outdated
| "types": ["vite/client"] | ||
| }, | ||
| "include": ["src", "deps/flow/flow.d.ts", "deps/supabase/types.d.ts"], | ||
| "include": ["src", "deps/flow/flow.d.ts", "deps/supabase/types.d.ts", "__mocks__/vitest-env.d.ts"], |
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'm new to vitest - is this a safe change to make?
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.
That is a great question... I will have to do some digging.
My initial thoughts it no. Reason being it feels wonky to me to include mock stuff here.
| "node_modules/dayjs": { | ||
| "version": "1.11.19", | ||
| "resolved": "https://registry.npmjs.org/dayjs/-/dayjs-1.11.19.tgz", | ||
| "integrity": "sha512-t5EcLVS6QPBNqM2z8fakk/NKel+Xzshgt8FFKAn+qwlD1pzZWxh0nVCrvFK7ZDb6XucZeF9z8C7CBWTRIVApAw==", | ||
| "optional": true, | ||
| "peer": true | ||
| }, |
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 getting removed is an issue. Even though we do not consume it our mui date input does.
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.
What version of NPM and Node are you running / ran while you did this work?
No longer need to manually include types in tsconfig
Marking the type we are expecting
the typing too much here
travjenkins
left a comment
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 - pushed some changes that were very nit picky.
Issues
https://github.com/estuary/ui/issues/issue_number
Changes
issue_number
Tests
Manually tested
Automated tests
Playwright tests ran locally
Screenshots
If applicable - please include some screenshots of the new UI