feat(http): new HttpClient API #17143

alxhub · 2017-05-31T18:39:26Z

HttpClient is an evolution of the existing Angular HTTP API, which exists
alongside of it in a separate package, @angular/http/client. This structure
ensures that existing codebases can slowly migrate to the new API.

The new API improves significantly on the ergonomics and features of the legacy
API. A partial list of new features includes:

Typed, synchronous response body access, including support for JSON body types
JSON is an assumed default and no longer needs to be explicitly parsed
Interceptors allow middleware logic to be inserted into the pipeline
Immutable request/response objects
Progress events for both request upload and response download
Post-request verification & flush based testing framework

alxhub · 2017-05-31T18:41:34Z

I'm expecting this to have some issues on CI, but want to get the review started while I resolve them. I may need @jasonaden to help me debug the build process.

mary-poppins · 2017-05-31T21:42:25Z

The angular.io preview for 8ac3fae7610574468704c54bc4f02c6c52da09a5 is available here.

diicar · 2017-06-01T10:19:03Z

packages/http/client/src/request.ts

+      return 'text/plain';
+    }
+    // Arrays, objects, and numbers will be encoded as JSON.
+    if (typeof this.body === 'object' || typeof this.body === 'number' || Array.isArray(this.body)) {


the check typeof this.body === 'boolean' maybe needed here

Good catch, I'll add it (though the toString() serialization of boolean values is the same as the JSON encoding).

sod · 2017-06-01T12:48:21Z

packages/http/client/test/client_spec.ts

+          expect((res as any)['data']).toEqual('hello world');
+          done();
+        });
+        backend.expectOne('/test').flush({'data': 'hello world'});


thats an awesome improvement over the current ConnectionBackend hoopla 👍

mhevery · 2017-06-06T18:17:30Z

packages/http/client/public_api.ts

+ */
+
+export {HttpBackend, HttpHandler} from './src/backend';
+export * from './src/client_types';


please export things individually so that we don't accidentally expose API.

mhevery · 2017-06-06T18:42:56Z

packages/http/client/src/jsonp.ts

