-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add Schema Reflection API #211
Add Schema Reflection API #211
Conversation
✅ Deploy Preview for valibot canceled.
|
Thanks a lot for this PR. Unfortunately, I don't have time to read everything right now. However, this comment I wrote this morning seems to be related. As soon as I find the time, I will read everything carefully and answer you in detail. |
library/src/validations/ip/ip.ts
Outdated
: getOutput(input); | ||
const kind = 'ip' as const; | ||
const requirement = | ||
/^((((25[0-5]|(2[0-4]|1\d|[1-9]|)\d)\.?\b){4})|((([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))))$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I combined the regex expressions into a single value. It's a little hard to read but essentially it's just introducing an |
OR between the two so that this can be exposed as a single requirement value instead of two.
Just wanted to pop in with an update: Since opening this I started working on a refactor of In working on this I realized that the |
Thanks a lot for your contribution! I will answer you by the weekend at the latest. |
No rush! |
Can you comment on my thoughts in this comment? Instead of a function with properties, what do you think about choosing a structure similar to the schemas using an object with a |
This is my answer on #191 (comment) If a validation function returns an object with According to my idea, export function minLength<TInput extends string | any[]>(
requirement: number,
message: ErrorMessage = 'Invalid length'
): MinLengthPipe<TInput> {
return {
kind: 'validation',
name: 'min_length',
requirement,
message,
_parse(input) {
return input.length < requirement
? getPipeIssues('min_length', message, input)
: getOutput(input);
},
};
} Calling const schema = {
kind: 'schema',
name: 'string',
async: false,
pipe: [{
kind: 'validation',
name: 'min_length',
requirement: 10,
message: 'Invalid length',
_parse(input) {
// ...
},
}],
_parse(input, info) {
// ...
},
}; This is just a first idea. I have not made a decision yet and am open to any ideas. |
I think I'd rather go the opposite direction, get rid of One thing I'm concerned about is that There's also to consider how much refactoring would be required by either approach. It would seem to me like with your proposal it would necessitate additional refactors to every Transform to have a consistent Pipe API. In setting out with this PR, I aimed to be purely additive and not introduce breaking changes to the existing API. So if possible, I'd like to keep the scope of changes here to a minimum. I did some reading on JSON schema to get a better sense of how this might relate to #23, and I don't think that it's a good idea to tightly tie the metadata properties of Schemas, Validations, Transforms, etc to the JSON schema format, but rather pursue writing a schema serializer. You can read up on the JSON schema spec here What is important to keep in mind, I think, is to avoid the use of One such example is I think I could get behind exposing the entire However, I don't think repurposing For now, this is where my thoughts are on where to go from here. tl;dr is that I'd prefer to keep this PR narrowly scoped and save larger refactors for another PR, and that we should be mindful of the tradeoffs at hand |
I think
We won't be able to prevent breaking changes while we're still in v0 and working on major features. I don't care about the effort either, because the best end product is what matters to me.
Why would you consider the JSON schema definition? It doesn't matter for Valibot and doesn't play a role, or am I misunderstanding something here?
I would develop Valibot completely independent of the JSON Schema definition, since serialization is not a relevant feature of Valibot, as it negates Valibot's advantages in terms of bundle size. If serialization and JSON Schema are important, I recommend using TypeBox instead of Valibot.
Yes, I agree, we should take that into account. However, since transformations are part of the Valibot pipeline, developers working with Valibot should be aware of this anyway. Exposing the pipeline as
Yes, with a schema it is not necessarily required. But for the pipeline I think it is, since transformations do not have For now, the disadvantages of
|
Please take a look over the latest commit. I went and refactored everything to use a uniform return shape with a I was going to continue advocating for Important to note, still, is that As far as my JSON schema comments go, you can ignore those. I assumed because of #23 that there was desire to support it somehow with this PR. For reference,
I think it's rather simple to pattern match on unique properties to Schema, Validation, or Transform to determine what type they are. Calling a night on contributions for now. Will catch up on any feedback tomorrow. |
Thank you for your contribution. I'll try to look at it tomorrow and think about it. I expect to give you feedback next week, with the goal of merging the improvements in the following days. |
> [!NOTE] > > The `@internal` TSDoc comment signals that a type is for internal use only. TypeScript can strip these types from the emitted type definitions to hide these internal APIs from third-party developers.
Short update: I try to review everything tomorrow or on the weekend. The goal is to release a new minor version next week. |
I have started to investigate and improve the code. I'll let you know as soon as I'm further along. |
I expect to share my changes here in the middle or end of this week so we can discuss the final details. |
I took a look over your latest commits (da3ca63, 775d539 and 5838643) and everything looks good to me. A concern I have with But otherwise, I like the addition of exposing error messages as well as the validation pipelines. That should help in form input validation. I'm good to ship this if you are! Thanks for all the effort in reviewing @fabian-hiller! |
Thanks a lot for the work you put into this PR! I will wait 1 to 3 days and then do the final review to merge the changes. |
I had a brief look at the changes. I still think that |
@lo1tuma can you describe in detail what you mean (e.g. with some sample code) and how you would improve it? |
What I mean is the problem that on the if (issue.type === 'stars_with') {
someStringOperation(issue.requirement); // requirement is unknown but I would expect type string
} I would need to do further runtime checks to verify what type |
Yes, I agree that we should address this in a follow-up. I plan to integrate Valibot more into Modular Forms and will probably find a lot of necessary improvements to Valibot along the way. |
I will think about your feedback and probably create an issue after this one is merged so that we do not forget about this in the long run. |
Thanks again for creating and contributing to this PR! |
This looks very promising, especially with my idea that Superforms v2 should be able to use most validation libraries through typeschema, in more or less complete versions of the validation format, where Zod is the benchmark, being fully introspectable. I would be happy to use Valibot as the test library for experimentation, so is there an example how to use the Reflection API? This is how I do it with Zod: https://github.com/ciscoheat/sveltekit-superforms/blob/main/src/lib/schemaEntity.ts#L293-L358 It's not difficult, once the metadata is easily accessible. So an example of that, together with how to be able to access some other properties like nullable, optional, default value, would get me very far. |
I think the code for Valibot is very similar to the code for Zod. The biggest difference will be that you extract all the details of a data type from the pipeline. I have created an issue so that we add a guide on this to the documentation in the long run. |
This PR is a proposal implementation for a schema reflection API submitted for consideration and refinement.
Adding a reflection API would enable development of third-party utility libraries for Valibot such as a mock generation library similar to
@anatine/zod-mock
, which is what initially inspired this effort.At present, given Valibot's architecture utilizing anonymous arrow functions as the return values of Validation functions and the lack of access to the given Pipe in the Schema object, it would be difficult if not entirely impossible to gather the required metadata about a Valibot schema definition in order to generate mock data that would pass schema validation. This is because many of the required values, such as function arguments and internal variables, cannot be accessed at all once defined.
In this PR I've added what I believe to be a minimal API which would expose this Schema and Pipe metadata. Let's consider some examples.
First, let's take a look at the Email validation function, which doesn't accept a user-defined requirement:
Note that
kind
,requirement
, andmessage
were all extracted to variables to keep the internals DRY, and thatObject.assign()
is being used to assign each of these values as static properties of the returned validator function.This exposes two of the most critical pieces of information about a validation check: what kind of check is being performed and what condition needs to be met for success.
Since the function being returned here is anonymous, even if we had access to the Pipe array in a Schema, each Pipe Item wouldn't have a
name
property to identify what validation was used. Additionally, because of the security limitations of strict mode, this function'sarguments
property also cannot be accessed.With this setup, we're safely exposing all of this information in a manner which these values can be accessed without risk of modifying the runtime behavior of the validation itself.
Note
Originally I had considered wrapping this "metadata" object in
Object.freeze()
to ensure that it was read-only at runtime, but I don't think it provides much benefit for the performance tradeoff.Let's take a look at one more Validation example:
User-defined requirements are roughly the same except for one important detail:
const TRequirement extends number
, where the use of theconst
modifier on this generic will instruct Typescript to treat the user-supplied value as narrowly as possible. This means that the return type ofminLength()
will show the user-defined values it was supplied rather than a widerrequirement: number
type.Note
For this initial implementation I wanted to introduce as few changes to the existing types as possible, so there's room for improvement still.
Next let's take a look at a Schema function:
New things here are the addition of the
checks
property, which always returns an array ofPipeMeta
objects, which are the values we added to each of the Validation functions. For every Schema that accepts a Pipe argument, achecks
property was added in the return value to expose metadata about the Pipe array.Notably every Transformation was untouched, so they don't provide any metadata.
checks
only contains metadata from Validation functions.Last, here's another Schema example:
Note
After further exploration writing a mocking library based on this PR, I no longer think the
entries
prop /SchemaMeta
type are necessary, so I've removed them.For iterable schemas (intersection
,object
,tuple
, andunion
), there's also a newentries
property, which either returns an array of key/value pairs, or simply aSchemaMeta[]
array. This property helps with introspection on nested schema types.This is what theSchemaMeta
varies by schema, but it breaks down like this:PipeMeta
object looks like:These are all the fields which comprise the reflection API. I think there's room for some further simplification and improvements to the type definitions. Names for properties/types aren't set in stone, so feedback on those is welcomed and expected.
In this first pass I went and added a bunch of new tests to most of the affected schemas to validate the property additions and the a utility function
getChecks()
and.getEntries()
There may be a few things I missed our haven't considered too. My goal here was adding just enough information about schemas to feed into a library such as Faker to generate mock data for tests, which is what I'm currently doing for my
zod
based schemas in@discordkit/client
. I can see this also being useful for other use cases as well, such as JSON Schema or OpenAPI spec code generation tools.Finally I want to acknowledge that I understand that this project is part of your Bachelor's thesis and that there wasn't a contributing guide that I could find, so I understand if you don't have much time for reviewing PRs or would prefer to pursue your own implementation. This was a fun weekend project for me and at the end of the day I'm just hoping this is more inspiring than just opening another feature request issue.