-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(integrations): Ensure httpclient integration works with Request #7786
Conversation
We cannot clone a request if it has a body which was read (which seems to happen somewhere). So instead we just re-use the same request if possible, instead of creating a new one.
@@ -14,7 +14,7 @@ | |||
"build:dev": "yarn build", | |||
"build:watch": "run-p build:transpile:watch build:types:watch", | |||
"build:dev:watch": "run-p build:watch", | |||
"build:transpile:watch": "yarn build:rollup --watch", |
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.
What is this about?
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.
just noticed this while working here that this is actually referencing wrong commands, so fixed them while at it! same for integration-shims, copy paste error 😅
size-limit report 📦
|
OK, turns out we can't do (all of) this - we actually use the passed in request and add headers to it in browser tracing, so we cannot swap this with a clone. So I'll only do the change for the httpclient integration, which should be fine too. |
We cannot clone a request if it has a body which was read (which seems to happen somewhere). So instead we just re-use the same request if possible, instead of creating a new one.
This fixes this in two places:
args
in the first place.httpclient
integration, and avoid creating unnecessary copies of the Request where we don't need it.I added some tests for different scenarios covering this.
Fixes #7785