Skip to content

Conversation

@oBusk
Copy link
Contributor

@oBusk oBusk commented Feb 21, 2019

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

What is the new behavior?

Added observe: 'events' to the example code as well as added small section mentioning that it is needed.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Closes #28886

@ngbot ngbot bot added this to the needsTriage milestone Feb 21, 2019
@oBusk oBusk marked this pull request as ready for review February 21, 2019 18:01
@oBusk oBusk requested a review from a team February 21, 2019 18:01
@brandonroberts brandonroberts added effort1: hours target: patch This PR is targeted for the next patch release risk: low state: community Someone from the Angular community is working on this issue or submitted this PR refactoring Issue that involves refactoring or code-cleanup labels Feb 22, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Feb 22, 2019
@brandonroberts brandonroberts self-assigned this Feb 22, 2019
@mary-poppins
Copy link

You can preview 8bf66c1 at https://pr28892-8bf66c1.ngbuilds.io/.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove changes here and just add the note

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But is not observe default value body? So that observe: 'events' is needed?

Copy link
Contributor

@brandonroberts brandonroberts Feb 22, 2019

Choose a reason for hiding this comment

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

Unless I'm mistaken, providing an HttpRequest instance returns an observable of events by default.

https://github.com/angular/angular/blob/master/packages/common/http/src/client.ts#L497

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I see. I was confused by different syntax of new HttpRequest() vs httpClient.request(). Will update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes still needed then in context of the example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add to the existing note instead of two notes right next to each other.

Add `observe: 'events'` to see all events, including the progress of transfers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

The text still needs to be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry I missed that.

Since, as you pointed out, observing events is default when sending request with HttpRequest, is there some wording to clarify that observe: events is needed when using the HttpClient.request with config object syntax?

If using HttpClient.request('POST', '/url', config) syntax, add observe: 'events' to see all events, including the progress of transfers.

Copy link
Contributor

@brandonroberts brandonroberts Feb 22, 2019

Choose a reason for hiding this comment

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

What about:

"When using the HttpClient#request() method with an HTTP verb, configure with observe: 'events' to see all events, including the progress of transfers."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@brandonroberts brandonroberts 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 22, 2019
@mary-poppins
Copy link

You can preview c39b69e at https://pr28892-c39b69e.ngbuilds.io/.

@mary-poppins
Copy link

You can preview c07dcc1 at https://pr28892-c07dcc1.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d0a672b at https://pr28892-d0a672b.ngbuilds.io/.

@brandonroberts
Copy link
Contributor

brandonroberts commented Feb 22, 2019

@oBusk will you rebase on master so the preview job can finish correctly? And also squash down to a single commit.

Thanks

@mary-poppins
Copy link

You can preview 8df1697 at https://pr28892-8df1697.ngbuilds.io/.

@brandonroberts
Copy link
Contributor

Couple more changes.

  • Update text to "When using HttpClient#request() with an HTTP method, ..."
  • Remove (http) from the git commit message, so its just docs: clarify documentation about listening to progress events

@brandonroberts brandonroberts changed the title docs(http): clarify documentation about listening to progress events docs: clarify documentation about listening to progress events Feb 22, 2019
@mary-poppins
Copy link

You can preview 5fb1e31 at https://pr28892-5fb1e31.ngbuilds.io/.

@brandonroberts brandonroberts added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 22, 2019
@brandonroberts
Copy link
Contributor

Thanks @oBusk!

@oBusk
Copy link
Contributor Author

oBusk commented Feb 22, 2019

@brandonroberts Thanks for the feedback! 👍 🎆

@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 Sep 14, 2019
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 cla: yes effort1: hours refactoring Issue that involves refactoring or code-cleanup risk: low state: community Someone from the Angular community is working on this issue or submitted this PR target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docs: http reportProgress requires observe events

5 participants