Quick comment: from API perspective it looks a bit unusual that the values are always converted to strings, i.e. in the following example:

const body = new HttpParams({fromObject: {b: 2}});
const b = body.get('b');

As an API user I may expect the original value to be returned (i.e. 2 as a number, but not as a string). Do we base this behavior on the params representation in a URL (i.e. ?b=2 in my case), which would always be a string?

Should we consider changing the get (and related methods) API signature instead?

Note: I'm not sure what the right approach is, just want to check if changing the get signature was considered and if yes, we can probably document that in the commit message and PR description for future reference + expand docs for get function to mention that the returned value would always be converted to a string (and why we do that).

Presubmit

@AndrewKushnir Thanks for your feedback. I have no strong opinions on this. This fix is a good first step (as the current behavior is broken), but if the team decides to change the signatures of get and others, I can probably help with that.

@AndrewKushnir Thanks for your feedback. I have no strong opinions on this. This fix is a good first step (as the current behavior is broken), but if the team decides to change the signatures of get and others, I can probably help with that.

I agree, we will schedule some time at the next framework meeting to discuss the public API for the HttpParams class.

I see that you just added the action: merge label, but the following checks are still failing:
     status "ci/angular: size" is failing
     status "ci/circleci: build-ivy-npm-packages" is failing
     status "ci/circleci: build-npm-packages" is failing
     status "ci/circleci: legacy-unit-tests-saucelabs" is failing
     status "ci/circleci: test" is failing
     status "ci/circleci: test_ivy_aot" is failing
     status "ci/circleci: test_zonejs" is failing
     status "google3" is failing
     status "ci/circleci: setup" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

The presubmit finished, but failed to report back. There are two legit test failures that exactly test against HttpParams, and are failing for the expected reasons. We'd have to fix those two tests forward along with this.

Should we consider changing the get (and related methods) API signature instead?

Discussed in our team meeting. We should not change the public API of this class to allow non-strings to be returned.

The aim of the class is to provide a stringified list of params for HTTP requests, not as a general purpose container for values.
Also if anything we should be aligning our API (and eventually, perhaps, our implementation) with the native URLSearchParams type.

@AndrewKushnir Thanks for your feedback. I have no strong opinions on this. This fix is a good first step (as the current behavior is broken), but if the team decides to change the signatures of get and others, I can probably help with that.

@cexbrayat thanks for the reply. I've looked at the code once again and found out that the append and appendAll methods also accept a union of string|number|boolean, but convert the values to strings later. So converting initial values to strings (the logic added in this PR) makes sense and is consistent with other parts of that class.

I'm adding the "blocked" label for now due to some failing tests in Google's codebase that would require a cleanup. We'd also need to run a global check in Google's codebase to see if we have more instances.

Is it possible to directly re-encapsulate the native URLSearchParams? Rather than manually maintaining the map.

@HyperLife1119 - do you mean for the HttpParams implementation to use the URLSearchParams internally?

do you mean for the HttpParams implementation to use the URLSearchParams internally?

Yes, but Angular currently does not do this, I want to know the reason.

URLSearchParams is not supported in IE11 - so we could not use it without a polyfill.
But in 13.0.0 support for IE11 is dropped from Angular so this could become an option.

URLSearchParams is not supported in IE11 - so we could not use it without a polyfill. But in 13.0.0 support for IE11 is dropped from Angular so this could become an option.

Thanks, I will continue to pay attention to this PR.

Global Presubmit (internal-only link).

@AndrewKushnir @cexbrayat Is this something we intend to land in 14?

If so, I kicked off a de-flake run of failures from the above TGP.

@dylhunn That was the idea, yes

Looking at the presubmit results, some cleanup in g3 is needed. We're squeaking in a bit late (freeze is Wednesday). Andrew and I will have a look at what's needed, but this may have slipped by us again 🤦

I'll post an update after our rerun finishes.

Quick update: we are waiting for some cleanup in Google's codebase to land. After that we'll rerun the necessary tests (TGP) and if everything goes well, we should be able to proceed with the merge of this PR.

I am running another TGP with all g3 patches applied:

• Initial Run
• Deflake Run
• Rerun at head

Edit: Looks like all failures are preexisting.

merge-assistance: we are still waiting for one g3 team to submit the fix we provided.

This PR was merged into the repository by commit 10691c6.

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