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

feat: add Valibot support with valibot$ to qwik-city #4998

Draft
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

brandonpittman
Copy link
Contributor

@brandonpittman brandonpittman commented Aug 24, 2023

Overview

Adds Valibot support with a valibot$ function similarly to how Zod is supported by zod$.

Fixes #4924

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

Requires:

  • valibot

Use cases and why

So you can use Valibot to validate your actions

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

@stackblitz
Copy link

stackblitz bot commented Aug 24, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Aug 24, 2023

Deploy Preview for qwik-insights canceled.

Name Link
🔨 Latest commit bfb1d8a
🔍 Latest deploy log https://app.netlify.com/sites/qwik-insights/deploys/651de4314b616a0008a4368f

@gioboa
Copy link
Member

gioboa commented Aug 24, 2023

Thanks for your nice PR, can you add the documentation for this please?

@brandonpittman
Copy link
Contributor Author

brandonpittman commented Aug 24, 2023

Thanks for your nice PR, can you add the documentation for this please?

Sure. Not done, yet. Will update docs tomorrow! Just wanted to share what I had so far.

@fabian-hiller Is there a way with Valibot to check if a value is an ObjectSchema at runtime?

@fabian-hiller
Copy link
Contributor

fabian-hiller commented Aug 24, 2023

@brandonpittman yes, this should work via the .schema key: https://github.com/fabian-hiller/valibot/blob/v0.13.1/library/src/schemas/object/object.ts#L65

And via .async you can find out if it needs to be validated async: https://github.com/fabian-hiller/valibot/blob/v0.13.1/library/src/schemas/object/object.ts#L75

@fabian-hiller
Copy link
Contributor

Note that you must also include BaseSchemaAsync and ObjectShapeAsync to support async validation.

@brandonpittman
Copy link
Contributor Author

Note that you must also include BaseSchemaAsync and ObjectShapeAsync to support async validation.

@fabian-hiller

It's not clear if we need async support. I've updated my PR with working code that satisfies TypeScript. I've removed any async types though. Can you take a look, please?

@brandonpittman brandonpittman marked this pull request as ready for review August 25, 2023 04:37
@brandonpittman brandonpittman marked this pull request as draft August 25, 2023 04:43
@fabian-hiller
Copy link
Contributor

It's not clear if we need async support.

Since the validation is done in an async function, we could enable async validation with safeParseAsync, BaseSchemaAsync and ObjectShapeAsync. This would allow, for example, to do network requests as part of the validation. If someone uses only sync validation, this is not a disadvantage. async is compatible with sync, but sync is not compatible with async.

@fabian-hiller
Copy link
Contributor

If you want to add async support, you need to replace safeParse with safePareseAsync and also add BaseSchemaAsync and ObjectShapeAsync to the types.

@brandonpittman
Copy link
Contributor Author

If you want to add async support, you need to replace safeParse with safePareseAsync and also add BaseSchemaAsync and ObjectShapeAsync to the types.

Okay. Will update it. Thanks.

@brandonpittman brandonpittman marked this pull request as ready for review August 25, 2023 12:32
@fabian-hiller
Copy link
Contributor

You must not remove BaseSchema and ObjectShape from the types. Otherwise the sync validation will not be accepted.

@brandonpittman
Copy link
Contributor Author

brandonpittman commented Aug 25, 2023

You must not remove BaseSchema and ObjectShape from the types. Otherwise the sync validation will not be accepted.

@fabian-hiller

Should I use BaseSchema instead of ObjectSchema<ObjectShape>? When I tried BaseSchema the .schema === "object' check encountered a type error.

Do I need to do something like obj.async ? safeParseAsync(obj, data) : safeParse(obj, data) or can I just give safeParseAsync a synchronous schema?

@brandonpittman brandonpittman marked this pull request as draft August 25, 2023 13:55
@mhevery
Copy link
Contributor

mhevery commented Aug 25, 2023

Thanks for working on this! ❤️

@fabian-hiller
Copy link
Contributor

Should I use BaseSchema instead of ObjectSchema<ObjectShape>? When I tried BaseSchema the .schema === "object' check encountered a type error.

I think I would write BaseSchema | BaseSchemaAsync | ObjectShape | ObjectShapeAsync. If we recognize that an object with typeof obj._parse === "function" is passed, it is a scheme that can be passed directly in safeParseAsync. Otherwise the object must be passed into objectAsync before we call safeParseAsync.

Do I need to do something like obj.async ? safeParseAsync(obj, data) : safeParse(obj, data) or can I just give safeParseAsync a synchronous schema?

This is not required. safeParseAsync can also handle BaseSchema.

@fabian-hiller
Copy link
Contributor

I will also be reviewing the code thoroughly over the weekend. This issue has the highest priority now. I would like to have it merged before my next big annoucnment next week or the week after.

@brandonpittman
Copy link
Contributor Author

@mhevery

  1. The first problem I'm having is in packages/qwik-city/runtime/src/types.ts. The return type from valibot$ seems to be returning the same thing that zod$ does. There are a bunch of nested types for the action constructor and I never could figure out how to separate the Zod types so that I could have Valibot types be accepted as well.
  2. The second problem is that Valibot's FlatErrors type can't take a generic. Even if I could get valibot$ to type hint the return so that flatten()'s nested key was hinted, the contents of nested can't be passed along.

@mhevery
Copy link
Contributor

mhevery commented Sep 26, 2023

I will have a look, sorry it is taking me so long to get to this...

@fabian-hiller
Copy link
Contributor

I won't get to my review until next weekend. Last weekend I did not find time for it. 😕

@fabian-hiller
Copy link
Contributor

