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

feat(http): Introduction of the fetch Backend for the HttpClient #50247

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ nodejs_register_toolchains(
node_version = "16.14.0",
)

nodejs_register_toolchains(
name = "node18",
node_version = "18.10.0",
)

# Download npm dependencies.
load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
load("//integration:npm_package_archives.bzl", "npm_package_archives")
Expand Down
17 changes: 16 additions & 1 deletion goldens/public-api/common/http/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ import { Observable } from 'rxjs';
import { Provider } from '@angular/core';
import { XhrFactory } from '@angular/common';

// @public
export class FetchBackend implements HttpBackend {
// (undocumented)
handle(req: HttpRequest<any>): Observable<HttpEvent<any>>;
// (undocumented)
static ɵfac: i0.ɵɵFactoryDeclaration<FetchBackend, never>;
// (undocumented)
static ɵprov: i0.ɵɵInjectableDeclaration<FetchBackend>;
}

// @public
export const HTTP_INTERCEPTORS: InjectionToken<HttpInterceptor[]>;

Expand Down Expand Up @@ -1741,6 +1751,8 @@ export enum HttpFeatureKind {
// (undocumented)
CustomXsrfConfiguration = 2,
// (undocumented)
Fetch = 6,
// (undocumented)
Interceptors = 0,
// (undocumented)
JsonpSupport = 4,
Expand Down Expand Up @@ -1783,7 +1795,7 @@ export class HttpHeaderResponse extends HttpResponseBase {
export class HttpHeaders {
constructor(headers?: string | {
[name: string]: string | number | (string | number)[];
});
} | Headers);
append(name: string, value: string | string[]): HttpHeaders;
delete(name: string, value?: string | string[]): HttpHeaders;
get(name: string): string | null;
Expand Down Expand Up @@ -2165,6 +2177,9 @@ export class JsonpInterceptor {
// @public
export function provideHttpClient(...features: HttpFeature<HttpFeatureKind>[]): EnvironmentProviders;

// @public
export function withFetch(): HttpFeature<HttpFeatureKind.Fetch>;

// @public
export function withInterceptors(interceptorFns: HttpInterceptorFn[]): HttpFeature<HttpFeatureKind.Interceptors>;

Expand Down
3 changes: 2 additions & 1 deletion packages/common/http/public_api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@
export {HttpBackend, HttpHandler} from './src/backend';
export {HttpClient} from './src/client';
export {HttpContext, HttpContextToken} from './src/context';
export {FetchBackend} from './src/fetch';
export {HttpHeaders} from './src/headers';
export {HTTP_INTERCEPTORS, HttpHandlerFn, HttpInterceptor, HttpInterceptorFn, HttpInterceptorHandler as ɵHttpInterceptorHandler, HttpInterceptorHandler as ɵHttpInterceptingHandler} from './src/interceptor';
export {JsonpClientBackend, JsonpInterceptor} from './src/jsonp';
export {HttpClientJsonpModule, HttpClientModule, HttpClientXsrfModule} from './src/module';
export {HttpParameterCodec, HttpParams, HttpParamsOptions, HttpUrlEncodingCodec} from './src/params';
export {HttpFeature, HttpFeatureKind, provideHttpClient, withInterceptors, withInterceptorsFromDi, withJsonpSupport, withNoXsrfProtection, withRequestsMadeViaParent, withXsrfConfiguration} from './src/provider';
export {HttpFeature, HttpFeatureKind, provideHttpClient, withFetch, withInterceptors, withInterceptorsFromDi, withJsonpSupport, withNoXsrfProtection, withRequestsMadeViaParent, withXsrfConfiguration} from './src/provider';
export {HttpRequest} from './src/request';
export {HttpDownloadProgressEvent, HttpErrorResponse, HttpEvent, HttpEventType, HttpHeaderResponse, HttpProgressEvent, HttpResponse, HttpResponseBase, HttpSentEvent, HttpStatusCode, HttpUploadProgressEvent, HttpUserEvent} from './src/response';
export {withHttpTransferCache as ɵwithHttpTransferCache} from './src/transfer_cache';
Expand Down
251 changes: 251 additions & 0 deletions packages/common/http/src/fetch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,251 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {inject, Injectable} from '@angular/core';
import {BehaviorSubject, defer, merge, Observable} from 'rxjs';
import {finalize} from 'rxjs/operators';

import {HttpBackend} from './backend';
import {HttpHeaders} from './headers';
import {HttpRequest} from './request';
import {HttpDownloadProgressEvent, HttpErrorResponse, HttpEvent, HttpEventType, HttpHeaderResponse, HttpResponse, HttpStatusCode} from './response';

const XSSI_PREFIX = /^\)\]\}',?\n/;

const REQUEST_URL_HEADER = `X-Request-URL`;

/**
* Determine an appropriate URL for the response, by checking either
* response url or the X-Request-URL header.
*/
function getResponseUrl(response: Response): string|null {
if (response.url) {
return response.url;
}
// stored as lowercase in the map
const xRequestUrl = REQUEST_URL_HEADER.toLocaleLowerCase();
return response.headers.get(xRequestUrl);
}

