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

chore: utilisations des mutations SWR sur les espaces de travail #1188

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ggrossetie
Copy link
Collaborator

L'objectif est de simplifier la gestion de l'état en supprimant les actions dans le store Redux.

Copy link

netlify bot commented Jan 21, 2025

Deploy Preview for stylo-docs canceled.

Name Link
🔨 Latest commit 3675917
🔍 Latest deploy log https://app.netlify.com/sites/stylo-docs/deploys/67928fbf355aca00089141f2

Copy link

netlify bot commented Jan 21, 2025

Deploy Preview for stylo-dev ready!

Name Link
🔨 Latest commit 3675917
🔍 Latest deploy log https://app.netlify.com/sites/stylo-dev/deploys/67928fbffdfa670008d17419
😎 Deploy Preview https://deploy-preview-1188--stylo-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ggrossetie ggrossetie force-pushed the fix-1171-hooks-usegraphql branch from 00e2d95 to 4c02d52 Compare January 22, 2025 16:38
@ggrossetie ggrossetie marked this pull request as ready for review January 22, 2025 16:44
Comment on lines +26 to +31
updatedAt: new Date().getTime(),
createdAt: new Date().getTime(),
stats: {
membersCount: 1,
articlesCount: 0,
},
Copy link
Member

Choose a reason for hiding this comment

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

Y'a besoin de passer ces infos ? Ça devrait être des défauts côté base de données, pas côté front.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

C'est avant que les données soient récupérées du serveur.
L'autre solution c'est de gérer l'absence de ces valeurs dans les composants mais je trouve ça dommage car le serveur retourne bien ces informations...

Comment on lines 55 to 74
const removeMember = async (user) => {
const { _id: userId } = user
await mutation({
query: removeMemberQuery,
variables: { workspaceId, userId },
})
const result = await mutate(
async (data) => {
return {
workspace: {
members: data.workspace.members.filter((m) => m._id !== userId),
},
}
},
{ revalidate: false }
)
await updateMembersCount(result)
}
Copy link
Member

Choose a reason for hiding this comment

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

Le seul truc qui me gêne c'est l'absence de tests : c'est très dur de voir si y'a un bug dans la mise à jour des données du cache :-/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Peut-être que SWR propose quelques primitives pour faciliter les tests. Sinon je peux essayer de mock pour tester une partie de la logique ?

export function useWorkspaceMembersActions(workspaceId) {
const { mutate: workspacesMutate } = useSWRConfig()
const key = useSWRKey()({ query: getWorkspaces })
const mutation = useMutation()
Copy link
Member

Choose a reason for hiding this comment

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

C'est trompeur : useMutation() retourne un object request bindé au jeton utilisateur, ça n'a rien à voir avec swr… qui a son propre useSWRMutation(). Alors qu'on a aussi useMutate()… qui utilise useSWR().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oui la notion de mutation est commune entre SWR et le concept de query/mutation de GraphQL

J'avais tenté de faire mutate -> SWR et mutation -> GraphQL mais ce n'est clairement pas satisfaisant 😞
J'ai fais une proposition dans #1171 (comment) pour améliorer le nommage.

Mon idée c'était de faire une utilisation complète/avancée de SWR avant de revoir le nommage/supprimer ce qui est obsolète.

Si ça te va, je préfère gérer le renommage dans une autre pull request.

front/src/hooks/workspace.js Outdated Show resolved Hide resolved
description: workspace.description,
name: workspace.name,
})
mutate(key, async (data) => ({
Copy link
Member

Choose a reason for hiding this comment

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

Si on utilise mutate(), faudrait que ça corresponde au concept de swr. Sinon ça ne fait pas de différence avec le useGraphQL() + runQuery() qu'on a déjà. Non ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Si on utilise mutate(), faudrait que ça corresponde au concept de swr.

Sur la ligne que tu commentes, le mutate provient bien de SWR et permet de modifier le cache SWR (et d'envoyer un revalidate).

Sinon ça ne fait pas de différence avec le useGraphQL() + runQuery() qu'on a déjà. Non ?

Le runQuery ne fait qu'un appel à l'API GraphQL (pas de lien avec SWR). Cela permet de faire une mutation ou une query GraphQL quand on a pas besoin de revalidate.
Cela dit, peut être que plus on va généraliser l'utilisation de SWR plus ce cas sera à la marge.

@ggrossetie
Copy link
Collaborator Author

Quelques changements sur mon dernier commit:

  • Ajout d'un test 🎉
  • Supprime le WorkspaceService
  • Regroupe les query et mutations GraphQL dans un fichier unique Workspaces.graphql

@ggrossetie ggrossetie force-pushed the fix-1171-hooks-usegraphql branch from 4c02d52 to 3675917 Compare January 23, 2025 18:51
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