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

Fix missing headers for fetch #687

Merged
merged 7 commits into from
Oct 14, 2024
Merged

Fix missing headers for fetch #687

merged 7 commits into from
Oct 14, 2024

Conversation

berndfuhrmann
Copy link
Contributor

Issue #679:

Description of changes:

  • For integration tests, added a small HTTP server that receives requests made via fetch. This offers the ability to check headers that were actually supplied in the request.
  • I reverted a changed introduced recently, so X-Amzn-Trace-Id headers are sent again
  • Exporting enableCapture function. This provides the ability to use this without changing globals. It also provides the ability to use different Request classes, e.g. undici.Request instead of globalThis.Request. This should provide a way for anyone who wants to use undici with dispatcher.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@berndfuhrmann berndfuhrmann requested a review from a team as a code owner September 20, 2024 07:50
@berndfuhrmann
Copy link
Contributor Author

Also added this bug to undici:
nodejs/undici#3630

@jj22ee
Copy link
Contributor

jj22ee commented Oct 5, 2024

Thanks for the fix and improvements for this and the undici package!

The changes lgtm. We can remove package-lock.json in fetch package as this is a monorepo, I suppose your changes to package.json should affect the root package-lock.json instead.

Not sure why the codecov is failing suddenly on the Node16.x/Ubuntu test, I'll look into this (and also #688). It should be unrelated to your PR.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.56%. Comparing base (9552603) to head (5470288).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #687   +/-   ##
=======================================
  Coverage   83.56%   83.56%           
=======================================
  Files          36       36           
  Lines        1813     1813           
=======================================
  Hits         1515     1515           
  Misses        298      298           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jj22ee jj22ee left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@jj22ee
Copy link
Contributor

jj22ee commented Oct 14, 2024

I've fixed the failing codecov in #692 and merged the fix into your branch, along with some minor fixes to the package-lock files.

@jj22ee jj22ee merged commit e28c1af into aws:master Oct 14, 2024
13 checks passed
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.

3 participants