Skip to content

fix(http): Add support for requesting body as URLSearchParams type #37767

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.

Sign up for GitHub

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

Jump to bottom
Closed
sera0731 wants to merge 4 commits into from
Closed

fix(http): Add support for requesting body as URLSearchParams type #37767

sera0731 wants to merge 4 commits into from

Conversation

@sera0731
Copy link

@sera0731 sera0731 commented Jun 26, 2020 •
edited
Loading

There is no support for URLSearchParams type request in the current version. This commit adds serialize body function for URLSearchParams type.

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?

Issue Number: 36317

There is no support for URLSearchParams type request in the current version. When a user passes the body parameter with this type, Angular considers it as a random object. It ends up being omitted.

What is the new behavior?

This commit adds serialization for URLSearchParams type. It will handle URLSearchParams type request, it will not be omitted.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@sera0731
fix(http): Add support for requesting body as URLSearchParams type
4da09d5 
There is no support for URLSearchParams type request in the current version. This commit adds serialize body function for URLSearchParams type.
@pullapprove pullapprove bot requested a review from alxhub June 26, 2020 03:34
alfaproject
alfaproject reviewed Jun 26, 2020
View reviewed changes
sera0731 added 3 commits June 26, 2020 01:41
@sera0731
fix(http): Update unit test for IE11
8df579b 
IE doesn't support URLSearchParams. There was no condition for an unsupported browser. It will fix unit test failure.
@sera0731
fix(http): Change formatting
8fc0144 
The If clause was not properly formatted. Bazel format command solved this issue to correct the file's format.
@sera0731
fix(http): Update isURLSearchParams type to unknown
d3e1d50 
As per alfaproject's review, I changed isURLSearchParams input parameter value's type from any to unknown.
@AndrewKushnir AndrewKushnir added the area: common/http Issues related to HTTP and HTTP Client label Jul 1, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jul 1, 2020
petebacondarwin
petebacondarwin suggested changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@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.

Hi @sera0731 - thanks for sending in this PR.

I note that @ajitsinghkaler also created a PR at #37852. But since yours came first I would like to help you to get this merged if you are still interested.

I think that serializing the value to a string is not the correct approach. Instead I think we should do similar to the other PR where we treat it like a special type that just gets passed through - like Blob and FormData. The gives the most flexibility to the caller to then use the URLSearchParams API to extract what they want.

Can you update your code to do this and change the test accordingly?

@petebacondarwin petebacondarwin mentioned this pull request Jan 28, 2021
Closed
14 tasks
@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Jan 28, 2021
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jan 28, 2021
@alxhub
Copy link
Member

alxhub commented Jan 29, 2021

👍 to @petebacondarwin's requests.

Additionally, this PR is currently 4 separate commits - it should probably be squashed into a single commit with the full implementation, and a commit message describing the changes being made and why in more detail.

@petebacondarwin
Copy link
Contributor

petebacondarwin commented Apr 17, 2021

Reverting to #37852 due to a lack of response.

@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented May 18, 2021

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 May 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alxhub alxhub Awaiting requested review from alxhub

2 more reviewers

@alfaproject alfaproject alfaproject left review comments

@petebacondarwin petebacondarwin petebacondarwin requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: common/http Issues related to HTTP and HTTP Client cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

6 participants

@sera0731 @alxhub @petebacondarwin @alfaproject @googlebot @AndrewKushnir