Different typing for SafeParseReturnType<T>

[scarletquasar]

Behavior description

Nowadays, the safeParse and safeParseAsync - functions for parsing Zod schemas - return types are based on SafeParseReturnType<T>, this type is an union containing the following types:

• SafeParseSuccess<Output>

  export declare type SafeParseSuccessOutput<Output> = {
    success: true;
    data: output;
  }
  

• SafeParseError<Input>

  export declare type SafeParseError<Input> = {
    success: false;
    error: ZodError<Input>;
  }
  

Behavior problem

This kind of behavior, besids applying bad practices by design - here I'm talking about having const values true and false for success instead of a boolean - also needs an explicit declaration on what type it should be. This situation causes more complexity in the user code - complexity that should be handled by the library itself and it's against the TypeScript idea of good inference and usability.

Solution suggestion

My solution, for that case, would be implementing a single typing for SafeParseReturnType<T> without unions - in a way that will not break existing code and will correctly adapt to any situation that the previously implemented unions covered.

Code:

export declare type SafeParseResult<InputOrOutput> = {
  success: boolean,
  data?: InputOrOutput,
  error?: ZodError<InputOrOutput>
}

[jaens]

As far as I see, the suggestion is exactly the opposite of TypeScript good practices - it incorrectly describes the return type and does not enable the standard practice of type narrowing based on eg if (result.success) { use(result.data); }.

Using literals as TypeScript types is a best practice - not sure where you are getting the nonsense of the opposite being the case from...

You are just using TypeScript wrong.

The correct expression to use in your screenshot is error: item.success === false ? item.error : undefined or error: "error" in item ? item.error : undefined. Otherwise the code really is trying to access an undefined property (error actually does really not exist in that case) for which TypeScript correctly raises an error. If you object to that, I suggest not using TypeScript with the default strictness options. Yes, it is ergonomically slightly unwieldy. Yes, every application or library WILL have the same problem and will need to use the same workaround. This is just standard TypeScript. Nothing to do with this library. This library does everything properly.

[schicks]

I think there's a middle path here, where we keep the safety of the current type (which is the main reason I use zod) but improve the ergonomics so that it can be used more flexibly.

export declare type SafeParseSuccessOutput<Output> = { 
    success: true; 
    data: output; 
    error? : never
}

export declare type SafeParseError<Input> = { 
    success: false; 
    error: ZodError<Input>; 
    data? : never
}

The never fields do not affect safety; this is still a tagged union that can be refined by checking success. However, it can also immediately be used as result.data ?? default, or in other patterns where we legitimately don't care which side of the union we have because we can handle the undefined case. This avoids writing a lot of intermediate variables that are currently necessary to address such cases.

[jaens]

Yes, that is a nice approach, it's a pattern used by other libraries as well, somehow forgot to write about it.

It can be made slightly "better" by using a base type (not really relevant for this small type, but having two completely disjoint types breaks eg. the automatic field rename refactoring which is relevant for bigger types)

eg. something like:

type SafeParseBase = {
	success: boolean;
	data?: never;
	error?: never;
}
  
export declare type SafeParseSuccessOutput<Output> = SafeParseBase & { 
	success: true; 
	data: output; 
}

export declare type SafeParseError<Input> = SafeParseBase & { 
	success: false; 
	error: ZodError<Input>; 
}

Should probably actually use interface & extends ...

[throw5way]

@scarletquasar

The type Zod implements is a disjoint union type. That's how disjoint union types are implemented. It's the only correct type in this situation.

Proposed type has 8 possible values instead of 2, and is just wrong.

Please consult TS documentation or any other literature on how to properly apply bad practices in your code.

[schicks]

Again, not the only correct type; the version with never branches also has only 2 distinct values (ignoring degenerate cases of explicit vs implicit undefineds), and solves the usability problem @scarletquasar is having.

[colinhacks]

As others have said, this is intended to be a discriminated union and it dramatically improves DX due to TypeScript's automatic type narrowing.

[schicks]

@colinhacks the suggestion I made in the PR is still a discriminated union and in addition to the DX benefits of that also allows for better DX when the user needs to respond with something even if parsing failed. This is a fairly common case and even came up in the tests for safeParse. Does that seem like a reasonable change?