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: support blob responses #422

Merged
merged 17 commits into from
Jul 10, 2023
Merged

feat: support blob responses #422

merged 17 commits into from
Jul 10, 2023

Conversation

jd1378
Copy link
Contributor

@jd1378 jd1378 commented Jul 3, 2023

Resolves #385 and unjs/nitro#1237

@Hebilicious Hebilicious added the enhancement New feature or request label Jul 3, 2023
@pi0 pi0 changed the title fix: handle 304 responses properly fix(proxy): handle 304 responses properly Jul 4, 2023
@pi0
Copy link
Member

pi0 commented Jul 4, 2023

Would you please add a unit test? 🙏🏼

@jd1378
Copy link
Contributor Author

jd1378 commented Jul 5, 2023

Well during writing tests I realized the real issue was that Blob type was not being handled properly, which was why the fix was working, So I handled it in app part
but still I saw when empty string is returned along with 304, it has problem handling it and responds with 500, so I still added the test and fix for that part as well

@jd1378 jd1378 changed the title fix(proxy): handle 304 responses properly fix(app): handle blob type returns Jul 5, 2023
@jd1378
Copy link
Contributor Author

jd1378 commented Jul 5, 2023

I have pushed a commit to fix the lint issues but I don't the commit in the PR, what am I doing wrong ? is it me or github being funny ?

@jd1378
Copy link
Contributor Author

jd1378 commented Jul 5, 2023

yeah it was GitHub, re-set the base branch on edit dialog and it showed up

test/proxy.test.ts Outdated Show resolved Hide resolved
src/utils/proxy.ts Outdated Show resolved Hide resolved
@Hebilicious
Copy link
Member

@jd1378 This is a very good change, thanks for adding the tests !

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #422 (5278702) into main (091f326) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 5278702 differs from pull request most recent head 61e9222. Consider uploading reports for the commit 61e9222 to get more accurate results

@@            Coverage Diff             @@
##             main     #422      +/-   ##
==========================================
+ Coverage   77.18%   77.22%   +0.04%     
==========================================
  Files          26       26              
  Lines        2608     2613       +5     
  Branches      377      379       +2     
==========================================
+ Hits         2013     2018       +5     
  Misses        595      595              
Impacted Files Coverage Δ
src/app.ts 97.43% <100.00%> (+0.06%) ⬆️

@jd1378
Copy link
Contributor Author

jd1378 commented Jul 5, 2023

pushed commit for both suggestions

@Hebilicious Hebilicious self-requested a review July 5, 2023 11:43
@Hebilicious Hebilicious requested a review from pi0 July 5, 2023 11:43
@pi0 pi0 changed the title fix(app): handle blob type returns feat(app, proxy): support blob responses Jul 10, 2023
@pi0 pi0 changed the title feat(app, proxy): support blob responses feat: support blob responses Jul 10, 2023
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thanks ❤️

@pi0 pi0 merged commit 5c4dcd1 into unjs:main Jul 10, 2023
4 checks passed
@pi0 pi0 mentioned this pull request Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request proxy ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix sendProxy function
3 participants