Hi there! I'm working on a PR on graphql-request. ...
# orm-helpb
brikou
09/12/2018, 8:09 AMHi there! I'm working on a PR on graphql-request. Still WIP but I'd like a feedback to know if I'm doing something usefull? Here it is: https://github.com/prisma/graphql-request/pull/105
d
divyendu
09/12/2018, 8:18 AMThat is awesome, feel free to DM me.. we can coordinate and get this moving. Thanks for contributing 🙂
b
brikou
09/12/2018, 8:18 AMok thanks
brikou
09/12/2018, 8:18 AMl will
brikou
09/12/2018, 9:46 AMI'll add gql tag example and link to TS sources
brikou
09/12/2018, 9:47 AMAlso fragment example
d
divyendu
09/12/2018, 10:46 AMSounds awesome 🙂
One small request: Can you please change this async wrapper part of code: https://github.com/prisma/graphql-request/pull/105/files#diff-04c6e90faac2675aa89e2176d2eec7d8R83
With this
async function mainb
brikou
09/12/2018, 11:56 AMOk no problem
👍 1
brikou
09/12/2018, 12:04 PMOk but I have to export every main, at least un TS, because without it we would have many main on global and so TS error, am I c'est?
brikou
09/12/2018, 12:04 PMClear*
d
divyendu
09/12/2018, 12:06 PMIn the PR at the moment, the async part is not exported? Why would you want that? Maybe I misunderstood something.
divyendu
09/12/2018, 12:07 PMEven if it is exported, it is scoped by the package and can be aliased while importing but I don't get the requirement. Please explain more 🙂
b
brikou
09/12/2018, 12:11 PMI'll explain un PR layer :)
👍 1
brikou
09/12/2018, 1:13 PM@divyendu do you know an gql endpoint that sets a cookie for cookie example?
brikou
09/12/2018, 1:13 PMotherwise example sucks a bit
brikou
09/12/2018, 1:16 PMDoes anyone knows a public endpoint that also return a 'set-cookie' header? (for writing an example code) 🤞
d
divyendu
09/12/2018, 1:48 PMNot that I am aware of from the top of my head 🙂
b
brikou
09/12/2018, 2:25 PMCan you take a look at my PR, ready for review, I can give you an example for the required example...
brikou
09/12/2018, 2:35 PMping @divyendu 😏
brikou
09/12/2018, 2:38 PMI'll start a new PR based on this one to add ts typings
d
divyendu
09/12/2018, 3:00 PMHey! sure.. I will take a look later today 🙂
b
brikou
09/12/2018, 3:03 PMthanks, can't wait to have your feedback
👍 1
brikou
09/12/2018, 5:20 PMI made 2 others PR following the 1st one https://github.com/prisma/graphql-request/pull/106 and https://github.com/prisma/graphql-request/pull/107, I can rebase once you ask me to clean diff with previous PR, thanks for your review 👍
👍 1
d
divyendu
09/12/2018, 7:45 PMHey! thanks for the awesome work.. I will take a look as soon as I can.. 🙂
b
brikou
09/12/2018, 8:16 PMGreat, I've other things in mind like rename T to TData and add TVariables types, also accept DocumentNode (gql) and not just string as query param
👍 1
brikou
09/12/2018, 8:17 PMBut do it once those are merged 😏
brikou
09/12/2018, 9:04 PMI also have a new PR for a long waiting bug https://github.com/prisma/graphql-request/pull/108
🙌 1
brikou
09/13/2018, 8:34 AM@divyendu I'm working on adding TVariables, and do you think changing TResult from "any" to "unknown" is doable?
brikou
09/13/2018, 8:36 AMI think this is great but could be kind of a breaking change for TS user
brikou
09/13/2018, 8:36 AMfor lazy TS users
d
divyendu
09/13/2018, 8:40 AMAny breaking changes will more discussion, I would suggest documenting the change in form of an issue and then we can discuss over there publicly and decide on it 🙂
👍 1
b
brikou
09/13/2018, 9:17 AMPR to add TVariables generic : https://github.com/prisma/graphql-request/pull/109
brikou
09/13/2018, 9:18 AM@divyendu What do you think about supporting both string and gql tag as query param?
d
divyendu
09/13/2018, 9:22 AMWe have gone on that road once, had to revert - for context - https://github.com/prisma/graphql-request/pull/92
b
brikou
09/13/2018, 9:40 AMA usage doc in README.md then should be enough 🙂
👍 1
brikou
09/13/2018, 10:47 AM2 Views