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

Update HTTP guide #32045

Closed
wants to merge 3 commits into from
Closed

Conversation

jbogarthyde
Copy link
Contributor

@jbogarthyde jbogarthyde commented Aug 7, 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?

HTTP guide needs work -- long and confusing

Issue Number: N/A

What is the new behavior?

Edit and reorganize content - from unfinished PR #28519

Does this PR introduce a breaking change?

  • Yes
  • No

@jbogarthyde jbogarthyde requested a review from a team as a code owner August 7, 2019 17:45
@ngbot ngbot bot modified the milestone: Backlog Aug 7, 2019
@jbogarthyde jbogarthyde mentioned this pull request Aug 7, 2019
14 tasks
@mary-poppins
Copy link

You can preview 093c698 at https://pr32045-093c698.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 81e59d9 at https://pr32045-81e59d9.ngbuilds.io/.

aio/content/guide/http.md Outdated Show resolved Hide resolved
You can structure your `HttpClient` request to include the type of the response object, to make consuming the output easier and more obvious.
Specifying the response type acts as a type assertion during the compile time.

For example, the subscribe callback above requires bracket notation to extract the data values.

<code-example
path="http/src/app/config/config.component.ts"
region="v1_callback">
</code-example>

You can't write `data.heroesUrl` because TypeScript correctly complains that the `data` object from the service does not have a `heroesUrl` property.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't true - in the example the type is declared in the callback function, so TypeScript knows that data is of type Config and data.heroesUrl is valid to write. I don't know any circumstance in TypeScript where dot-property access isn't allowed but bracket access is. Under some configurations they can mean different things to the optimizer, but they both should be allowed.

I think you're looking for an example of the form:

.subscribe(data => this.config = {
    heroesUrl: (data as any).heroesUrl,
    textfile:  (data as any).textfile,
});

Here, data is of type Object, so accessing data.heroesUrl or data.textfile isn't allowed without the cast.

It's possible to specify the type in the callback function as done in the example currently, but that only works for that specific subscribe callback function - any RxJS operators, etc will also need the type specified. The advantage of specifying it as a type parameter to .get is that the entire Observable will be correctly typed, no matter where you store/pass it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restructured these sections - please check.

aio/content/guide/http.md Outdated Show resolved Hide resolved

<code-example
path="http/src/app/config/config.service.ts"
region="getConfig_2"
header="app/config/config.service.ts (getConfig v.2)">
</code-example>

<div class="alert is-important">

When you pass an interface or a class as a type parameter to the `HttpClient.get()` method, use the RxJS `map` operator to read the response data. Angular parses the response with `JSON.parse()`, but does not instantiate an object of the type parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When you pass an interface or a class as a type parameter to the `HttpClient.get()` method, use the RxJS `map` operator to read the response data. Angular parses the response with `JSON.parse()`, but does not instantiate an object of the type parameter.
When you pass an interface as a type parameter to the `HttpClient.get()` method, use the RxJS `map` operator to read the response data. Angular parses the response with `JSON.parse()`, but does not instantiate an object of the type parameter.

(changed to not mention classes)

Also, I think this section is misleading - map should be used to transform data, it has no special connection with type parameters. The role of map is to convert from the model the server returns to the model needed by the UI, so that subscribing can be done with the async pipe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check

aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
@mary-poppins
Copy link

You can preview 5a3821d at https://pr32045-5a3821d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 853a538 at https://pr32045-853a538.ngbuilds.io/.

@mary-poppins
Copy link

You can preview ad9916f at https://pr32045-ad9916f.ngbuilds.io/.

@jbogarthyde jbogarthyde self-assigned this Aug 14, 2019
@mary-poppins
Copy link

You can preview 662ec0f at https://pr32045-662ec0f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 041d9b7 at https://pr32045-041d9b7.ngbuilds.io/.

@mary-poppins
Copy link

You can preview e965f68 at https://pr32045-e965f68.ngbuilds.io/.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

please make the suggested adjustments, otherwise lgtm. thanks!

aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
@IgorMinar IgorMinar 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 Aug 21, 2019
@jbogarthyde jbogarthyde 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 Aug 21, 2019
@mary-poppins
Copy link

You can preview d251fa6 at https://pr32045-d251fa6.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 7cfe0ca at https://pr32045-7cfe0ca.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 37da89e at https://pr32045-37da89e.ngbuilds.io/.


The `HttpClient.get()` method parsed the JSON server response into the anonymous `Object` type. It doesn't know what the shape of that object is.
Notice that in the above example, the `subscribe` callback uses bracket notation to extract the data values.
You can't write `data.heroesUrl` because TypeScript correctly complains that the data object from the service does not have a `heroesUrl` property.
Copy link
Member

Choose a reason for hiding this comment

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

This is still not true - data['heroesUrl'] and data.heroesUrl` are the same in TypeScript, with the same type-checking rules applied. There isn't a circumstance where the former is valid and the latter isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just removing the whole paragraph, I hope that works.

aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
aio/content/guide/http.md Outdated Show resolved Hide resolved
@jbogarthyde jbogarthyde added this to Waiting for Approval in docs Aug 21, 2019
@mary-poppins
Copy link

You can preview 2a8c292 at https://pr32045-2a8c292.ngbuilds.io/.

@jbogarthyde jbogarthyde added the action: merge The PR is ready for merge by the caretaker label Aug 21, 2019
atscott pushed a commit that referenced this pull request Aug 22, 2019
@atscott atscott closed this in 21edc6a Aug 22, 2019
@jbogarthyde jbogarthyde moved this from Waiting for Approval to Done in docs Aug 22, 2019
@jbogarthyde jbogarthyde deleted the jb-http-guide branch August 26, 2019 18:14
ngdevelop-tech pushed a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
arnehoek pushed a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
@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 26, 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 aio: preview cla: yes effort2: days freq2: medium risk: low target: patch This PR is targeted for the next patch release type: bug/fix
Projects
docs
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants