Refactor Bindgen #37
Refactor Bindgen #37
Conversation
bhelx
force-pushed
the
refactor-bindgen
branch
from
68e89cf
to
c1e823f
Compare
bhelx
force-pushed
the
refactor-bindgen
branch
from
2c40e31
to
ee1065c
Compare
bhelx
force-pushed
the
refactor-bindgen
branch
from
ee1065c
to
3e85fcf
Compare
| case 'int64': | ||
| throw Error(`We do not support format int64 yet`) |
There was a problem hiding this comment.
we may want to make this a bigint?
There was a problem hiding this comment.
Yeah, bigint would cover it!
There was a problem hiding this comment.
need to think a bit about how we should encode it since i think we're limited by the JSON spec. maybe just as a numerical string? "123456789098765432"?
There was a problem hiding this comment.
I like how (at least in Chrome) throws:
Do not know how to serialize a BigInt
😆
| function dateToJson(v: Date): string { | ||
| function castArray(caster: (v: any) => any) { | ||
| return (v?: Array<any>) => { | ||
| if (isNull(v)) return v |
There was a problem hiding this comment.
A (kind-of lengthy) question! So, IIUC right now the JS bindgen is more permissive than other bindgens about what values it accepts. That can lead to some surprise down the line for hosts if they build their host service and initial plugins using JS. (This happened in extendabot: some interfaces were actually nullable but not marked as such. The JS bindgen plugins worked, but go, rust, and others failed in ways that were difficult to debug from the guest side.)
Should we track type nullability here and fail if an array (or array member) is set to null inappropriately to match the other bindgens? (It's possible we're doing that and I just missed it, too!)
There was a problem hiding this comment.
This kind of comes at the question of the line b/w types and validation. Who is responsible for validation and where should that happen? I wanted this casting code to be agnostic to that. Though I understand with other, typed languages you get more validation out of the box. I still need to think a bit about where they should happen. We could maybe follow up with a stricter casting routine. I'm still holding out hope that we can use a dependency for all this code and delete it.
There was a problem hiding this comment.
Something to note about the casting code if it's not clear, we don't cast the whole object. only the parts that need it. So i do think if we consider some kind of validation we may need to do that on the whole object somehow.
| case 'int64': | ||
| throw Error(`We do not support format int64 yet`) |
There was a problem hiding this comment.
Yeah, bigint would cover it!
|
Nice - not too bad at all to convert this. I am curious if the |
@nilslice I think this is just something that we have to workaround in JS for now. I'd like to follow up by looking at a dependency to handle this, or now that I see that |
See dylibso/xtp-bindgen#18
This refactor should serve as the basis for how we change the other bindgens. I've kept it backwards compatible but once the changes are rolled out everywhere, we should trim unused properties and features.
Note we have removed all code that references XTP schema type directly like:
Also note that coincidentally, this implements #36