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

Allow multiple brands and add Unbranded utility #88

Merged
merged 12 commits into from
Sep 17, 2023

Conversation

zkulbeda
Copy link
Contributor

fixes #87

I found UnionToIntersection type in keyof.ts and decided to just copy it in brand.ts. Should it imports type for code deduplication?

@netlify
Copy link

netlify bot commented Aug 11, 2023

Deploy Preview for valibot ready!

Name Link
🔨 Latest commit 6dcab51
🔍 Latest deploy log https://app.netlify.com/sites/valibot/deploys/6506714076cbdb0008961d77
😎 Deploy Preview https://deploy-preview-88--valibot.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -633,6 +672,20 @@ export function brand<
/**
* Brands the output type of a schema.
*
* @example
Copy link
Owner

Choose a reason for hiding this comment

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

To keep the source code as simple as possible, I prefer not to add examples. I will add examples in the future to the API reference on the website.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll delete it

expectTypeOf<BrandedPostId>().not.toMatchTypeOf<BrandedUserId>();
});

test('should support unbranding', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

Why is unbranding and this test necessary? Is there a practical use case for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It's for practical use case.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain the practical use case? Does Zod have this feature?

Copy link
Contributor Author

@zkulbeda zkulbeda Aug 12, 2023

Choose a reason for hiding this comment

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

I was using it to create a mapper to convert database unbranded types from ORM to branded types without schema parsing (because it was already validated on insert step) with type safety (database row should match unbranded schema). Like imagine you have a large object schema with branded fields and you don't want to manually create an object type with the same field's types but unbranded. So you could write typescript generic that unwraps all fields with Unbranded utility. Actually, it's not required, but convinient for a user. The user may write Unbranded by themselves, because I exported BrandSymbol, so if you will decide to remove it, it can take a good place in the method's docs hints.

* @example
* Unbranded<string & Brand<'foo'> & Brand<'bar'>> = string
*/
export type Unbranded<BrandedType> = BrandedType extends infer T &
Copy link
Owner

Choose a reason for hiding this comment

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

If the newly added types are only needed in the tests, they should not be in the brand.ts file, but directly under brand.test.ts. Or alternatively under types.ts in the brand folder if there is a practical use case for it outside the library code.

Copy link
Contributor Author

@zkulbeda zkulbeda Aug 11, 2023

Choose a reason for hiding this comment

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

Yes, It's for outside code. Should I export the file types.ts from index.ts in the folder?

@fabian-hiller
Copy link
Owner

I will review the PR next week.

@fabian-hiller fabian-hiller self-assigned this Aug 13, 2023
@fabian-hiller fabian-hiller added the enhancement New feature or request label Aug 13, 2023
@fabian-hiller
Copy link
Owner

I have now checked your code. We can extend the Brand type so that brand can be applied multiple times. However, currently I would not add Brands and Unbranded as it complicates Valibot's source code and testing and I don't quite understand the benefit yet. I have not found any issue or PR at Zod in this regard. So I assume that this feature is rarely needed. If you disagree here, please let me know.

@zkulbeda
Copy link
Contributor Author

zkulbeda commented Sep 8, 2023

I see. Anyway, please, stay exporting brand symbol for possibility of implementing these types by user themselves. I will edit this pr later, if you don't do this sooner.

@fabian-hiller fabian-hiller merged commit ae97a42 into fabian-hiller:main Sep 17, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple brands schema results to never
2 participants