refactor: Enabling typescript 5.3.3 in @calcom/trpc#19394
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
emrysal
changed the title
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
| "dompurify": "^3.1.7", | ||
| "dotenv-cli": "^6.0.0", | ||
| "entities": "^4.4.0", | ||
| "eslint-config-next": "^13.2.1", |
There was a problem hiding this comment.
Dependency conflict, having eslint-config-next here causes a version mismatch with calcom/config
| "@calcom/eslint/deprecated-imports-next-router": "off", | ||
| }, | ||
| }, | ||
| { |
| @@ -1,6 +1,6 @@ | |||
| import { z } from "zod"; | |||
There was a problem hiding this comment.
@eunjae-lee heavy refactor of this file to improve TS performance
There was a problem hiding this comment.
Thanks. I love that you put satisfies so that we won't accidentally break it in the future. If z.infer causes bad performance, maybe we could have an eslint warning rule?
There was a problem hiding this comment.
That would warn our entire codebase, z.infer is heavy on TS but it was uniquely heavy here because it was inferring deeply (z.infer<z.infer<z.infer<..)
| @@ -1,80 +0,0 @@ | |||
| import type { | |||
There was a problem hiding this comment.
Long since deprecated but now removed in favour of Suspense
| }, | ||
| "devDependencies": { | ||
| "typescript": "^4.9.4" | ||
| "typescript": "^5.3.3" |
| getScenarioData, | ||
| } from "@calcom/web/test/utils/bookingScenario/bookingScenario"; | ||
| import { getZoomAppCredential } from "@calcom/web/test/utils/bookingScenario/bookingScenario"; | ||
| import { expectSuccesfulLocationChangeEmails } from "@calcom/web/test/utils/bookingScenario/expects"; |
There was a problem hiding this comment.
This caused a huge amount of issues and in fact makes this entire test invalid, needless to say trpc should not depend on calcom/web
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | ||
| import { QueryCell } from "@calcom/trpc/components/QueryCell"; | ||
| import { trpc } from "@calcom/trpc/react"; | ||
| import { Button } from "@calcom/ui/components/button"; |
There was a problem hiding this comment.
Just to understand the purpose, is this import instead of "@calcom/ui" because of TS perf issue? or is there any other reason?
There was a problem hiding this comment.
This is not strictly necessary, but it does improve performance, when you include a file like "@calcom/ui" ts evaluates all files referenced within that file, rather than just in Button. so if you're more specific, performance improves.
eunjae-lee
left a comment
eunjae-lee
left a comment
There was a problem hiding this comment.
Thanks for the hard work!
E2E results are ready! |
* Refactors related to enabling typescript 5.3.3 in @calcom/trpc * Also skip respective tests (obviously logic is broken) * This causes problems for some reason, revert * Bring back QueryCell as it's used by atoms
* Refactors related to enabling typescript 5.3.3 in @calcom/trpc * Also skip respective tests (obviously logic is broken) * This causes problems for some reason, revert * Bring back QueryCell as it's used by atoms
5 participants
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?