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

feat: add support for Request / Response progress #277

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

Conversation

picunada
Copy link
Contributor

πŸ”— Linked issue

Issue #45

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Added support for Request / Response progress.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@picunada picunada marked this pull request as draft August 24, 2023 05:37
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #277 (e8af744) into main (da64d78) will increase coverage by 1.10%.
The diff coverage is 96.82%.

❗ Current head e8af744 differs from pull request most recent head 12c07f1. Consider uploading reports for the commit 12c07f1 to get more accurate results

@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
+ Coverage   92.00%   93.10%   +1.10%     
==========================================
  Files           5        5              
  Lines         450      609     +159     
  Branches      102      109       +7     
==========================================
+ Hits          414      567     +153     
- Misses         36       42       +6     
Files Coverage Ξ”
src/fetch.ts 96.50% <96.82%> (+1.10%) ⬆️

... and 3 files with indirect coverage changes

@picunada
Copy link
Contributor Author

picunada commented Aug 24, 2023

Hello, I would like to request help for this PR.

For now I only implemented progress for req/response with json.

First commit here working probably only within Browser env.
How can I add support for Node streams?
Also requesting refactor advices :)

@pi0
Copy link
Member

pi0 commented Oct 26, 2023

Hi dear @picunada thanks for picking on this and nice idea for initial POC.

I think in order to move this idea forward, i am mainly looking forward to exposing minimum enough hooks in order to be able creating fetchWithProgrsss utility by wrapping $fetch, this way we can both make sure not adding overhead to the core and also implement platform dependent implementations. Also we could directly support a binary transform to allow all kind of responses. Not sure how we can wrap native utils such as response.formData() (one idea might be wrapping Response)

Basically what am i looking forward for next steps is to somehow introduce something like TrackedRequest / TrackedResponse wrapper classes. They might make this feature complete

@Dmytro-Tihunov
Copy link

Hi dear @picunada thanks for picking on this and nice idea for initial POC.

I think in order to move this idea forward, i am mainly looking forward to exposing minimum enough hooks in order to be able creating fetchWithProgrsss utility by wrapping $fetch, this way we can both make sure not adding overhead to the core and also implement platform dependent implementations. Also we could directly support a binary transform to allow all kind of responses. Not sure how we can wrap native utils such as response.formData() (one idea might be wrapping Response)

Basically what am i looking forward for next steps is to somehow introduce something like TrackedRequest / TrackedResponse wrapper classes. They might make this feature complete

Looking forward to this

@ChrisGV04

This comment was marked as off-topic.

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.

None yet

4 participants