Skip to content

Conversation

Myzel394
Copy link
Collaborator

@Myzel394 Myzel394 commented Nov 22, 2023

Pre-Review

Mainly inspired by the typescript generator; added some extra zod related stuff

@Myzel394 Myzel394 requested a review from psteinroe November 23, 2023 10:47
Copy link

@psteinroe psteinroe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few minor nits, and can you run prettier?

Comment on lines 235 to 236
'json',
'jsonb',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is json a string?

Comment on lines 247 to 249
if (["date", "time", "timetz", "timestamp", "timestamptz", "timestamp with time zone"].includes(pgType)) {
return 'date()'
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not correct. dates must be strings. for with timezone types we also need to set the offset parameter to true.

https://zod.dev/?id=iso-datetimes

methods.push(`regex(/${UUID_REGEX}/)`)

if (column.default_value === "gen_random_uuid()") {
methods.push("default(uuidv4)")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not add default values to the zod schema -> they are added in the BE anyways.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise you might have a case where someone queries the backend without the id column, passes it through the zod schema and then has a wrong id set.


// UUID
if (column.format === "uuid") {
methods.push(`regex(/${UUID_REGEX}/)`)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not .uuid()?

// Date and time types
if (["date", "time", "timetz", "timestamp", "timestamptz"].includes(column.format)) {
if (column.default_value === "now()") {
methods.push("default(() => new Date())")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no default value please

return methods
}

function extractGeneralZodMethods(column: PostgresColumn): string[] {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets rename it to extractNullabesAndOptionals to something better than "general"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's too specific in my opinion


const output = `
import * as z from 'zod'
import { v4 as uuidv4 } from 'uuid'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove (see below)

@Myzel394
Copy link
Collaborator Author

@psteinroe to speed up your review process: You can ignore the "apply linter" commit, as it only applies the linter but doesn't change any logic

@Myzel394 Myzel394 requested a review from psteinroe November 23, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants