DX-1543: Introduce Zod Schema #55
Conversation
| @@ -61,7 +62,8 @@ export const processOptions = <TResponse extends Response = Response, TInitialPa | |||
|
|
|||
| // try to parse the payload | |||
| try { | |||
| return JSON.parse(initialRequest) as TInitialPayload; | |||
| const parsed = JSON.parse(initialRequest) as TInitialPayload; | |||
There was a problem hiding this comment.
is it possible to call schema.parse directly on initialRequest?
There was a problem hiding this comment.
No the body should be parsed to json in order to parse with zod, or else zod.string() would match everything, since they're all string without parsing to json
| export type PublicServeOptions< | ||
| TInitialPayload = unknown, | ||
| TResponse extends Response = Response, | ||
| > = Omit<WorkflowServeOptions<TResponse, TInitialPayload>, "onStepFinish" | "useJSONContent">; | ||
| > = Omit<WorkflowServeOptions<TResponse, TInitialPayload>, "onStepFinish" | "useJSONContent"> & ValidationOptions<TInitialPayload>; |
There was a problem hiding this comment.
Let's add ValidationOptions to WorkflowServeoptions
src/serve/options.ts
Outdated
| : undefined, | ||
| baseUrl: environment.UPSTASH_WORKFLOW_URL, | ||
| env: environment, | ||
| retries: DEFAULT_RETRIES, | ||
| useJSONContent: false, | ||
| disableTelemetry: false, | ||
| schema: z.any(), |
There was a problem hiding this comment.
we can add schema to non-required fields https://github.com/upstash/workflow-js/blob/github-workflows/src/serve/options.ts#L24 and not pass anything here
| @@ -236,7 +239,7 @@ export const serve = < | |||
| TResponse extends Response = Response, | |||
| >( | |||
| routeFunction: RouteFunction<TInitialPayload>, | |||
| options?: Omit<WorkflowServeOptions<TResponse, TInitialPayload>, "useJSONContent"> | |||
| options?: Omit<WorkflowServeOptions<TResponse, TInitialPayload>, "useJSONContent"> & ValidationOptions<TInitialPayload> | |||
There was a problem hiding this comment.
we can remove ValidationOptions from here if we add them to WorkflowServeOptions
|
|
||
| // try to parse the payload | ||
| try { | ||
| return JSON.parse(initialRequest) as TInitialPayload; | ||
| const parsed = JSON.parse(initialRequest) as TInitialPayload; |
There was a problem hiding this comment.
since we make initialPayloadParser and schema mutually exclusive, this looks odd. The code looks correct theoretically but looks weird.
I am okay to write it this way but we should add tests:
- what happens if schema and a correct payload (put a spy over schema.parse and make sure it's called)
- what happens if schema and an incorrect payload is passed -> serve should return error
- what happens if I override the type errors, pass schema and initial payload parser? schema should be ignored. this won't happen but we should verify the behavior with tests
There was a problem hiding this comment.
I will be adding tests. I didn't want to distribute the zod parser everywhere. Besides even though they're mutually exclusive on public interface, initialParser always exists internally. That's why it made sense to me
| @@ -233,7 +230,13 @@ export type WorkflowServeOptions< | |||
| * Set `disableTelemetry` to disable this behavior. | |||
| */ | |||
| disableTelemetry?: boolean; | |||
| }; | |||
| } & ValidationOptions<TInitialPayload>; | |||
There was a problem hiding this comment.
@CahidArda I've actually added it there, but somehow they're not available on the PublicServeOptions automatically, that's why i added it again. Couldn't sort out the reason
No description provided.