search

jeddeloh / rescript-apollo-client

ReScript bindings for the Apollo Client ecosystem
MIT License
126 stars 18 forks source link

[Discussion] Convert to using optional records? #151

Closed illusionalsagacity closed 3 weeks ago

illusionalsagacity commented 1 year ago

It'd likely cut down on the size overhead of the library, and it certainly would look more like the TS/JS usage of apollo-client.

I also think it may be possible to reduce some of the runtime overhead, which in my experience has led to some odd edge cases where rescript-apollo-client will behave differently with it's return being used for effect dependencies to TS code. Some of this is unavoidable due to the graphql-ppx parsing, but it'd be nice to see where this could potentially reduce it.

That said, I kinda like the named parameters stylistically, and I am not sure if other users are still on ReScript or even bs-platform 9 still?

jeddeloh commented 1 year ago

Good or bad, I do think that using optional records is more in line with the philosophy of this library—that being that it's such a large surface area to cover, we want to be as close to the Apollo api as possible. Ideally, one could use their docs exclusively. Might be prudent to see some example usage first to confirm it works out like we would like. I'm not in a place right now where I'm going to take on a refactor that large, but I'm happy to help anyone who might.

There is also some defensive coding around exceptions that adds a lot of complexity for little gain that could be removed. There's long discussion about that somewhere on here :)