Skip to content
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

GraphQLHookOptions - add refetchQueries type. #1751

Conversation

noire-munich
Copy link
Collaborator

@noire-munich noire-munich commented Feb 7, 2021

As seen on the forums, refetchQueries is not typed yet,
https://community.redwoodjs.com/t/is-refetchqueries-deprecated/1791/9

PR for #1752

@github-actions
Copy link

github-actions bot commented Feb 7, 2021

@Tobbe
Copy link
Member

Tobbe commented Feb 7, 2021

Thanks for getting started on this @noire-munich. Can you please clean up the lint warnings?

@noire-munich
Copy link
Collaborator Author

Done!
I'll write that one to a git hook...

@Tobbe
Copy link
Member

Tobbe commented Feb 7, 2021

I'll write that one to a git hook...

Something I should have done myself a long time ago! 😁

Now that I can read the code I had a second look, and I'm afraid this doesn't scale very well. Next time someone is going to want to use some other option for useQuery or useMutation. And what if they're using some other graphql client (i.e. not Apollo Client) that might not even have refetchQueries as an option at all?

I think we need to try to come up with a more generic solution. Probably something that uses actual generics. I've previously done some exploration around the whole gql client situation, and in doing so I've tried writing better types for everything. I don't have anything that's complete yet. But in one of my crazy experiments where I wrote my own gql client I did come up with something that's at least more flexible than what we have today with the types. Have a look here if you want https://github.com/redwoodjs/redwood/pull/1535/files#diff-dc5df66f19077795660133684ca45c60b2651b16962fc9cd3635f52bf03c661bR55

If it sounds like too much work, don't worry. I'll get around to fixing this myself eventually. Probably as soon as I run in to this problem myself in my own app. But if you do want to spend the time on it, that would be awesome! 🙂 No pressure either way.

@noire-munich
Copy link
Collaborator Author

Now that I can read the code I had a second look, and I'm afraid this doesn't scale very well. Next time someone is going to want to use some other option for useQuery or useMutation. And what if they're using some other graphql client (i.e. not Apollo Client) that might not even have refetchQueries as an option at all?

Agreed! Which is why I proposed to make refetchQueries optional, if we're on an Apollo client it will make sense - or keeping it as the default RW graphql client.

This being said your solution is more elegant. I'm rather short on time lately, I can't block any PR related to this.
I had a look at your branch though, I'll definitely be looking forward to see your updates on it. I've seen a couple of @todos, do you have any checklist?

@Tobbe
Copy link
Member

Tobbe commented Feb 8, 2021

I'm rather short on time lately, I can't block any PR related to this.

No worries, someone else will get to it eventually 👍 That PR I linked was an experiment as part of this bigger idea: #1602 Your imput on that one is appreciated, if you have anything to add 🙂

@thedavidprice
Copy link
Contributor

@dac09 ok for this one in v0.25?

@Tobbe Tobbe marked this pull request as draft February 9, 2021 08:47
@Tobbe
Copy link
Member

Tobbe commented Feb 9, 2021

I'm marking this as draft until @noire-munich, I or someone else implements a more generic solution than what's currently in this PR (see my previous comments for details)

@dac09
Copy link
Collaborator

dac09 commented Feb 9, 2021

Looks clear to me. Shouldn't conflict with prerender

@noire-munich
Copy link
Collaborator Author

No worries, someone else will get to it eventually +1 That PR I linked was an experiment as part of this bigger idea: #1602 Your imput on that one is appreciated, if you have anything to add slightly_smiling_face

that's great, thanks :).
I'll put #1602 on my study radar then

@jtoar
Copy link
Contributor

jtoar commented May 22, 2021

@noire-munich super sorry we lost track of this one! @dac09 will #2485 handle this?

@dac09
Copy link
Collaborator

dac09 commented Jun 4, 2021

Implemented in #2485 - thanks for this @noire-munich !

@dac09 dac09 closed this Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript - refetchQueries not typed
5 participants