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 intersection schema #117

Merged
merged 6 commits into from
Oct 1, 2023
Merged

Conversation

alonidiom
Copy link
Contributor

Fixes #113 (including the specific case mentioned)

@netlify
Copy link

netlify bot commented Aug 23, 2023

Deploy Preview for valibot ready!

Name Link
🔨 Latest commit 94928c8
🔍 Latest deploy log https://app.netlify.com/sites/valibot/deploys/6518eb57629d680008223449
😎 Deploy Preview https://deploy-preview-117--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.

@fabian-hiller
Copy link
Owner

Thank you for your contribution! The next few days I have to focus on my bachelor thesis. As soon as I find time, I'll get back to you.

@fabian-hiller fabian-hiller self-assigned this Aug 27, 2023
@fabian-hiller fabian-hiller added enhancement New feature or request priority This has priority labels Aug 27, 2023
@fabian-hiller fabian-hiller added the question Further information is requested label Sep 8, 2023
@fabian-hiller
Copy link
Owner

fabian-hiller commented Sep 17, 2023

If there is still interest in this PR, please check this comment beforehand and give me feedback on it.

@fabian-hiller fabian-hiller removed question Further information is requested priority This has priority labels Sep 17, 2023
return value;
}) as IntersectionOutput<TIntersectionOptions>,
}
: getIssues(

Choose a reason for hiding this comment

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

In my case validation errors from intersected objects didn't bubble up, and I had to do this:

return !issues && output
        ? {
           ...
          }
        : issues?.length
            ? { issues }
            : getIssues(info, 'type', 'intersection', error || 'Invalid type', input, issues)

Please check this, and if valid this applies to intersectionAsync, too

// If there are issues, set output and break loop
if (result.issues) {
issues = result.issues;
break;

Choose a reason for hiding this comment

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

In my case validation errors from intersected objects didn't bubble up, and I had to do this:

        if (result.issues) {
          issues = result.issues
          while (issues?.length) {
            issues = (issues => issues.length ? issues as Issues : undefined)(issues.flatMap(i => i.issues ?? []))
              ?? issues
            break
          }
          break
        } else ...

Please check this, and if valid this applies to intersectionAsync, too

@fabian-hiller fabian-hiller added the priority This has priority label Sep 18, 2023
@fabian-hiller
Copy link
Owner

I will probably review this PR in the next few days.

@Karakatiza666
Copy link

@alonidiom Please check out my fork of your PR! Karakatiza666@fd1830f
I believe it improves type inference, and behaviour when handling parsing errors

@fabian-hiller
Copy link
Owner

I am currently working on this PR. I'll check your code in a moment.

@fabian-hiller
Copy link
Owner

union is a logical OR. For this reason, the issues are added as subissues, as they usually contradict each other. intersection is a logical AND, so in my opinion this procedure is not necessary here. I have also investigated the implementation of Zod in this regard.

I'm not done yet, but I welcome feedback or questions now before I finalize it.

@fabian-hiller
Copy link
Owner

Apart from that, I also revised the code that merges two or more outputs so that it should now work with every schema.

@Karakatiza666
Copy link

Will check if I encounter issues with new code tomorrow, wasn't able to try earlier

@fabian-hiller
Copy link
Owner

I have finalized the implementation. Feel free to test out the changes and let me know if anything can be improved.

@fabian-hiller
Copy link
Owner

I plan to merge this PR today or tomorrow.

@fabian-hiller fabian-hiller merged commit 77faa84 into fabian-hiller:main Oct 1, 2023
5 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 priority This has priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing intersection helper
3 participants