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

Options are overwritten if json is available. #507

Open
ThanosDi opened this issue May 24, 2023 · 1 comment
Open

Options are overwritten if json is available. #507

ThanosDi opened this issue May 24, 2023 · 1 comment

Comments

@ThanosDi
Copy link

Based on this line it seems that the original options are being overwritten with {body: this._options.body}.

this.request = new globalThis.Request(this.request, {body: this._options.body});

This causes issues with 3rd party libraries(for example highlight.io) when they try to copy the request headers from the options rather than the request object.

Is this intended?

@sholladay
Copy link
Collaborator

I think it was written that way for simplicity back when this code was only modifying the options object, before we started using a Request instance to help process the options and headers.

Now we should be able to pass the JSON body directly to the Request constructor instead of using the options object as an intermediary.

I support this change, but we'll need to make sure it doesn't cause regressions with retry requests. Retries have to recreate the request and reapply the options, but I can't remember if this code runs again on retries. If it doesn't, the retried request won't have a stringified body anymore.

We do have some retry tests, but I'm not sure if they'll catch that kind of bug.

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

No branches or pull requests

2 participants