Skip to content

fix(common): allow number or boolean as http params#40663

Closed
cexbrayat wants to merge 1 commit intoangular:masterfrom
cexbrayat:fix/http-params-type
Closed

fix(common): allow number or boolean as http params#40663
cexbrayat wants to merge 1 commit intoangular:masterfrom
cexbrayat:fix/http-params-type

Conversation

@cexbrayat
Copy link
Member

@cexbrayat cexbrayat commented Feb 2, 2021

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, the HTTP client expects string values for parameters, forcing us to cast numbers or boolean.
For example, if page is a number, we have to do something like:

this.http.get('/api/config', { params: { page: `${page}` } });

Issue Number: #23856 (but does not address the Date part of the issue)

What is the new behavior?

Allows to use number and boolean directly as HTTP params, instead of having to convert it to string first.

this.http.get('/api/config', { params: { page }});

HttpParams has also been updated to have most of its methods accept number or boolean values.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

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.

Generally looks good to me.
I think we might want to add to the tests. Should we consider cases where a param is set via one type (e.g. number) but then mutated/deleted via another type (e.g. string)?

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from petebacondarwin February 2, 2021 20:52
@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: common/http Issues related to HTTP and HTTP Client state: community Someone from the Angular community is working on this issue or submitted this PR target: minor This PR is targeted for the next minor release labels Feb 2, 2021
@ngbot ngbot bot modified the milestone: Backlog Feb 2, 2021
alxhub
alxhub previously requested changes Feb 2, 2021
@cexbrayat cexbrayat force-pushed the fix/http-params-type branch from 0d440da to 2569e75 Compare February 3, 2021 09:31
@cexbrayat
Copy link
Member Author

After discussion, we settled to use string|number|boolean|ReadonlyArray<string|number|boolean> everywhere.
The feedback should be addressed, you can take another look.

@cexbrayat cexbrayat force-pushed the fix/http-params-type branch from 2569e75 to f9f391b Compare February 3, 2021 10:21
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.

A couple of final NITs.

@petebacondarwin petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 3, 2021
@cexbrayat
Copy link
Member Author

@petebacondarwin Done!

@cexbrayat cexbrayat force-pushed the fix/http-params-type branch from f9f391b to 3974a1c Compare February 3, 2021 12:39
@petebacondarwin petebacondarwin removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 3, 2021
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

LGTM 🍪
I like how thorough these tests are. Thanks for adding them.

@petebacondarwin petebacondarwin dismissed alxhub’s stale review February 16, 2021 22:35

Requested changes were resolved.

@petebacondarwin petebacondarwin added the action: presubmit The PR is in need of a google3 presubmit label Feb 16, 2021
This was referenced Mar 15, 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 Mar 29, 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 Issues related to HTTP and HTTP Client breaking changes cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: medium state: community Someone from the Angular community is working on this issue or submitted this PR target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants