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(common): add right ContentType for boolean values with HttpClient request body(#38924) #41885

Closed

Conversation

gopal-jayaraman
Copy link
Contributor

We are adding application/json as ContentType for boolean as the body, currently it is seen as text/plain, where it should be seen as application/json, since it is valid JSON, like numbers.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

currently a boolean as the body is seen as text/plain, where it should be seen as application/json, since it is valid JSON, like numbers.

Issue Number: #38924

What is the new behavior?

currently a boolean as the body is seen as text/plain, where it should be seen as application/json, since it is valid JSON, like numbers.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

I know this shouldn't be a breaking change, but I wonder if it would actually break some people who have relied on booleans being marked as text...

@@ -324,7 +324,7 @@ export class HttpRequest<T> {
}
// Arrays, objects, and numbers will be encoded as JSON.
if (typeof this.body === 'object' || typeof this.body === 'number' ||
Array.isArray(this.body)) {
typeof this.body === 'boolean' || Array.isArray(this.body)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Array.isArray is redundant because this.body is an object in that case

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S.: Update the comment in line 325

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alfaproject - I have updated the review comments, Please check.

… request body(angular#38924)

currently a boolean as body is seen as text/plain, where is should be seen as application/json, since it is valid JSON, like numbers.
@petebacondarwin petebacondarwin added action: presubmit The PR is in need of a google3 presubmit area: common/http target: patch This PR is targeted for the next patch release labels Apr 30, 2021
@ngbot ngbot bot modified the milestone: Backlog Apr 30, 2021
@petebacondarwin petebacondarwin removed the request for review from jessicajaniuk April 30, 2021 16:34
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir
Copy link
Contributor

@petebacondarwin FYI presubmits are successful for the changes in this PR.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label May 1, 2021
@petebacondarwin petebacondarwin added the action: merge The PR is ready for merge by the caretaker label May 1, 2021
mhevery pushed a commit that referenced this pull request May 3, 2021
… request body(#38924) (#41885)

currently a boolean as body is seen as text/plain, where is should be seen as application/json, since it is valid JSON, like numbers.

PR Close #41885
mhevery pushed a commit that referenced this pull request May 3, 2021
… request body(#38924) (#41885)

currently a boolean as body is seen as text/plain, where is should be seen as application/json, since it is valid JSON, like numbers.

PR Close #41885
@mhevery mhevery closed this in 6e11feb May 3, 2021
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: common/http cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants