-
-
Notifications
You must be signed in to change notification settings - Fork 216
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
[INTROSPECTION][DISCUSSION] allow to introspect with BaseSchema & BaseSchemaAsync #258
Comments
Requesting the same thing in #257 essentially. The essential problem here is that we can't really have type guards or build generic utilities that work with these building blocks without proper type inheritance. Admittedly, this is partially my fault for not catching this in the PR review, as this was possible before but got taken out along the way somewhere. |
There is a reason why I did not include To make a long story short. My current recommendation is not to use |
Issue #191 with #191 (comment) is partially related. |
For now i'm adding every single schema definition in a union as it adds typescasting automatically with typescript once you match. But the definition is really ugly. Maybe a type union of all schemas types (like the one i created) could be added to valibot (should not impact treeshaking, since it's type only) Here is the monster : type GenericSchemaAsync =
| AnySchemaAsync
| ArraySchemaAsync<BaseSchema | BaseSchemaAsync>
| BigintSchemaAsync
| BooleanSchemaAsync
| DateSchemaAsync
| EnumSchemaAsync<Enum>
| NullSchemaAsync
| NumberSchemaAsync
| NullableSchemaAsync<BaseSchema | BaseSchemaAsync>
| NullishSchemaAsync<BaseSchema | BaseSchemaAsync>
| BlobSchemaAsync
| VoidSchemaAsync
| NeverSchemaAsync
| SetSchemaAsync<BaseSchema | BaseSchemaAsync>
| MapSchemaAsync<BaseSchemaAsync | BaseSchema, BaseSchemaAsync | BaseSchema>
| ObjectSchemaAsync<
ObjectEntriesAsync,
BaseSchema | BaseSchemaAsync | undefined
>
| StringSchemaAsync
| SymbolSchemaAsync
| UndefinedSchemaAsync
| UnionSchemaAsync<UnionOptionsAsync>
| UnknownSchemaAsync
| LiteralSchemaAsync<Literal>
| NanSchemaAsync
| TupleSchemaAsync<TupleItems>
| RecordSchemaAsync<RecordKeyAsync, BaseSchema | BaseSchemaAsync>
| SpecialSchemaAsync<any>
| VariantSchemaAsync<string, VariantOptionsAsync<string>>
| InstanceSchemaAsync<Class>
| OptionalSchemaAsync<BaseSchema>
| PicklistSchemaAsync<PicklistOptions>
| RecursiveSchemaAsync<() => BaseSchema | BaseSchemaAsync>
| NonNullishSchemaAsync<BaseSchemaAsync>
| NonNullableSchemaAsync<BaseSchema | BaseSchemaAsync>
| NonOptionalSchemaAsync<BaseSchemaAsync | BaseSchema>;
type GenericSchema =
| AnySchema
| ArraySchema<BaseSchema>
| BigintSchema
| BooleanSchema
| DateSchema
| EnumSchema<Enum>
| NullSchema
| NumberSchema
| NullableSchema<BaseSchema>
| NullishSchema<BaseSchema>
| BlobSchema
| VoidSchema
| NeverSchema
| SetSchema<BaseSchema>
| MapSchema<BaseSchema, BaseSchema>
| ObjectSchema<ObjectEntries, BaseSchema | undefined>
| StringSchema
| SymbolSchema
| UndefinedSchema
| UnionSchema<UnionOptions>
| UnknownSchema
| LiteralSchema<Literal>
| NanSchema
| TupleSchema<TupleItems>
| RecordSchema<RecordKey, BaseSchema>
| SpecialSchema<any>
| VariantSchema<string, VariantOptions<string>>
| InstanceSchema<Class>
| OptionalSchema<BaseSchema>
| PicklistSchema<PicklistOptions>
| RecursiveSchema<() => BaseSchema>
| NonNullishSchema<BaseSchema>
| NonNullableSchema<BaseSchema>
| NonOptionalSchema<BaseSchema>;
function valibotSchemaToJsonSchema<T extends GenericSchema | GenericSchemaAsync>(
schema: T
): any {
switch (schema.type) {
case "string":
// handle string schema
break;
case "number":
// handle number schema
break;
case "boolean":
// handle boolean schema
break;
case "object":
// handle object schema
break;
case "array":
// handle array schema
break;
case "null":
// handle null schema
break;
case "any":
// handle any schema
break;
}
} |
Thank you for your feedback. Yes, as written here #191 (comment) I plan to investigate this. |
So, I think I would be okay with a Union export for all of the schema types because it doesn't add any weight to the package since it's just type info. However, I still think extending off of a common root interface is the best way to go, as it is generally more maintainable as this library grows and it is simple to understand assignment with this sort of abstraction. Any potential errors are easier to debug when the types themselves are less complex. With a really large union of objects, it can be difficult to debug a type error when one does crop up. Rather than exporting one swiss-army-knife utility for discriminating between all schemas as with your example with a switch statement above, I think small, individual type predicate functions would be better. The main reason being that you want to tree-shake out as much code here as possible, and you can't shake out unused cases in a switch. I think the changes I've drafted in #259 will cover what you've requested here. In some sense here, even though Valibot is built with a functional architecture, every schema is essentially a constructor function returning an instance of a schema class (albeit with very few properties and no inherited methods). What we're discussing here boils down to Here is an example of what I mean. Here we've deduplicated some of the code, and we could even go further here because with this we no longer need a export class BigintSchema<TOutput = bigint> implements BaseSchema<bigint, TOutput> {
type = 'bigint' as const;
async = false as const;
message: ErrorMessage;
pipe?: Pipe<bigint>;
constructor(arg1?: ErrorMessage | Pipe<bigint>, arg2?: Pipe<bigint>) {
const [message = 'Invalid type', pipe] = getDefaultArgs(arg1, arg2);
this.message = message;
this.pipe = pipe;
}
_parse(input: unknown, info?: ParseInfo): _ParseResult<TOutput> {
if (typeof input !== 'bigint') {
return getSchemaIssues(info, 'type', 'bigint', this.message, input);
}
return executePipe(
input,
this.pipe,
info,
'bigint'
) as _ParseResult<TOutput>;
}
}
// Honestly, these aren't even necessary anymore except to avoid the `new` keyword
export function bigint(pipe?: Pipe<bigint>): BigintSchema;
export function bigint(
message?: ErrorMessage,
pipe?: Pipe<bigint>
): BigintSchema;
export function bigint(
arg1?: ErrorMessage | Pipe<bigint>,
arg2?: Pipe<bigint>
): BigintSchema {
return new BigintSchema(arg1, arg2);
} Would this be a good idea? I don't know. I think there are some clear advantages as far as deduplication of code is concerned, but my guess would be that this feels icky as we'd be throwing in some OOP to a mostly functional library. |
Thanks for your detailed response. Your thoughts are good and I think it might make sense to investigate this approach to discuss the pros and cons in bundle size, performance and developer experience. As for I welcome further feedback from others on this topic. |
I haven't personally run into that issue with And I'm also not quite sure if this odd behavior with |
I just started generating ElasticSearch mappings (basically database schema declarations) from my Valibot schemas, and I ran into this issue as well. I already have schemas defined like this: import { array, date, object, string, uuid } from "valibot";
const UserSchema = object({
id: string([uuid()]),
createdAt: date(),
name: string(),
friendIds: array(string([uuid()])),
}); I want to generate an ElasticSearch mapping from this schema. I intend to use that mapping to execute an ElasticSearch migration. It needs to look like this in the end: import { type MappingTypeMapping } from "@elastic/elasticsearch";
const UserMapping: MappingTypeMapping = {
dynamic: "strict",
properties: {
id: {
type: "keyword",
},
createdAt: {
type: "date",
},
name: {
type: "text",
},
friendIds: {
type: "keyword",
},
},
}; Here is how I approach this conversion so far: function schemaToMapping(
schema: BaseSchema | BaseSchemaAsync
): MappingTypeMapping {
switch (schema.type) {
case "object":
return {
dynamic: "strict",
properties: Object.fromEntries(
Object.entries(schema.entries).map(([k, v]) => [
k,
schemaToMapping(v),
])
),
};
case "array":
// ElasticSearch doesn't index arrays:
// https://www.elastic.co/guide/en/elasticsearch/reference/current/array.html
return schemaToMapping(schema.item);
case "string":
return { type: "text" };
case "date":
return { type: "date" };
default:
throw new Error("Unimplemented");
}
} But right now, the
This approach doesn't seem clean to me, because
I don't understand what you mean. Could you give an example function where this leads to an error? I'm not suggesting you're wrong, I just can't follow the argument. 😅 |
Thanks for your message. I am aware that this is not perfect yet, but it is my goal to find a good developer experience on the long run. Please share any ideas and feedback with me. This is how I would implement it at the moment. If we change the schema types to import * as v from 'valibot';
type MappingSchema =
| v.BaseSchema
| v.ObjectSchema<v.ObjectEntries>
| v.ArraySchema<v.BaseSchema>
| v.StringSchema
| v.DateSchema;
function schemaToMapping(schema: MappingSchema) {
if ('type' in schema) {
switch (schema.type) {
case 'object':
return {
dynamic: 'strict',
properties: Object.fromEntries(
Object.entries(schema.entries).map(([k, v]) => [
k,
schemaToMapping(v),
])
),
};
case 'array':
// ElasticSearch doesn't index arrays:
// https://www.elastic.co/guide/en/elasticsearch/reference/current/array.html
return schemaToMapping(schema.item);
case 'string':
return { type: 'text' };
case 'date':
return { type: 'date' };
default:
throw new Error('Unimplemented');
}
}
} In a perfect world, we could change the schema type as follows. This would make it 100% type safe and also remove the type MappingSchema =
| v.ObjectSchema<MappingSchema>
| v.ArraySchema<MappingSchema>
| v.StringSchema
| v.DateSchema; |
Thanks for the suggestion! You are completely right, my function needs to have a union type parameter, simply adding For others on this thread, let me explain why (because it wasn't obvious to me at first either). What we want to do is to switch on a schema function f(schema: BaseSchema) {
switch(schema.type) {
case "object":
console.log(schema.entries);
break;
case "array":
console.log(schema.item);
break;
}
} This will never work if the interface FooSchema extends BaseSchema {
type: "object";
} I just defined a new subtype of As a result, TypeScript will never do the desired type narrowing as long as
I agree that this would be a nice quality of life improvement that would be great to have in Valibot – this way, I don't have to type out the union type myself. |
This should be fixed. The latest version provides |
At the moment when introspecting generic schemas, typescript don't know about the existance of the schema type attribute .
exemple, with error at line 6:
Potential Solutions
Add a generic attribute to BaseSchema and BaseSchemaAsync :
Add an introspection helper
Add an introspection visitor
exemple :
Any other ideas ?
The text was updated successfully, but these errors were encountered: