feat: Implement standard schema interface #2258
Conversation
logaretm
force-pushed
the
feat/implement-standard-schema
branch
from
70eb7f4 to
9ca5949
Compare
|
I see this and appreciate your work here, i'm trying to get some time to dedicate to looking at it! |
| expect(verifyStandardSchema(object())).toBe(true); | ||
| expect(verifyStandardSchema(date())).toBe(true); | ||
| expect(verifyStandardSchema(mixed())).toBe(true); | ||
| expect(verifyStandardSchema(tuple([mixed()]))).toBe(true); |
There was a problem hiding this comment.
Added it, along with a special test for it.
src/schema.ts
Outdated
| @@ -202,6 +214,11 @@ export default abstract class Schema< | |||
| this.withMutation((s) => { | |||
| s.nonNullable(); | |||
| }); | |||
|
|
|||
| this['~standard'] = createStandardSchemaProps< | |||
There was a problem hiding this comment.
this won't work correctly, b/c it closes over the schema instance. Most method calls in yup create new schema instances via cloning and the validate method won't keep up with them, e.g. string().required()['~standard'].validate will be the nonRequired schema instance
There was a problem hiding this comment.
You are right, I re-implemented them as getters now which should carry over well with cloning. I added a couple of tests to test that as well.
| // Track if we're inside quotes (for property names with special chars) | ||
| let inQuotes = false; | ||
|
|
||
| for (let i = 0; i < path.length; i++) { |
There was a problem hiding this comment.
what is going on with path logic? There are already utilities for splitting and traversing yup's path's in utils, see the reach utility
There was a problem hiding this comment.
Here I'm not traversing the schema path which is what I think reach and getIn are for?
What I'm doing here is I'm converting error path from as single string value to an array of strings/symbols which is what the standard schema interface requires.
For example these are the output paths from ValidationErrors and next to them is how they should look like in standard schema:
| Error Message Path | Array Format |
|---|---|
| obj.foo | ['obj', 'foo'] |
| obj["not.obj.nested"] | ['obj', 'not.obj.nested'] |
| arr[0].foo | ['arr', '0', 'foo'] |
| arr[0]["not.array.nested"] | ['arr', '0', 'not.array.nested'] |
| ["not.a.field"] | ['not.a.field'] |
I scanned the codebase quickly, do we have utilities for this kind of thing?
| segments.push(currentSegment); | ||
| } else { | ||
| // Handle quoted property names (e.g. obj["foo.bar"]) | ||
| segments.push(currentSegment.replace(/^"|"$/g, '')); |
There was a problem hiding this comment.
i don't think yup even handles these cases, i'm not sure it makes sense to do it here, it's misleading, b/c validate will still likely treat these as actual path segments
There was a problem hiding this comment.
Right, I didn't really encounter this in all my time using yup so I didn't consider if it was supported or not. What do you think I should do then?
There was a problem hiding this comment.
can we add lot more diverse and exhaustive tests around this interface?
There was a problem hiding this comment.
Absolutely, I'm relying on the other parts working as expected because this only adapts the interface to yup's current implementation so I was only testing that layer.
Happy to add more tests, what kind of tests would you like to see? General ones for each schema?
|
Thanks for taking a look, I addressed some of the issues and added clarification to your comments on the path stuff. Happy to keep iterating with your guidance. |
What
This is a PR that implements the standard schema interface for yup.
Motivation: Standard schema is an initiative that is aimed to get all Typed schema providers to support a unified interface that will enable various tools and 3rd party libraries to seamlessly integrate with "a schema provider" which reduces friction when migrating from one provider to the other, and allows mixing these providers if one is suited for a task more than the rest. Another benefit is it allows 3rd party authors to ditch the "resolver" pattern which in turn reduces their users' friction.
We discussed this briefly in #2255 with no clear green light given on this, so I understand if this is something you don't want the burden of maintaining. I already did the work locally to play with it, since I was planning to build a yup standard schema adapter anyways. So, I thought I might as well just PR it.
I made sure to add tests and type tests as well, happy to address any issues in this PR.
How
To avoid adding
@standard-schema/specas a dependency, I copied the types which is the recommended approach. I then make sure it is aligned with the spec by testing it against the spec which I added as a dev dependency.I also had to implement a path splitter to make the paths compatible with the standard schema output.
Bundle size diff (~3%)
closes #2255