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

add useQuery and useMutation hooks #7

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

luca-montaigut
Copy link

I use these hooks in my projects and I think they can be useful in this lib.

This is a first version, we can imagine improving them with onComplete and onError callbacks to increase the control of the user during his interactions with the db.

@GuiBibeau
Copy link
Owner

@luca-montaigut Thank you very much for this PR! I'll review everything tonight. I'll try to add soon a publish pipeline and versionning mechanism to get these new hooks you did released!

Copy link
Owner

@GuiBibeau GuiBibeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the contribution. I just want to improve a bit the typings to provide a fully typed experience on the hooks.

I potentially will also introduce these ideas in a current piece of work I am doing if that is fine with you.

import { useEffect, useState } from 'react'
import type { Response } from '../types'

export const useQuery = (query: any) => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyway to make this accept a generic and pass it to the state? would be useful for DX

Comment on lines 11 to 14
const execute = async (mutation: any) => {
setMutationState((prev) => ({ ...prev, loading: true, error: null }))
try {
const { data, error } = await mutation
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? to get typed data returned

async function execute<T>(mutation: SupabaseQueryBuilder<T>)  {
}

@luca-montaigut
Copy link
Author

Thank you very much for the contribution. I just want to improve a bit the typings to provide a fully typed experience on the hooks.

I potentially will also introduce these ideas in a current piece of work I am doing if that is fine with you.

Totally agree with you I'm not very proud of these 'any'... But I didn't really know how to do it, I'll dig into the subject.

You can of course use these hooks! I use them for this project: supabase-starter-kit and I intend to rework it with your lib once the hooks are clean and merged.

And then maybe I'll write a little something (like a tutorial or a blog post) about my starter-kit and this lib (plus it's my first contribution to open-source 🎉 )

@GuiBibeau
Copy link
Owner

@luca-montaigut Merci beaucoup pour la contribution!!

I've added useTable but added some features on top of your approach such as generics and SWR capacities. It was a great idea to implement this into the lib. Do you want to try a similar approach to the useMutation and try to add it to the lib?

@luca-montaigut
Copy link
Author

@luca-montaigut Merci beaucoup pour la contribution!!

I've added useTable but added some features on top of your approach such as generics and SWR capacities. It was a great idea to implement this into the lib. Do you want to try a similar approach to the useMutation and try to add it to the lib?

I managed to type for useQuery but for useMutation I'm missing a little something: typing works for "normal" query (.from) but not for auth (.auth.signUp etc...). Let me know what you thing about this second version 😃

useTable is cool for easily grabbing a whole table =) 💪 good job!

However, I have a few questions (sorry if the answers are obvious but I'm still a junior):

1/ I don't see the point of the useSupabase() vs importing the client => you still have to import something no? (+ the context)
I have the impression that it makes the code less standard compared to the doc, to other projects or stackoverflow answers without a real benefit since the client does not change.

2/ Why import a second fetch lib (SWR)? I think we can manage it without adding another dependency

=> I saw that you didn't put a loading state and that you use !data instead. For the query it's ok but I'm not sure we can do without for the mutation, I tried in a sandbox project and I couldn't manage without it.

@GuiBibeau
Copy link
Owner

@luca-montaigut Here I don't see swr as a fetching library, the fetching implementation is actually left to the user. The goal of the lib really is to get revalidation based on UX usage versus listening to the whole table events that could get expensive the more there are.

We could do away with useSupabase for sure. I'm evaluating if there will be future usage to having a singleton client eventually in context. Not sure yet but I'll keep it for a bit while we explore the topic.

Loading will happen totally for useMutation, I'll incorporate your PR as it is for it actually

@luca-montaigut
Copy link
Author

luca-montaigut commented Mar 10, 2022

@luca-montaigut Here I don't see swr as a fetching library, the fetching implementation is actually left to the user. The goal of the lib really is to get revalidation based on UX usage versus listening to the whole table events that could get expensive the more there are.

We could do away with useSupabase for sure. I'm evaluating if there will be future usage to having a singleton client eventually in context. Not sure yet but I'll keep it for a bit while we explore the topic.

Loading will happen totally for useMutation, I'll incorporate your PR as it is for it actually

Maybe to split the difference with the useSupabase() we can keep it but not force tu usage of hook like useQuery / useTable in the context ?

Do you have an idea to finish typing the useMutation so that this also works:

  const registerWithEmailAndPassword = async (
    email: string,
    password: string,
  ) => {
    execute(
      supabase.auth.signUp({
        email,
        password,
      }),
    );
  };

For normal mutation, typing work but not for auth ...

@GuiBibeau
Copy link
Owner

@luca-montaigut Here I don't see swr as a fetching library, the fetching implementation is actually left to the user. The goal of the lib really is to get revalidation based on UX usage versus listening to the whole table events that could get expensive the more there are.
We could do away with useSupabase for sure. I'm evaluating if there will be future usage to having a singleton client eventually in context. Not sure yet but I'll keep it for a bit while we explore the topic.
Loading will happen totally for useMutation, I'll incorporate your PR as it is for it actually

Maybe to split the difference with the useSupabase() we can keep it but not force tu usage of hook like useQuery / useTable in the context ?

Do you have an idea to finish typing the useMutation so that this also works:

  const registerWithEmailAndPassword = async (
    email: string,
    password: string,
  ) => {
    execute(
      supabase.auth.signUp({
        email,
        password,
      }),
    );
  };

Good idea! I'll check tomorrow for the types and splitting the difference

@luca-montaigut
Copy link
Author

Hey @gbibeaul , I've added a refetch mecanism on useQuery, it was usefull on my projet.
For typing I still searched but without success. I may not be good enough in TS but I have the impression that there are too many different types possible and that they don't combine with each other

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants