-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add support for promisified query and mutation methods. #157
Add support for promisified query and mutation methods. #157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Parker, I tried to do this locally but I was spending too much time in the TypeScript mindset trying to figure out how to extend the current source with that toPromise()
method.
Yeah, intersection types in Reason are quite difficult. In the case of most |
"husky": "^2.4.1", | ||
"lint-staged": "^8.2.1", | ||
"npm-run-all": "^4.1.5", | ||
"reason-react": "0.7.0" | ||
}, | ||
"peerDependencies": { | ||
"bs-fetch": "^0.5.0", | ||
"bs-fetch": "^0.5.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a side note, why is this a peer dependency? I was trying to figure that out but I’m not sure whether having it as just a dependency would cause issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would. It's a dep so we can get proper type support for Fetch.requestInit
. But yeah, I think we could just make it a dependency and save everyone the trouble.
) => { | ||
executeQuery(~client, ~request, ~opts?, ()) | ||
|> Wonka.take(1) | ||
|> Wonka.toPromise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make more sense maybe not to convert these to promises? It seems to me like you wouldn’t gain much in Reason from using promises 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that I don't think we necessarily gain much, but I think it's still good to cover in the lib in case people want to interact with the Js.Promise.t
type rather than Wonka.sourceT
. I could imagine folks coming from JS and learning about Js.Promise.t
and wanting a way to get a Js.Promise.t
from these methods. These bindings are such low cost anyway that I think it's still worthwhile (tho I prefer Wonka.subscribe
personally as well).
This PR begins adding support for
urql
v1.5, namely the promisifiedquery
andmutation
methods on the Client 🎉 This is fortunately quite easy in Reason, as the TS implementations of this just callWonka.take
andWonka.toPromise
under the hood.As part of housekeeping, I also upgraded our BuckleScript version to 7 (finally), changed our default project compilation to ES6, and added in the proper transform to support testing, similar to what we have in
renature
and what @kitten showed in his commit on thev2
branch.Next task will be adding support for the
useClient
hook, and then I think we'll have what we need to cut a release 🎉