search

jeddeloh / rescript-apollo-client

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

Incompatibility/Bug with @apollo/client > 3.5.0 #133

Closed illusionalsagacity closed 2 years ago

illusionalsagacity commented 2 years ago

https://github.com/apollographql/apollo-client/commit/91945a1574aeb600b8faf6d099dbeabe1af866a1#diff-fb7fa652de6d84c08b87344a06a4d120b664c6e8bf491d16575794c755141477R356

A change was made in @apollo/client > 3.5 that means the query property in the options object will override the first argument and because rescript encodes query: undefined in that options object there's a runtime error. Technically I think the responsibility lies with apollo here, but humans probably wouldn't write useQuery(documentNode, { query: undefined }) either.

The easiest way to fix this looks like replacing query: None with query: Some(Operation.query), but would also be fixed by removing the query property entirely from the options type definitions. I'd be happy to do either just let me know what you'd prefer.

Also affects useMutation, useLazyQuery, etc

jeddeloh commented 2 years ago

Thanks for bringing this up! It's hard for me to imagine why query is even part of QueryHookOptions. Maybe it's extended from another type that is used in client.query? Regardless, I try not to put my own spin on things, and just try and translate them as-is and assume the type in typescript is correct, so I propose we keep the functionality.

We can fix this case by either doing query: Some(Operation.query) as you suggest, or we could switch the underlying QueryHookOptions.Js_.t type to use @obj so the properties are stripped when we convert in the toJs function. This has unfortunately been needed in a few other places where Apollo is checking keys not values. The @obj approach is probably more robust in terms of this kind of thing not happening in the future, but Some(Operation.query) is probably fine as well. Any thoughts?

illusionalsagacity commented 2 years ago

Thanks for bringing this up! It's hard for me to imagine why query is even part of QueryHookOptions. Maybe it's extended from another type that is used in client.query? Regardless, I try not to put my own spin on things, and just try and translate them as-is and assume the type in typescript is correct, so I propose we keep the functionality.

I think they just share the options type between client.query and the hook?

We can fix this case by either doing query: Some(Operation.query) as you suggest, or we could switch the underlying QueryHookOptions.Js_.t type to use @obj so the properties are stripped when we convert in the toJs function. This has unfortunately been needed in a few other places where Apollo is checking keys not values. The @obj approach is probably more robust in terms of this kind of thing not happening in the future, but Some(Operation.query) is probably fine as well. Any thoughts?

The@obj change sounds good to me