DEC-623: Switch from joi to zod
#117
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
luixo
force-pushed
the
refactor/unify-types
branch
from
ba4f89f
to
a084599
Compare
|
was hoping to get started reviewing this while waiting on updates to the previous PR, but I am unable to run the backend. Getting the following error |
common/types/core/projects.schema.ts
Outdated
| }), | ||
| }; | ||
|
|
||
| export const outputs = { |
There was a problem hiding this comment.
is there a need to use Zod for our outpout types? It is valuable for input checking but for outputs I think we could just return normal types based on Prisma
There was a problem hiding this comment.
There is no direct need for that.
I decided to do that to keep input and output types inference unified.
When constructing a new handler or changing an old one - it is way easier to wrap head around the same type structure.
Also, it will help keep flavored types strict.
| export type Flavored< | ||
| T extends string, | ||
| R extends string | number = string, | ||
| > = R & { | ||
| __flavor?: T; | ||
| }; | ||
|
|
||
| export const flavored = <T extends string, R extends string | number>( | ||
| x: R, | ||
| ): x is Flavored<T, R> => true; |
There was a problem hiding this comment.
can you explain the flavored types and their value please?
There was a problem hiding this comment.
found some context in this issue but would like to hear the motivation from your perspective
There was a problem hiding this comment.
Flavored types catches error when we pass a string value that is a key (say, project slug) to the function that consumes a string, but of different type (say, contract slug).
I chose flavored over branded because we would have to wrap around a lot of actual branding to run those while flavored are lightweight and there is no need in heavy lifting.
I suggest to check out if migrating to branding types is going to actually be easy, it could be a more strict check on passing around string'd types.
luixo
force-pushed
the
refactor/unify-types
branch
from
6a22892
to
bdd303c
Compare
luixo
changed the base branch from
refactor/unify-types
to
refactor/pct-num-account-balance
|
I factored out a lot of PRs from here so this one is now only about actual validating library switch |
luixo
force-pushed
the
refactor/pct-num-account-balance
branch
from
f3c0ad1
to
b169e52
Compare
luixo
changed the base branch from
refactor/pct-num-account-balance
to
develop
|
I'll merge this as soon as the last prerequisite is merged, there is our last chance to overview the migration! |
| @@ -152,7 +153,7 @@ export class TriggeredAlertsService { | |||
| slug, | |||
| name: alert.name, | |||
| alertId: alert.id, | |||
| type: this.alertsService.toAlertType(rule), | |||
| type: this.alertsService.toAlertType(rule) as Alerts.RuleType, | |||
There was a problem hiding this comment.
No idea why I had to add this though toAlertType returns exactly this type :(
| // { from: number, to: number } | | ||
| // { from?: number, to: number } | | ||
| // { from: number, to?: number } | ||
| // to validate this instead? |
There was a problem hiding this comment.
This is a valid question
| | CreateEmailDestinationConfig | ||
| | CreateTelegramDestinationConfig; | ||
| }; | ||
| export const rule = z.union([ |
There was a problem hiding this comment.
This should be discriminatedUnion (it would help debugging the 400s), but refined objects can no longer be used in discriminatedUnion (it was fixed here, but not released yet).
| }; | ||
| const enabledDestination = databaseDestination | ||
| .pick({ id: true, name: true }) | ||
| .merge( |
There was a problem hiding this comment.
Generally, you want to use merge to have an intersection of two objects, because and is "dumb", but intersection may also work.
Be cautious with these.
luixo
force-pushed
the
refactor/zod
branch
2 times, most recently
from
991a7a6
to
62de68d
Compare
That's a big one with a lot of actual code migration, not only types.
Please look through this carefully.