Skip to content

Commit

Permalink
fix(http): resolve withRequestsMadeViaParent behavior with `withFet…
Browse files Browse the repository at this point in the history
…ch` (#55652)

This commit addresses dependency injection defects when using the `withFetch` API. Formerly, utilizing `withFetch` led to the automatic setting of `HttpBackend` to `FetchBackend`, which proved problematic in certain scenarios. Notably, conflicts arose when integrating `withRequestsMadeViaParent` and manually overriding tokens, as observed in instances like `InMemoryWebApiModule`.

PR Close #55652
  • Loading branch information
alan-agius4 authored and AndrewKushnir committed May 6, 2024
1 parent 8459ee4 commit d9b339f
Show file tree
Hide file tree
Showing 7 changed files with 127 additions and 125 deletions.
3 changes: 1 addition & 2 deletions adev/src/content/examples/ssr/src/app/app.config.ts
Expand Up @@ -14,8 +14,7 @@ import {InMemoryDataService} from './in-memory-data.service';
export const appConfig: ApplicationConfig = {
providers: [
provideRouter(routes),
// TODO: Enable using Fetch API when disabling `HttpClientInMemoryWebApiModule`.
provideHttpClient(/* withFetch()*/),
provideHttpClient(withFetch()),
provideClientHydration(),
provideProtractorTestingSupport(), // essential for e2e testing

Expand Down
13 changes: 0 additions & 13 deletions packages/common/http/src/interceptor.ts
Expand Up @@ -215,13 +215,6 @@ export const HTTP_ROOT_INTERCEPTOR_FNS = new InjectionToken<readonly HttpInterce
ngDevMode ? 'HTTP_ROOT_INTERCEPTOR_FNS' : '',
);

/**
* A provider to set a global primary http backend. If set, it will override the default one
*/
export const PRIMARY_HTTP_BACKEND = new InjectionToken<HttpBackend>(
ngDevMode ? 'PRIMARY_HTTP_BACKEND' : '',
);

// TODO(atscott): We need a larger discussion about stability and what should contribute to stability.
// Should the whole interceptor chain contribute to stability or just the backend request #55075?
// Should HttpClient contribute to stability automatically at all?
Expand Down Expand Up @@ -280,12 +273,6 @@ export class HttpInterceptorHandler extends HttpHandler {
) {
super();

// Check if there is a preferred HTTP backend configured and use it if that's the case.
// This is needed to enable `FetchBackend` globally for all HttpClient's when `withFetch`
// is used.
const primaryHttpBackend = inject(PRIMARY_HTTP_BACKEND, {optional: true});
this.backend = primaryHttpBackend ?? backend;

// We strongly recommend using fetch backend for HTTP calls when SSR is used
// for an application. The logic below checks if that's the case and produces
// a warning otherwise.
Expand Down
1 change: 0 additions & 1 deletion packages/common/http/src/private_export.ts
Expand Up @@ -8,6 +8,5 @@

export {
HTTP_ROOT_INTERCEPTOR_FNS as ɵHTTP_ROOT_INTERCEPTOR_FNS,
PRIMARY_HTTP_BACKEND as ɵPRIMARY_HTTP_BACKEND,
REQUESTS_CONTRIBUTE_TO_STABILITY as ɵREQUESTS_CONTRIBUTE_TO_STABILITY,
} from './interceptor';
21 changes: 6 additions & 15 deletions packages/common/http/src/provider.ts
Expand Up @@ -22,7 +22,6 @@ import {
HttpInterceptorFn,
HttpInterceptorHandler,
legacyInterceptorFnFactory,
PRIMARY_HTTP_BACKEND,
} from './interceptor';
import {
jsonpCallbackContext,
Expand Down Expand Up @@ -126,7 +125,12 @@ export function provideHttpClient(
HttpXhrBackend,
HttpInterceptorHandler,
{provide: HttpHandler, useExisting: HttpInterceptorHandler},
{provide: HttpBackend, useExisting: HttpXhrBackend},
{
provide: HttpBackend,
useFactory: () => {
return inject(FetchBackend, {optional: true}) ?? inject(HttpXhrBackend);
},
},
{
provide: HTTP_INTERCEPTOR_FNS,
useValue: xsrfInterceptorFn,
Expand Down Expand Up @@ -294,26 +298,13 @@ export function withRequestsMadeViaParent(): HttpFeature<HttpFeatureKind.Request
/**
* Configures the current `HttpClient` instance to make requests using the fetch API.
*
* This `FetchBackend` requires the support of the Fetch API which is available on all evergreen
* browsers and on NodeJS from v18 onward.
*
* Note: The Fetch API doesn't support progress report on uploads.
*
* @publicApi
*/
export function withFetch(): HttpFeature<HttpFeatureKind.Fetch> {
if ((typeof ngDevMode === 'undefined' || ngDevMode) && typeof fetch !== 'function') {
// TODO: Create a runtime error
// TODO: Use ENVIRONMENT_INITIALIZER to contextualize the error message (browser or server)
throw new Error(
'The `withFetch` feature of HttpClient requires the `fetch` API to be available. ' +
'If you run the code in a Node environment, make sure you use Node v18.10 or later.',
);
}

return makeHttpFeature(HttpFeatureKind.Fetch, [
FetchBackend,
{provide: HttpBackend, useExisting: FetchBackend},
{provide: PRIMARY_HTTP_BACKEND, useExisting: FetchBackend},
]);
}
208 changes: 118 additions & 90 deletions packages/common/http/test/provider_spec.ts
Expand Up @@ -40,6 +40,8 @@ import {EMPTY, Observable, from} from 'rxjs';

import {HttpInterceptorFn, resetFetchBackendWarningFlag} from '../src/interceptor';
import {
HttpFeature,
HttpFeatureKind,
provideHttpClient,
withFetch,
withInterceptors,
Expand Down Expand Up @@ -339,94 +341,107 @@ describe('provideHttpClient', () => {
});

describe('withRequestsMadeViaParent()', () => {
it('should have independent HTTP setups if not explicitly specified', async () => {
TestBed.configureTestingModule({
providers: [provideHttpClient(), provideHttpClientTesting()],
});

const child = createEnvironmentInjector(
[
provideHttpClient(),
{
provide: XhrFactory,
useValue: {
build: () => {
throw new Error('Request reached the "backend".');
for (const backend of ['fetch', 'xhr']) {
describe(`given '${backend}' backend`, () => {
const commonHttpFeatures: HttpFeature<HttpFeatureKind>[] = [];
if (backend === 'fetch') {
commonHttpFeatures.push(withFetch());
}

it('should have independent HTTP setups if not explicitly specified', async () => {
TestBed.configureTestingModule({
providers: [provideHttpClient(...commonHttpFeatures), provideHttpClientTesting()],
});

const child = createEnvironmentInjector(
[
provideHttpClient(),
{
provide: XhrFactory,
useValue: {
build: () => {
throw new Error('Request reached the "backend".');
},
},
},
},
},
],
TestBed.inject(EnvironmentInjector),
);

// Because `child` is an entirely independent HTTP context, it is not connected to the
// HTTP testing backend from the parent injector, and requests attempted via the child's
// `HttpClient` will fail.
await expectAsync(child.get(HttpClient).get('/test').toPromise()).toBeRejected();
});

it('should connect child to parent configuration if specified', () => {
TestBed.configureTestingModule({
providers: [provideHttpClient(), provideHttpClientTesting()],
});

const child = createEnvironmentInjector(
[provideHttpClient(withRequestsMadeViaParent())],
TestBed.inject(EnvironmentInjector),
);

// `child` is now to the parent HTTP context and therefore the testing backend, and so a
// request made via its `HttpClient` can be made.
child.get(HttpClient).get('/test', {responseType: 'text'}).subscribe();
const req = TestBed.inject(HttpTestingController).expectOne('/test');
req.flush('');
});

it('should include interceptors from both parent and child contexts', () => {
TestBed.configureTestingModule({
providers: [
provideHttpClient(withInterceptors([makeLiteralTagInterceptorFn('parent')])),
provideHttpClientTesting(),
],
});
],
TestBed.inject(EnvironmentInjector),
);

// Because `child` is an entirely independent HTTP context, it is not connected to the
// HTTP testing backend from the parent injector, and requests attempted via the child's
// `HttpClient` will fail.
await expectAsync(child.get(HttpClient).get('/test').toPromise()).toBeRejected();
});

const child = createEnvironmentInjector(
[
provideHttpClient(
withRequestsMadeViaParent(),
withInterceptors([makeLiteralTagInterceptorFn('child')]),
),
],
TestBed.inject(EnvironmentInjector),
);
it('should connect child to parent configuration if specified', () => {
TestBed.configureTestingModule({
providers: [provideHttpClient(...commonHttpFeatures), provideHttpClientTesting()],
});

const child = createEnvironmentInjector(
[provideHttpClient(withRequestsMadeViaParent())],
TestBed.inject(EnvironmentInjector),
);

// `child` is now to the parent HTTP context and therefore the testing backend, and so a
// request made via its `HttpClient` can be made.
child.get(HttpClient).get('/test', {responseType: 'text'}).subscribe();
const req = TestBed.inject(HttpTestingController).expectOne('/test');
req.flush('');
});

child.get(HttpClient).get('/test', {responseType: 'text'}).subscribe();
const req = TestBed.inject(HttpTestingController).expectOne('/test');
expect(req.request.headers.get('X-Tag')).toEqual('child,parent');
req.flush('');
});
it('should include interceptors from both parent and child contexts', () => {
TestBed.configureTestingModule({
providers: [
provideHttpClient(
...commonHttpFeatures,
withInterceptors([makeLiteralTagInterceptorFn('parent')]),
),
provideHttpClientTesting(),
],
});

const child = createEnvironmentInjector(
[
provideHttpClient(
withRequestsMadeViaParent(),
withInterceptors([makeLiteralTagInterceptorFn('child')]),
),
],
TestBed.inject(EnvironmentInjector),
);

child.get(HttpClient).get('/test', {responseType: 'text'}).subscribe();
const req = TestBed.inject(HttpTestingController).expectOne('/test');
expect(req.request.headers.get('X-Tag')).toEqual('child,parent');
req.flush('');
});

it('should be able to connect to a legacy-provided HttpClient context', () => {
TestBed.configureTestingModule({
imports: [HttpClientTestingModule],
providers: [provideLegacyInterceptor('parent')],
it('should be able to connect to a legacy-provided HttpClient context', () => {
TestBed.configureTestingModule({
imports: [HttpClientTestingModule],
providers: [provideLegacyInterceptor('parent')],
});

const child = createEnvironmentInjector(
[
provideHttpClient(
...commonHttpFeatures,
withRequestsMadeViaParent(),
withInterceptors([makeLiteralTagInterceptorFn('child')]),
),
],
TestBed.inject(EnvironmentInjector),
);

child.get(HttpClient).get('/test', {responseType: 'text'}).subscribe();
const req = TestBed.inject(HttpTestingController).expectOne('/test');
expect(req.request.headers.get('X-Tag')).toEqual('child,parent');
req.flush('');
});
});

const child = createEnvironmentInjector(
[
provideHttpClient(
withRequestsMadeViaParent(),
withInterceptors([makeLiteralTagInterceptorFn('child')]),
),
],
TestBed.inject(EnvironmentInjector),
);

child.get(HttpClient).get('/test', {responseType: 'text'}).subscribe();
const req = TestBed.inject(HttpTestingController).expectOne('/test');
expect(req.request.headers.get('X-Tag')).toEqual('child,parent');
req.flush('');
});
}
});

describe('compatibility with Http NgModules', () => {
Expand Down Expand Up @@ -472,20 +487,33 @@ describe('provideHttpClient', () => {
expect(consoleWarnSpy.calls.count()).toBe(0);
});

it('withFetch should always override the backend', () => {
it(`'withFetch' should not override provided backend`, () => {
class CustomBackendExtends extends HttpXhrBackend {}

TestBed.resetTestingModule();
TestBed.configureTestingModule({
providers: [
provideHttpClient(withFetch()),
// This emulates a situation when `provideHttpClient()` is used
// later in a different part of an app. We want to make sure that
// the `FetchBackend` is enabled in that case as well.
{provide: HttpBackend, useClass: HttpXhrBackend},
{provide: HttpBackend, useClass: CustomBackendExtends},
],
});

const handler = TestBed.inject(HttpHandler);
expect((handler as any).backend).toBeInstanceOf(FetchBackend);
const backend = TestBed.inject(HttpBackend);
expect(backend).toBeInstanceOf(CustomBackendExtends);
});

it(`fetch API should be used in child when 'withFetch' was used in parent injector`, () => {
TestBed.configureTestingModule({
providers: [provideHttpClient(withFetch()), provideHttpClientTesting()],
});

const child = createEnvironmentInjector(
[provideHttpClient()],
TestBed.inject(EnvironmentInjector),
);

const backend = child.get(HttpBackend);
expect(backend).toBeInstanceOf(FetchBackend);
});

it('should not warn if fetch is not configured when running in a browser', () => {
Expand Down
Expand Up @@ -7,7 +7,7 @@
*/

import {XhrFactory} from '@angular/common';
import {HttpBackend, ɵPRIMARY_HTTP_BACKEND as PRIMARY_HTTP_BACKEND} from '@angular/common/http';
import {HttpBackend} from '@angular/common/http';
import {ModuleWithProviders, NgModule, Type} from '@angular/core';

import {HttpClientBackendService} from './http-client-backend-service';
Expand Down Expand Up @@ -57,7 +57,6 @@ export class HttpClientInMemoryWebApiModule {
useFactory: httpClientInMemBackendServiceFactory,
deps: [InMemoryDbService, InMemoryBackendConfig, XhrFactory],
},
{provide: PRIMARY_HTTP_BACKEND, useExisting: HttpBackend},
],
};
}
Expand Down
Expand Up @@ -7,7 +7,7 @@
*/

import {XhrFactory} from '@angular/common';
import {HttpBackend, ɵPRIMARY_HTTP_BACKEND as PRIMARY_HTTP_BACKEND} from '@angular/common/http';
import {HttpBackend} from '@angular/common/http';
import {ModuleWithProviders, NgModule, Type} from '@angular/core';

import {httpClientInMemBackendServiceFactory} from './http-client-in-memory-web-api-module';
Expand Down Expand Up @@ -47,7 +47,6 @@ export class InMemoryWebApiModule {
useFactory: httpClientInMemBackendServiceFactory,
deps: [InMemoryDbService, InMemoryBackendConfig, XhrFactory],
},
{provide: PRIMARY_HTTP_BACKEND, useExisting: HttpBackend},
],
};
}
Expand Down

0 comments on commit d9b339f

Please sign in to comment.