Skip to content
This repository was archived by the owner on Jan 3, 2026. It is now read-only.

fix!: examine response content-type if no contentType is set#535

Merged
sofisl merged 6 commits into
googleapis:mainfrom
ddelgrosso1:examine-content-type
Jul 11, 2023
Merged

fix!: examine response content-type if no contentType is set#535
sofisl merged 6 commits into
googleapis:mainfrom
ddelgrosso1:examine-content-type

Conversation

@ddelgrosso1

Copy link
Copy Markdown
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #226 🦕

@ddelgrosso1 ddelgrosso1 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 23, 2023
@ddelgrosso1 ddelgrosso1 requested a review from a team May 23, 2023 17:58
@product-auto-label product-auto-label Bot added the size: m Pull request size is medium. label May 23, 2023
Comment thread src/gaxios.ts

opts.validateStatus = opts.validateStatus || this.validateStatus;
opts.responseType = opts.responseType || 'json';
opts.responseType = opts.responseType || 'unknown';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I want to get some opinions on whether or not we would consider this a breaking change. Previously we defaulted for to JSON as the contentType if the caller did not specify one. This is sort of a faulty assumption and we probably should have been looking at the response headers. I changed this to default to unknown but I fear that consumers may have been relying on this old JSON behavior.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this would technically be breaking, although customers would be relying on a bug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we do this, we should also update the README: https://github.com/googleapis/gaxios#request-options

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the README

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 23, 2023
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 23, 2023
@ddelgrosso1

Copy link
Copy Markdown
Contributor Author

Compodoc > 1.1.19 has a dependency on marked 5.0.2 which requires node 18 minimum. Pinning compodoc in this PR.

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 23, 2023
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 23, 2023
@generated-files-bot

Copy link
Copy Markdown

Warning: This pull request is touching the following templated files:

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 31, 2023
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 31, 2023
@ddelgrosso1 ddelgrosso1 added the next major: breaking change this is a change that we should wait to bundle into the next major version label May 31, 2023
@danielbankhead danielbankhead changed the title fix: examine response content-type if no contentType is set fix!: examine response content-type if no contentType is set Jun 2, 2023
@sofisl sofisl removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 10, 2023
@sofisl sofisl added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 11, 2023
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 11, 2023
@sofisl sofisl merged commit cd8ca7b into googleapis:main Jul 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

next major: breaking change this is a change that we should wait to bundle into the next major version size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Content-Type response header is ignored

3 participants