+/**
+ * Type for a map of JSONP callbacks.
+ */
+export interface JsonpAdapterCallbackMap {


Convert this to abstract class

mhevery · 2017-06-06T18:43:05Z

packages/http/client/src/jsonp.ts

+ * 
+ * In the browser, this should always be the `window` object.
+ */
+export const JSONP_ADAPTER_CALLBACK_MAP = new InjectionToken<JsonpAdapterCallbackMap>('JSONP_ADAPTER_CALLBACK_MAP');


remove this.

mhevery · 2017-06-06T18:43:16Z

packages/http/client/src/jsonp.ts

+ */
+@Injectable()
+export class JsonpClientBackend implements HttpBackend {
+  constructor(@Inject(JSONP_ADAPTER_CALLBACK_MAP) private callbackMap: JsonpAdapterCallbackMap, @Inject(DOCUMENT) private document: any) {}


no need for @Inject

mhevery · 2017-06-06T18:45:47Z

packages/http/client/src/jsonp.ts

+
+        // Remove the response callback from the callbackMap (window object in the
+        // browser).
+        delete this.callbackMap[callback];


When you use delete the object becomes slow forever. Consider this.callbackMap[...] = undefined

I think in this case, because it's window deleting is reasonable (and keeps from polluting it forever / leaking memory).

mhevery · 2017-06-06T19:06:06Z

packages/http/client/src/client.ts

+export class HttpClient {
+  constructor(private handler: HttpHandler) {}
+
+  request<R>(req: HttpRequest<any>): Observable<HttpEvent<R>>;


document these

mary-poppins · 2017-06-06T22:30:13Z

The angular.io preview for c83299dabb2695bc2817c40b38cdcfcef18f7a6e is available here.

cexbrayat · 2017-06-07T08:41:11Z

packages/http/client/src/request.ts

+ */
+function mightHaveBody(method: string): boolean {
+  switch (method) {
+    case 'DELETE':


In case this is not on purpose: DELETE is listed here as not having a body, but is in the HttpBodyMethod type above.

cexbrayat · 2017-06-07T08:54:47Z

packages/http/client/src/xhr.ts

+    // Quick check to give a better error message when a user attempts to use
+    // HttpClient.jsonp() without installing the JsonpClientModule
+    if (req.method === 'JSONP') {
+      throw new Error(`Attempted to construct Jsonp request without JsonpClientModule installed.`);


Looks like the module is HttpClientJsonpModule instead of JsonpClientModule

mary-poppins · 2017-06-07T17:56:55Z

The angular.io preview for b18bb9fe693bcc28a5d17b4587a59aced5a596a6 is available here.

mary-poppins · 2017-06-07T18:10:59Z

The angular.io preview for 4b770e260967b8802f058cb5f29446b8a82571ea is available here.

mary-poppins · 2017-06-14T17:52:03Z

The angular.io preview for 10db6e58ae76cf58fb1d9dac4610dfb491bccdf2 is available here.

mary-poppins · 2017-06-15T18:38:52Z

The angular.io preview for 5a2fe052c78d413313f2626aa085e4d8723e8e56 is available here.

mary-poppins · 2017-06-15T18:39:52Z

The angular.io preview for 27793b024fef212213dc9401f3d471e20164c2fd is available here.

mary-poppins · 2017-06-15T19:46:44Z

The angular.io preview for 0495b72f6c281fd3c45de246c1a2f23162f659ae is available here.

IgorMinar · 2017-07-06T17:38:38Z

tools/public_api_guard/common/http.d.ts

+}
+
+/** @experimental */
+export declare type HttpSerializedBody = ArrayBuffer | Blob | FormData | string;


IgorMinar · 2017-07-06T17:40:25Z

tools/public_api_guard/common/http.d.ts

+}
+
+/** @experimental */
+export declare type HttpMethod = 'POST' | 'PUT' | 'PATCH' | 'DELETE' | 'GET' | 'HEAD' | 'JSONP' | 'OPTIONS';


could we make this type internal? the public apis that use this type also use | string so the type is meaningless there.

mary-poppins · 2017-07-06T18:54:41Z

You can preview c4ae85a at https://pr17143-c4ae85a.ngbuilds.io/.

mary-poppins · 2017-07-06T19:01:49Z

You can preview ef6d58d at https://pr17143-ef6d58d.ngbuilds.io/.

mary-poppins · 2017-07-06T19:54:32Z

You can preview 1097bf2 at https://pr17143-1097bf2.ngbuilds.io/.

mary-poppins · 2017-07-06T20:50:07Z

You can preview cd73359 at https://pr17143-cd73359.ngbuilds.io/.

IgorMinar

expand the interceptor test and this is good to go.

🎉 🎈 🎆

mary-poppins · 2017-07-06T21:18:21Z

You can preview 475a86c at https://pr17143-475a86c.ngbuilds.io/.

HttpClient is an evolution of the existing Angular HTTP API, which exists alongside of it in a separate package, @angular/common/http. This structure ensures that existing codebases can slowly migrate to the new API. The new API improves significantly on the ergonomics and features of the legacy API. A partial list of new features includes: * Typed, synchronous response body access, including support for JSON body types * JSON is an assumed default and no longer needs to be explicitly parsed * Interceptors allow middleware logic to be inserted into the pipeline * Immutable request/response objects * Progress events for both request upload and response download * Post-request verification & flush based testing framework

mary-poppins · 2017-07-06T22:31:25Z

You can preview e37f23f at https://pr17143-e37f23f.ngbuilds.io/.

cexbrayat · 2017-07-07T07:45:08Z

@alxhub @IgorMinar I've got a few remaining questions if you don't mind.

I don't see how the query parameters are passed to the HttpClient. The old Http service allowed http.get(url, { params: { status: currentStatus }}), but I don't think it's possible with HttpClient?
You probably thought long about this, but @angular/common/http is really counter-intuitive :)
If expectOne is going to be the only matching method available for unit tests, can we have a third parameter to check the body and another to check the headers? The method field of RequestMatch could also be more strictly typed ('GET'|'POST'|...).

Currently testing a service doing a POST implies something like:

http
 .expectOne(req => {
   expect(req.method).toBe('POST');
   expect(req.url).toBe(`/api/users/1`);
   expect(req.body).toEqual(user);
   return true;
 })
 .flush(user);

whereas we could have:

http
 .expectOne('POST', '/api/users/1', user)
 .flush(user);

or (even better?):

http
 .expectPOST('/api/users/1', user)
 .flush(user);

The error message in a unit test when the request doesn't match what was expected is not very helpful: Expected one matching request, found none.. We'll need to see what was expected and what was the actual value.

If this is going to be merged soon, do you want me to open specific issues for some/all the points above?

IgorMinar · 2017-07-07T17:34:16Z

@cexbrayat thanks for the feedback!

"I don't see how the query parameters are passed to the HttpClient"
@alxhub is adding this feature
"@angular/common/http is really counter-intuitive"
I'd like us to consolidate on a fewer packages (i.e. merge all the platform packages into one, possibly move forms into common as well, etc)
like the idea of expectPOST, expectGET helper matchers with better error messages.

alxhub · 2017-07-07T18:01:15Z

@cexbrayat awesome feedback, thanks!

I don't see how the query parameters are passed to the HttpClient.

As Igor mentioned, this is coming to the API.

If expectOne is going to be the only matching method available for unit tests, can we have a third parameter to check the body and another to check the headers?

I'd rather not go down this route, actually. The new API considers request matching to be separate from assertion. I would write your POST body expectation as follows:

const req = http.expectOne({method: 'POST', url: '/api/users/1');
expect(req.request.body).toBe(user);
req.flush(user);

This example is actually broken:

http
 .expectOne(req => {
   expect(req.method).toBe('POST');
   expect(req.url).toBe(`/api/users/1`);
   expect(req.body).toEqual(user);
   return true;
 })
 .flush(user);

The function taken by expectOne() is a predicate - it will be called against every request currently queued. Those expect()s will fail if there are other, unrelated requests that happen to not match them.

jasonaden · 2017-07-07T19:11:54Z

Merged manually.

cexbrayat · 2017-07-08T15:02:06Z

@alxhub @IgorMinar Thank you for listening and taking the time to answer!

I followed with a few issues regarding params handling (potential bugs) and testing APIs (potential improvements :) ):

itsdevdom · 2017-07-17T20:44:21Z

Great work!

I'm curious, why did you guys put the new HTTP client into @angular/common/http and not into @angular/http? Because of breaking changes (if so, why not publish with v5)?
And what will happen to the "old" implementation (which, by the way, is still mentioned in the "Angular / Tutorial / HTTP" page)? Will the @angular/http package be deprecated and removed at some point in the future? Or will the new implementation replace the old one?

Just interested in the overall strategy here :)

angular-automatic-lock-bot · 2019-09-12T02:22:12Z

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

googlebot added the cla: yes label May 31, 2017

alxhub force-pushed the http-client branch 3 times, most recently from a03de4d to 8ac3fae Compare May 31, 2017 21:33

vicb added the comp: http label May 31, 2017

diicar reviewed Jun 1, 2017

View reviewed changes

sod reviewed Jun 1, 2017

View reviewed changes

robwormald self-assigned this Jun 3, 2017

aitboudad mentioned this pull request Jun 5, 2017

RFC: Http interceptors and transformers #2684

Closed

mhevery suggested changes Jun 6, 2017

View reviewed changes

mhevery previously requested changes Jun 6, 2017

View reviewed changes

alxhub force-pushed the http-client branch from 8ac3fae to c83299d Compare June 6, 2017 22:21

cexbrayat reviewed Jun 7, 2017

View reviewed changes

alxhub force-pushed the http-client branch from c83299d to b18bb9f Compare June 7, 2017 17:48

alxhub force-pushed the http-client branch from b18bb9f to 4b770e2 Compare June 7, 2017 17:59

alxhub force-pushed the http-client branch from 4b770e2 to 10db6e5 Compare June 14, 2017 17:25

sfabriece mentioned this pull request Jun 14, 2017

[feature request] support new angular HttpClient auth0/angular2-jwt#362

Closed

alxhub force-pushed the http-client branch 2 times, most recently from 5a2fe05 to 27793b0 Compare June 15, 2017 18:22

alxhub force-pushed the http-client branch from 27793b0 to 0495b72 Compare June 15, 2017 19:36

IgorMinar reviewed Jul 6, 2017

View reviewed changes

alxhub force-pushed the http-client branch 2 times, most recently from c4ae85a to ef6d58d Compare July 6, 2017 18:49

refactor: move DOCUMENT from platform/browser to common

3907a65

alxhub force-pushed the http-client branch from ef6d58d to 1097bf2 Compare July 6, 2017 19:45

alxhub force-pushed the http-client branch from 2150c08 to cd73359 Compare July 6, 2017 20:40

IgorMinar approved these changes Jul 6, 2017

View reviewed changes

alxhub force-pushed the http-client branch from cd73359 to 475a86c Compare July 6, 2017 21:02

alxhub added the action: merge The PR is ready for merge by the caretaker label Jul 6, 2017

alxhub force-pushed the http-client branch from 475a86c to e37f23f Compare July 6, 2017 22:16

jasonaden closed this Jul 7, 2017

alxhub deleted the http-client branch July 7, 2017 21:23

cexbrayat mentioned this pull request Jul 8, 2017

common/http: expectOne could help to check body and headers #18014

Closed

cexbrayat mentioned this pull request Jul 12, 2017

feat(http): add HttpTestingModule #13044

Closed

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

feat(http): new HttpClient API #17143

feat(http): new HttpClient API #17143

Uh oh!

Conversation

alxhub commented May 31, 2017

Uh oh!

alxhub commented May 31, 2017

Uh oh!

mary-poppins commented May 31, 2017

Uh oh!

diicar Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sod Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jun 7, 2017

Uh oh!

mary-poppins commented Jun 7, 2017

Uh oh!

mary-poppins commented Jun 14, 2017

Uh oh!

mary-poppins commented Jun 15, 2017

Uh oh!

mary-poppins commented Jun 15, 2017

Uh oh!

mary-poppins commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jul 6, 2017

Uh oh!

mary-poppins commented Jul 6, 2017

Uh oh!

mary-poppins commented Jul 6, 2017

Uh oh!

mary-poppins commented Jul 6, 2017

Uh oh!

diicar Jun 1, 2017 •

edited

Loading

alxhub Jun 1, 2017 •

edited

Loading

sod Jun 1, 2017 •

edited

Loading