The Wayback Machine - https://web.archive.org/web/20211020162956/https://github.com/angular/angular/pull/37873
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

Jump to bottom

fix(common): add params and report progress to put #37873

Closed
wants to merge 1 commit into from
Closed

fix(common): add params and report progress to put #37873

wants to merge 1 commit into from

Conversation

@ajitsinghkaler
Copy link
Contributor

@ajitsinghkaler ajitsinghkaler commented Jul 1, 2020

When response type is Json the put sigbnature did not have report progress and params key. This makes it difiicult to type check added them to the signature

Fixes #23600

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: #23600

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information
@ngbot ngbot bot added this to the needsTriage milestone Jul 1, 2020
jelbourn
jelbourn approved these changes Jul 16, 2020
View changes
Copy link
Member

@jelbourn jelbourn left a comment

LGTM

Reviewed-for: public-api

@ajitsinghkaler
Copy link
Contributor Author

@ajitsinghkaler ajitsinghkaler commented Jul 16, 2020

Do we need a google3 presubmit on this??

@pullapprove pullapprove bot requested a review from petebacondarwin Jul 16, 2020
petebacondarwin
petebacondarwin requested changes Jul 16, 2020
View changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

This seems like a perfectly reasonable fix unless someone can explain why this particular overload did not have these options? It looks like a simple oversight to me.

There are some typos in the commit message. Can you change it to:

fix(common): add `params` and `reportProgress` options to `HttpClient.put()` overload

When the response type is JSON, the `put()` overload signature did not have `reportProgress`
and  `params` options. This makes it difficult to type-check this overload.

This commit adds them to the overload signature.

Fixes #23600

@pullapprove pullapprove bot requested a review from petebacondarwin Jul 16, 2020
@ajitsinghkaler
Copy link
Contributor Author

@ajitsinghkaler ajitsinghkaler commented Jul 18, 2020

@petebacondarwin changed the message

petebacondarwin
petebacondarwin approved these changes Jul 18, 2020
View changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment •
edited

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from petebacondarwin Jul 18, 2020
petebacondarwin
petebacondarwin approved these changes Jul 18, 2020
View changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Reviewed-for: public-api-approvers

@pullapprove pullapprove bot requested review from AndrewKushnir and atscott and removed request for IgorMinar Jul 23, 2020
atscott
atscott approved these changes Jul 28, 2020
View changes
Copy link
Contributor

@atscott atscott left a comment

Reviewed-for: public-api

atscott
atscott approved these changes Jul 28, 2020
View changes
Copy link
Contributor

@atscott atscott left a comment

Reviewed-for: fw-core

AndrewKushnir
AndrewKushnir approved these changes Jul 28, 2020
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir removed the request for review from pkozlowski-opensource Jul 28, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 28, 2020

FYI, the remaining group that needs to give an LGTM is fw-http (@alxhub).

alxhub
alxhub approved these changes Jul 28, 2020
View changes
@ajitsinghkaler
Copy link
Contributor Author

@ajitsinghkaler ajitsinghkaler commented Sep 15, 2020 •
edited

@AndrewKushnir is there a problem with the presubmit. Should I change something in this.

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Sep 15, 2020

Hi @ajitsinghkaler, I've started a new presubmit and will let you know how it goes. Meanwhile, could you please rebase this PR so we re-run CI for this change with the most up-to-date version of master branch? Thank you.

@ajitsinghkaler
fix(common): add `params` and `reportProgress` options to `HttpClient…
f80c3d3 
….put()` overload

When the response type is JSON, the `put()` overload signature did not have `reportProgress`
and  `params` options. This makes it difficult to type-check this overload.

This commit adds them to the overload signature.

Fixes angular#23600
@ajitsinghkaler
Copy link
Contributor Author

@ajitsinghkaler ajitsinghkaler commented Sep 15, 2020

@AndrewKushnir rebased it.

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Sep 16, 2020

Hi @ajitsinghkaler, the presubmit went well. It looks like this PR has all approvals, so I'm adding the "merge" label. Thank you.

@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Oct 17, 2020

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 Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin

@atscott atscott

@jelbourn jelbourn

@alxhub alxhub

@AndrewKushnir AndrewKushnir

Assignees
No one assigned
Labels
action: merge cla: yes comp: common/http target: major
Projects
None yet
Milestone
needsTriage
Linked issues

Successfully merging this pull request may close these issues.

HttpClient#put with observe: events and generic as return type doesn't take in reportProgress: true
7 participants
@ajitsinghkaler @AndrewKushnir @petebacondarwin @atscott @jelbourn @alxhub @googlebot