/**
* Uses `fetch` to send requests to a backend server.
*
* This `FetchBackend` requires the support of the
* [Fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) which is available on all
* supported browsers and on Node.js v18 or later.
*
* @see {@link HttpHandler}
*
* @publicApi
* @developerPreview
*/
@Injectable()
export class FetchBackend implements HttpBackend {
// We need to bind the native fetch to its context or it will throw an "illegal invocation"
private readonly fetchImpl =
inject(FetchFactory, {optional: true})?.fetch ?? fetch.bind(globalThis);

handle(req: HttpRequest<any>): Observable<HttpEvent<any>> {
Copy link

Choose a reason for hiding this comment

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

is this method guaranteed to be called for each subscription? if not, this whole method body should be wrapped in defer(() => { /* ... */ }). a previous implementation did something similar by returning new Observable().

I just want to make sure that multiple subscriptions to the same request Observable don't affect each other:

const r = http.get('.');
r.subscribe().unsubscribe();
r.subscribe(console.log); // should still work and not reuse the abortController of the first request

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand your wories here but here is the call site:

const events$: Observable<HttpEvent<any>> =
of(req).pipe(concatMap((req: HttpRequest<any>) => this.handler.handle(req)));

Copy link

Choose a reason for hiding this comment

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

XhrBackend contains code exactly for this purpose:

// Everything happens on Observable subscription.
return new Observable((observer: Observer<HttpEvent<any>>) => {

based on your comment it seems that is no longer necessary or at least the comment is outdated

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

HttpClient may lazily call handle, but in the middle of the chain any interceptor is also free to create many intermediate request Observables, and may not subscribe to them. Just calling handle shouldn't trigger a request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx for the insight Alex, @alan-agius4 that leaves us with no other choice than a floating promise right ? (cf my latest commit)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, you can refactor the code to avoid using the observer.

I think something like the below should be enough (I did not test it, just pseudo code)

merge(
  of(null).pipe(
    switchMap(() => this.doRequest(req, abortController.signal, (event) => eventStream.next(event)).finally(() => eventStream.complete()))
  ),
  eventStream,
)

Copy link
Member

Choose a reason for hiding this comment

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

Floating promises are normal and reasonable. Under the hood, switchMap is going to take the promise returned by doRequest and do .then(...) to emit into the observer when it completes (basically, it creates a floating promise).

// Deferring to allow creating a new AbortController on retry()
return defer(() => {
const abortController = new AbortController();

// TODO: use `AsyncGenerators` instead of `BehaviorSubject` when we no longer support RXJS 6.x
// We can't use it now because from() doesn't accept generators on RxJS 6.
// Just replace `reportEvent` calls with `yield`.
const eventStream = new BehaviorSubject<HttpEvent<any>>({type: HttpEventType.Sent});

// Everything happens on Observable subscription.
return merge(
defer(() => {
return this
.doRequest(req, abortController.signal, (event) => eventStream.next(event))
.finally(() => eventStream.complete());
}),
eventStream,
)
.pipe(finalize(() => {
// Aborting the fetch call when the observable is unsubscribed
abortController.abort();
}));
});
}

private async doRequest(
request: HttpRequest<any>, signal: AbortSignal,
reportEvent: (event: HttpEvent<any>) => void): Promise<HttpResponse<any>> {
const init = this.createRequestInit(request);
let response;

try {
response = await this.fetchImpl(request.url, {signal, ...init});
} catch (error: any) {
throw new HttpErrorResponse({
error,
status: error.status ?? 0,
statusText: error.statusText,
url: request.url,
headers: error.headers,
});
}

const headers = new HttpHeaders(response.headers);
const statusText = response.statusText;
const url = getResponseUrl(response) ?? request.url;

let status = response.status;
let body: string|ArrayBuffer|Blob|object|null = null;

if (request.reportProgress) {
reportEvent(new HttpHeaderResponse({headers, status, statusText, url}));
}

if (response.body) {
// Read Progess
const contentLength = response.headers.get('content-length');
const chunks: Uint8Array[] = [];
const reader = response.body.getReader();
let receivedLength = 0;

let decoder: TextDecoder;
let partialText: string|undefined;

while (true) {
const {done, value} = await reader.read();
JeanMeche marked this conversation as resolved.
Show resolved Hide resolved

if (done) {
break;
}

chunks.push(value);
receivedLength += value.length;

if (request.reportProgress) {
partialText = request.responseType === 'text' ?
(partialText ?? '') + (decoder ??= new TextDecoder).decode(value, {stream: true}) :
undefined;

reportEvent({
type: HttpEventType.DownloadProgress,
total: contentLength ? +contentLength : undefined,
loaded: receivedLength,
partialText,
} as HttpDownloadProgressEvent);
}
}

// Combine all chunks.
const chunksAll = this.concatChunks(chunks, receivedLength);

body = this.parseBody(request, response, chunksAll);
}

// Same behavior as the XhrBackend
if (status === 0) {
status = body ? HttpStatusCode.Ok : 0;
}

// ok determines whether the response will be transmitted on the event or
// error channel. Unsuccessful status codes (not 2xx) will always be errors,
// but a successful status code can still result in an error if the user
// asked for JSON data and the body cannot be parsed as such.
const ok = status >= 200 && status < 300;

if (ok) {
return new HttpResponse({
body,
headers,
status,
statusText,
url,
});
}

throw new HttpErrorResponse({
JeanMeche marked this conversation as resolved.
Show resolved Hide resolved
error: body,
headers,
status,
statusText,
url,
});
}

private parseBody(request: HttpRequest<any>, response: Response, binContent: Uint8Array): string
|ArrayBuffer|Blob|object|null {
try {
switch (request.responseType) {
case 'json':
// stripping the XSSI when present
const text = new TextDecoder().decode(binContent).replace(XSSI_PREFIX, '');
return text === '' ? null : JSON.parse(text) as object;
case 'text':
return new TextDecoder().decode(binContent);
case 'blob':
return new Blob([binContent]);
case 'arraybuffer':
return binContent.buffer;
}
} catch (error) {
// body loading or parsing failed
throw new HttpErrorResponse({
error,
headers: new HttpHeaders(response.headers),
status: response.status,
statusText: response.statusText,
url: getResponseUrl(response) ?? request.url,
});
}
}

private createRequestInit(req: HttpRequest<any>): RequestInit {
// We could share some of this logic with the XhrBackend

const headers: Record<string, string> = {};
const credentials: RequestCredentials|undefined = req.withCredentials ? 'include' : undefined;

// Setting all the requested headers.
req.headers.forEach((name, values) => (headers[name] = values.join(',')));

// Add an Accept header if one isn't present already.
headers['Accept'] ??= 'application/json, text/plain, */*';

// Auto-detect the Content-Type header if one isn't present already.
if (!headers['Content-Type']) {
const detectedType = req.detectContentTypeHeader();
// Sometimes Content-Type detection fails.
if (detectedType !== null) {
headers['Content-Type'] = detectedType;
}
}
JeanMeche marked this conversation as resolved.
Show resolved Hide resolved

return {
body: req.body,
method: req.method,
headers,
credentials,
};
}

private concatChunks(chunks: Uint8Array[], totalLength: number): Uint8Array {
const chunksAll = new Uint8Array(totalLength);
let position = 0;
for (const chunk of chunks) {
chunksAll.set(chunk, position);
position += chunk.length;
}

return chunksAll;
}
}

/**
* Abstract class to provide a mocked implementation of `fetch()`
*/
export abstract class FetchFactory {
JeanMeche marked this conversation as resolved.
Show resolved Hide resolved
JeanMeche marked this conversation as resolved.
Show resolved Hide resolved
abstract fetch: typeof fetch;
}
33 changes: 16 additions & 17 deletions packages/common/http/src/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export class HttpHeaders {

/** Constructs a new HTTP header object with the given values.*/

constructor(headers?: string|{[name: string]: string | number | (string | number)[]}) {
constructor(headers?: string|{[name: string]: string | number | (string | number)[]}|Headers) {
if (!headers) {
this.headers = new Map<string, string[]>();
} else if (typeof headers === 'string') {
Expand All @@ -66,28 +66,19 @@ export class HttpHeaders {
}
});
};
} else if (typeof Headers !== 'undefined' && headers instanceof Headers) {
this.headers = new Map<string, string[]>();
headers.forEach((values: string, name: string) => {
this.setHeaderEntries(name, values);
});
} else {
this.lazyInit = () => {
if (typeof ngDevMode === 'undefined' || ngDevMode) {
assertValidHeaders(headers);
}
this.headers = new Map<string, string[]>();
Object.entries(headers).forEach(([name, values]) => {
let headerValues: string[];

if (typeof values === 'string') {
headerValues = [values];
} else if (typeof values === 'number') {
headerValues = [values.toString()];
} else {
headerValues = values.map((value) => value.toString());
}

if (headerValues.length > 0) {
const key = name.toLowerCase();
this.headers.set(key, headerValues);
this.maybeSetNormalizedName(name, key);
}
this.setHeaderEntries(name, values);
});
};
}
Expand Down Expand Up @@ -258,6 +249,14 @@ export class HttpHeaders {
}
}

private setHeaderEntries(name: string, values: any) {
const headerValues =
(Array.isArray(values) ? values : [values]).map((value) => value.toString());
const key = name.toLowerCase();
this.headers.set(key, headerValues);
this.maybeSetNormalizedName(name, key);
}

/**
* @internal
*/
Expand All @@ -273,7 +272,7 @@ export class HttpHeaders {
* must be either strings, numbers or arrays. Throws an error if an invalid
* header value is present.
*/
function assertValidHeaders(headers: Record<string, unknown>):
function assertValidHeaders(headers: Record<string, unknown>|Headers):
asserts headers is Record<string, string|string[]|number|number[]> {
for (const [key, value] of Object.entries(headers)) {
if (!(typeof value === 'string' || typeof value === 'number') && !Array.isArray(value)) {
Expand Down