Zod has a couple helpers for inferring flattened error types.

Thank you for the hint. I will add such a type to Valibot too.

@fabian-hiller
Copy link
Contributor

@brandonpittman do you prefer that I edit the PR directly (if I get permission to do so) or add comments to your code via GitHub review instead?

@brandonpittman
Copy link
Contributor Author

brandonpittman commented Oct 1, 2023

@brandonpittman do you prefer that I edit the PR directly (if I get permission to do so) or add comments to your code via GitHub review instead?

@fabian-hiller Please edit the PR.

@fabian-hiller
Copy link
Contributor

Thank you for the hint. I will add such a type to Valibot too.

The next version of Valibot will bring improved types for flatten and FlatErrors.

@fabian-hiller
Copy link
Contributor

Valibot v0.18.0 is now available. You can type flatten and FlatErrors using a generic similar to Zod.

@gitguardian
Copy link

gitguardian bot commented Oct 4, 2023

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
- Generic High Entropy Secret bfb1d8a packages/docs/src/routes/demo/cookbook/mediaController/index.tsx View secret
- Generic High Entropy Secret bfb1d8a packages/docs/src/routes/demo/cookbook/mediaController/index.tsx View secret
- Generic High Entropy Secret bfb1d8a packages/docs/src/routes/demo/cookbook/mediaController/index.tsx View secret
- Generic High Entropy Secret bfb1d8a packages/docs/src/routes/demo/cookbook/mediaController/index.tsx View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@mhevery
Copy link
Contributor

mhevery commented Oct 4, 2023

  1. The first problem I'm having is in packages/qwik-city/runtime/src/types.ts. The return type from valibot$ seems to be returning the same thing that zod$ does. There are a bunch of nested types for the action constructor and I never could figure out how to separate the Zod types so that I could have Valibot types be accepted as well.

So I tried to build it locally but the demo files are failing to build. Something is off from your types...

Not sure how you are building it but here is my process

  1. open packages/docs/package.json
  2. change to "@builder.io/qwik-city": "wqrkspace:*",
  3. from the project root run npm build
  4. Notice build errors is packages/docs/src/routes/demo/valibot/advanced/index.tsx and packages/docs/src/routes/demo/valibot/simple/index.tsx

What kind of help do you need from me to move this forward?

@fabian-hiller
Copy link
Contributor

I'll probably finally get around to thoroughly reviewing this PR this weekend. I will then commit directly to the PR and make improvements. I hope that this is possible without special permissions, since I am not an official maintainer of the Qwik repo.

@fabian-hiller
Copy link
Contributor

@brandonpittman a update for you. I have not forgotten about this PR and will finish it as soon as possible. We are currently working on a major structural change to Valibot. As soon as this is done, I will working on this PR.

@wmertens
Copy link
Member

So the current state is that @fabian-hiller will make changes when he finds the time, right?

@fabian-hiller
Copy link
Contributor

Yes, this is the current state. But everyone is welcome to continue working on it before I find the time. In about 3 weeks I will be able to fully focus on my open source work again and will probably finish the PR before Christmas.

@fabian-hiller
Copy link
Contributor

I plan to work on and finalize this PR by the end of next week.

@fabian-hiller
Copy link
Contributor

I am very sorry that this is taking so long. I expect to finalize this PR next week.

@shairez
Copy link
Collaborator

shairez commented Jan 22, 2024

Thanks @fabian-hiller ! can't wait!

@fabian-hiller
Copy link
Contributor

Other things came up that I wanted to preordain. This PR is on my list for the next two weeks.

@fabian-hiller
Copy link
Contributor

fabian-hiller commented Feb 6, 2024

There is a Valibot PR I want to review and maybe merge before I finish this implementation because some types may change. I will give an update here in a few days.

@fabian-hiller
Copy link
Contributor

Sorry to push this implementation back again. You may have seen this discussion on Twitter or GitHub. I plan to work on this first, as some types may change.

@fabian-hiller
Copy link
Contributor

Short update: I am basically rewriting Valibot at the moment. I am trying to improve the API design, performance, bundle size and types, but this will take some time. I also have to rewrite the documentation. I don't want to make any more promises about the timing of this PR. As soon as I feel ready, I will finish the implementation.

@wmertens wmertens marked this pull request as draft March 17, 2024 07:48
@brandonpittman
Copy link
Contributor Author

brandonpittman commented Mar 17, 2024

The funny thing is: the implementation works. It’s just the types I wanted help with. I didn’t understand the types the original implementation had—because they assumed Zod was being used.

@fabian-hiller
Copy link
Contributor

I expect a lot of breaking changes for the next release. So even though the implementation of this PR is almost done, it might make sense to wait to not confuse people.

@brandonpittman
Copy link
Contributor Author

brandonpittman commented Mar 29, 2024

@fabian-hiller Can you tell me what is going to change with Valibot that might affect this? The feature worked, it’s just the types that didn’t work.

I’m gonna take a stab at fixing the types again. (Making the action types less Zod-specific in another PR first might be better though.)

@wmertens
Copy link
Member

@brandonpittman I spoke with him recently, there's a lot of changes to the API coming, as I understand it. So that zod decoupling first send like a good idea

@fabian-hiller
Copy link
Contributor

fabian-hiller commented Mar 29, 2024

The API will be mostly the same (except for the new pipe function), but the types will change and lead to a breaking change for the valibot$ implementation. Therefore, I recommend adding the valibot$ API to Qwik once I am done with the rewrite and start preparing our v1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP: qwik-city TYPE: enhancement New feature or request WAITING FOR: team Waiting for one of the core team members to review and reply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[✨] Valibot Adapter for Qwik
7 participants