-
Notifications
You must be signed in to change notification settings - Fork 1
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 MultipartHttpClient to handle file upload #7
Conversation
vincentdbs
commented
Jan 10, 2025
- Add multi part data fetch client
039d7be
to
1dbbe13
Compare
return this; | ||
} | ||
|
||
files(files: File[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pourquoi ces ajouts ? pour moi c'est propre à chaque projet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parce que, la plupart du temps, on veut s'en servir pour uploader des fichiers donc autant avoir les méthodes dispos sur la requête directement. Je peux supprimer si c'est superflu
|
||
export type MultipartHttpClient<T> = (request: MultipartHttpRequest<unknown>) => T; | ||
|
||
export class MultipartHttpRequest<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ce serait pas possible de capitaliser sur https://github.com/Coreoz/simple-http-request-builder/blob/master/src/lib/HttpRequest.ts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quand tu dis capitaliser c'est à dire ? réutiliser ou juste plus s'en servir ?
pour moi, faire les deux dans la même classe va complexifier un code se voulant assez simple.
en revanche, peut etre pourrait-on plus l'utiliser avec une API type ReadableStream, mais ca n'est pas le code fait ici
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je suis de l'avis de Lucas sur la séparation. Le code exporté par la lib pouvant être réutilisé l'a été dans le fichier.
Par contre, je pense qu'il faudrait réorganiser les dossiers de la lib car tout est à la racine pour l'instant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je comprends que ça serait compliqué d'utiliser HttpRequest.ts ici.
Et effectivement, il faudrait déplacer les 2 fichiers dans un dossier dédié multipart
. De plus, il faudrait documenter le fonctionnement de ces fichiers (comment utiliser le client multipart ? dans quel cas ?) dans le fichier README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est fait pour le dossier multipart.
Le readme était déjà là ahah