Virtual-Royaume / Royaume-Discord-Bot

It is the bot of our Discord community, it offers various utility features. It is written in TypeScript and uses DiscordJS to interact with the Discord API.
9 stars 1 forks source link

Correction du système de preview du code mentionné dans un lien GitHub #173

Closed Unarray closed 1 year ago

Unarray commented 1 year ago

Close #170

Unarray commented 1 year ago

Cette PR amène un problème dans la conception de notre fonction restRequest Celle-ci ne va pas pouvoir être utilisé pour ce type de request (requêtes ou le Content-Type est différent de application/json)

Dans notre object retourné par notre fonction, nous avons :

return {
  success: true,
  data: await response.json()
};

le await response.json() parse notre réponse en JSON. Sauf que quand notre réponse n'est pas du JSON, cela nous revoie une erreur du type "not valid JSON"

Vu la conception de la fonction, je ne sais pas trop si il vaut mieux la modifier ou garder celle-la pour le json et en faire une autre pour les chaines de charactères ou autre...

Bluzzi commented 1 year ago

Il y a un problème avec ESLint qui ne passe pas, ceci sera règlé en faisant une update du package @bluzzi/eslint-config (pnpm update -L).

Bluzzi commented 1 year ago

Le mieux serait de créer une nouvelle fonction pour ce cas d'usage je pense, modifier celle-ci lui apporterait trop de complexité pour un cas d'usage peu fréquent.

Unarray commented 1 year ago

Voici les changements. Nous avons deux fonctions pour les requêtes rest

Il y a juste une chose que je n'aime pas actuellement c'est qu'il y a de la répétition...

Les deux fonctions ```ts export const restJsonRequest = async(method: Method, endpoint: string, config: RequestParams = {}): Promise> => { if (config.query) { const urlParams = new URLSearchParams(config.query); endpoint = `${endpoint}?${urlParams.toString()}`; } config.headers = { ...config.headers, "Content-Type": "application/json" }; const response = await fetch(endpoint, { ...config, method: method }); if (!response.ok) { Logger.error("Rest request failed :"); console.log(method, endpoint, config); return { success: false, data: null }; } return { success: true, data: await response.json() }; }; ``` ```ts export const restTextRequest = async(method: Method, endpoint: string, config: RequestParams = {}): Promise> => { if (config.query) { const urlParams = new URLSearchParams(config.query); endpoint = `${endpoint}?${urlParams.toString()}`; } config.headers = { ...config.headers, "Content-Type": "text/plain" }; const response = await fetch(endpoint, { ...config, method: method }); if (!response.ok) { Logger.error("Rest request failed :"); console.log(method, endpoint, config); return { success: false, data: null }; } return { success: true, data: await response.text() }; }; ```

Comme on le voit ce sont quasiment les mêmes fonctions et je ne trouve pas ca très propre. j'avais pensé a faire une fonction générale nommée restRequest par exemple et comme valeur de retour nous rendrait

{
  succes: boolean,
  response: globalThis.Response // La valeur de retour de la fonction `fetch`
} 

Le seul problème est que j'ai peur que pour le typage de restJsonRequest et restTextRequest le typage devient complexe.

Unarray commented 1 year ago

Il y a un problème avec ESLint qui ne passe pas, ceci sera règlé en faisant une update du package @bluzzi/eslint-config (pnpm update -L).

Cela n'a rien réglé

Bluzzi commented 1 year ago

En effet ça fait beaucoup de duplications de code, il faudrait une fonction globale pour gérer ça. Si tu n'es pas inspiré pour la faire, tu peux ouvrir une issue et merge cette PR.

Pour le problème ESLint, je m'en charge après.

Unarray commented 1 year ago

Ca vous semble mieux ou encore a améliorer ?