-
Notifications
You must be signed in to change notification settings - Fork 11
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
SDK string conversion issues #50
Comments
Hey @hammo92 In general, a custom type, which accepts string and Date would be the way to go on our side, from my point of view. This would be the best DX imo. |
@sebastianwessel apparently it's a side effect of the transition from JSON to CBOR. With the new implementation seems that quite a few types are expecting classes:
I guess for the best dx we would have to create custom types for each. Or change our schema declaration to separate input and output schemas. |
As long as these classes are available in the SDK, we can use I would prefer to decouple validation from transformation. It might be a good idea to have some helper in the future. |
Do we not still have the issue of the input and output schema having different types if we are using the classes? It's fine for the Record as we're returned a RecordId instance, but ee're never returned an instance of date by the sdk we are always returned a string value await db.query(`DEFINE TABLE test SCHEMAFULL; DEFINE FIELD time ON test TYPE datetime; DEFINE FIELD relation ON test TYPE record<test>;`)
const a = await db.create("test", {
relation: new RecordId("test", "123"),
time: new Date()
})
[
{
id: RecordId { tb: 'test', id: '5r5qyfp1hp9mhaj6ar98' },
relation: RecordId { tb: 'test', id: '123' },
time: 2024-07-25T16:13:44.124Z
}
] |
Tbh, I need to look into the SDK and try out the new version. I wonder if it really makes sense to have validation during reads from db. |
I think you're right that validation on read is excessive, are you thinking to generate the read types without inferring them from Zod? |
Yes. Might also be simpler, as we only need to figure out the type and if it is optional/required, but not something like "is email". |
Makes sense, should be simpler to implement that way as well. |
The JS SDK adds the
s
parameter to all parameters provided as a string; this prevents Surreal from inferring types and forces a difference between the input and output schemas for a table. This causes errors with our schema definitions as we are expecting to be able to pass a date string to date fields but this will throw an error:But our generated type for this table would be
Barring a change to the SDK I think the best approach is likely to create a custom Zod type for dates similar to the recordId type which applies a transform and converts the value to a date; this would allow us to provide a string or date object argument for datetime fields.
The text was updated successfully, but these errors were encountered: