Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(validation-errors): type inference for branded, nullable, optional and array types #284

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

aleclofabbro
Copy link
Contributor

returnValidationErrors() typechecking fails when a property is a primitive-object-intersection type (typical case of branded type).

returnValidationErrors() refers to type SchemaErrors<S> for typings
the purpose of type SchemaErrors<S> as is:

type SchemaErrors<S> = {
	[K in keyof S]?: S[K] extends object | null | undefined 
	  ? Prettify<VEList & SchemaErrors<S[K]>> 
	  : VEList;
} & {};

is to map S properties conditionally:
if property S[K] is object | null | undefined maps to Prettify<VEList & SchemaErrors<S[K]>>
else (property S[K] is primitive) maps to VEList

the issue arises when property S[K] is a branded primitive type.
Branded primitive types are typically an intersection of a primitive with an object holding the brand
(e.g. type email = string & { [k in brand_prop_type]: unique symbol })

type SchemaErrors<S> mapping condition considers branded-primitive-types as object, mapping them incorrectly, and finally leading to returnValidationErrors() type errors

Proposed changes

the issue is resolved by simply reversing the condition:

type SchemaErrors<S> = {
  [K in keyof S]?: S[K] extends number | string | boolean | bigint | symbol
    ? VEList
    : Prettify<VEList & SchemaErrors<S[K]>>;
} & {};

this way any primitive - even when intersected - is eagerly catched and properly managed, keeping logic unaltered

Related issue(s) or discussion(s)

This is an amend of a previous (closed) PR
#277

re #


…emaerrors<s>): brand primitives

flip `type SchemaErrors<S>` mapping conditional to eagerly detect branded (object-intersected)
primitives

amend PR#277
Copy link

vercel bot commented Oct 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-safe-action-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 22, 2024 2:07pm
next-safe-action-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 22, 2024 2:07pm

Copy link

vercel bot commented Oct 21, 2024

@aleclofabbro is attempting to deploy a commit to the Edoardo Ranghieri's projects Team on Vercel.

A member of the Team first needs to authorize it.

@TheEdoRan
Copy link
Owner

@aleclofabbro updated code to fix inference for optionals, nullables and arrays too. Please check it out, lgtm so I think it can be merged.

@TheEdoRan TheEdoRan changed the title support branded primitive types (object-intersected-primitive types) fix(validation-errors): type inference for branded, nullable, optional and array types Oct 21, 2024
@aleclofabbro
Copy link
Contributor Author

@TheEdoRan are you sure | null | undefined | any[] should be added to the condition ?
the original puts together objects, null, and undefined ( S[K] extends object | null | undefined )
having null and undefined to be pushed to the Prettify<VEList & SchemaErrors<S[K]>> branch ,
the same for any[], being it an object
optionals should not be an issue, as all properties are mapped to be optional, and the S[K] extends condition checks for the defined prop type, even for optional properties
makes sense ?

@TheEdoRan
Copy link
Owner

@aleclofabbro yep, while I was working on this I found out that arrays, nullables and optionals weren't typed as expected. In fact, if I remove null | undefined | any[] from the NotObject type, having a schema like this one below, the resulting type of ValidationErrors isn't correct:

image

If I instead include those additional types in the NotObject type, the resulting type of ValidationErrors is correct:

image

@aleclofabbro
Copy link
Contributor Author

aleclofabbro commented Oct 22, 2024

ha! so that was an existent bug you spotted out while checking my PR ? if so, that's simply great, pow-pow !
lgtm too , any action from me ?

@TheEdoRan
Copy link
Owner

@aleclofabbro Yeah! haha. No actions required from you, gonna merge this soon, thank you for your contribution!

@TheEdoRan TheEdoRan merged commit 2e1fe62 into TheEdoRan:main Oct 22, 2024
5 checks passed
Copy link

🎉 This PR is included in version 7.9.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants