Use common types to fix incompatible types between browser and Node

sdk/core/core-http/review/core-http.api.md

@@ -168,9 +168,11 @@ export function createPipelineFromOptions(pipelineOptions: InternalPipelineOptio
 // @public (undocumented)
 export class DefaultHttpClient extends FetchHttpClient {
     // Warning: (ae-forgotten-export) The symbol "CommonRequestInfo" needs to be exported by the entry point coreHttp.d.ts
+    // Warning: (ae-forgotten-export) The symbol "CommonRequestInit" needs to be exported by the entry point coreHttp.d.ts
+    // Warning: (ae-forgotten-export) The symbol "CommonResponse" needs to be exported by the entry point coreHttp.d.ts
     //
     // (undocumented)
-    fetch(input: CommonRequestInfo, init?: RequestInit): Promise<Response>;
+    fetch(input: CommonRequestInfo, init?: CommonRequestInit): Promise<CommonResponse>;
     // (undocumented)
     prepareRequest(httpRequest: WebResourceLike): Promise<Partial<RequestInit>>;
     // (undocumented)

sdk/core/core-http/src/browserFetchHttpClient.ts

@@ -1,7 +1,12 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import { FetchHttpClient, CommonRequestInfo } from "./fetchHttpClient";
+import {
+  FetchHttpClient,
+  CommonRequestInfo,
+  CommonResponse,
+  CommonRequestInit
+} from "./fetchHttpClient";
 import { HttpOperationResponse } from "./httpOperationResponse";
 import { WebResourceLike } from "./webResource";
 
@@ -15,7 +20,7 @@ export class BrowserFetchHttpClient extends FetchHttpClient {
   }
 
   // eslint-disable-next-line @azure/azure-sdk/ts-apisurface-standardized-verbs
-  fetch(input: CommonRequestInfo, init?: RequestInit): Promise<Response> {
+  fetch(input: CommonRequestInfo, init?: CommonRequestInit): Promise<CommonResponse> {
     return fetch(input, init);
   }
 }

sdk/core/core-http/src/fetchHttpClient.ts

@@ -17,7 +17,19 @@ interface FetchError extends Error {
   type?: string;
 }
 
-export type CommonRequestInfo = Request | string;
+export type CommonRequestInfo = string; // We only ever call fetch() on string urls.
+
+export type CommonRequestInit = Omit<RequestInit, "body" | "headers" | "signal"> & {
+  body?: any;
+  headers?: any;
+  signal?: any;
+};
+
+export type CommonResponse = Omit<Response, "body" | "trailer" | "formData"> & {
+  body: any;
+  trailer: any;
+  formData: any;
+};
 
 export class ReportTransform extends Transform {
   private loadedBytes: number = 0;
@@ -134,7 +146,7 @@ export abstract class FetchHttpClient implements HttpClient {
     };
 
     try {
-      const response: Response = await this.fetch(httpRequest.url, requestInit);
+      const response: CommonResponse = await this.fetch(httpRequest.url, requestInit);
 
       const headers = parseHeaders(response.headers);
       const operationResponse: HttpOperationResponse = {
@@ -191,7 +203,7 @@ export abstract class FetchHttpClient implements HttpClient {
 
   abstract async prepareRequest(httpRequest: WebResourceLike): Promise<Partial<RequestInit>>;
   abstract async processRequest(operationResponse: HttpOperationResponse): Promise<void>;
-  abstract async fetch(input: CommonRequestInfo, init?: RequestInit): Promise<Response>;
+  abstract async fetch(input: CommonRequestInfo, init?: CommonRequestInit): Promise<CommonResponse>;
 }
 
 function isReadableStream(body: any): body is Readable {

sdk/core/core-http/src/nodeFetchHttpClient.ts

@@ -6,20 +6,16 @@ import * as http from "http";
 import * as https from "https";
 import node_fetch from "node-fetch";
 
-import { FetchHttpClient, CommonRequestInfo } from "./fetchHttpClient";
+import {
+  FetchHttpClient,
+  CommonRequestInfo,
+  CommonRequestInit,
+  CommonResponse
+} from "./fetchHttpClient";
 import { HttpOperationResponse } from "./httpOperationResponse";
 import { WebResourceLike } from "./webResource";
 import { createProxyAgent, ProxyAgent, isUrlHttps } from "./proxyAgent";
 
-interface GlobalWithFetch extends NodeJS.Global {
-  fetch: typeof node_fetch;
-}
-
-const globalWithFetch = global as GlobalWithFetch;
-if (typeof globalWithFetch.fetch !== "function") {
-  globalWithFetch.fetch = node_fetch;
-}
-
 interface AgentCache {
   httpAgent?: http.Agent;
   httpsAgent?: https.Agent;
@@ -38,6 +34,20 @@ export class NodeFetchHttpClient extends FetchHttpClient {
 
   private readonly cookieJar = new tough.CookieJar(undefined, { looseMode: true });
 
+  /**
+   * As we no longer use the global fetch on NodeJS, we need a way to set the local fetch to a mock for testing.
+   */
+  private _fetch: typeof node_fetch | undefined;
+
+  constructor(_fetch?: typeof node_fetch) {
+    super();
+    this._fetch = _fetch;
+  }
+
+  private getFetch(): typeof node_fetch {
+    return this._fetch || node_fetch;
+  }
+
   private getOrCreateAgent(httpRequest: WebResourceLike): http.Agent | https.Agent {
     const isHttps = isUrlHttps(httpRequest.url);
 
@@ -87,8 +97,9 @@ export class NodeFetchHttpClient extends FetchHttpClient {
   }
 
   // eslint-disable-next-line @azure/azure-sdk/ts-apisurface-standardized-verbs
-  async fetch(input: CommonRequestInfo, init?: RequestInit): Promise<Response> {
-    return fetch(input, init);
+  async fetch(input: CommonRequestInfo, init?: CommonRequestInit): Promise<CommonResponse> {
+    const response = await this.getFetch()(input, init);
+    return (response as unknown) as CommonResponse;
   }
 
   async prepareRequest(httpRequest: WebResourceLike): Promise<Partial<RequestInit>> {

sdk/core/core-http/test/defaultHttpClientTests.node.ts

@@ -39,7 +39,7 @@ describe("defaultHttpClient (node)", function() {
       };
     });
 
-    const client = new DefaultHttpClient();
+    const client = new DefaultHttpClient(httpMock.getFetch());
 
     const request1 = new WebResource("http://my.fake.domain/set-cookie");
     const response1 = await client.sendRequest(request1);
@@ -126,7 +126,8 @@ describe("defaultHttpClient (node)", function() {
         (ev) => listener(download, ev)
       );
 
-      const client = new DefaultHttpClient();
+      const client = new DefaultHttpClient(httpMock.getFetch());
+
       const response = await client.sendRequest(request);
       response.status.should.equal(250);
       if (response.blobBody) {

sdk/core/core-http/test/defaultHttpClientTests.ts

@@ -33,7 +33,7 @@ describe("defaultHttpClient", function() {
     });
 
     const request = new WebResource(resourceUrl, "GET");
-    const httpClient = new DefaultHttpClient();
+    const httpClient = new DefaultHttpClient(httpMock.getFetch());
 
     const response = await httpClient.sendRequest(request);
     response.status.should.equal(404);
@@ -58,7 +58,8 @@ describe("defaultHttpClient", function() {
       undefined,
       controller.signal
     );
-    const client = new DefaultHttpClient();
+    const client = new DefaultHttpClient(httpMock.getFetch());
+
     const promise = client.sendRequest(request);
     controller.abort();
     try {
@@ -90,7 +91,8 @@ describe("defaultHttpClient", function() {
       controller.signal
     );
     controller.abort();
-    const client = new DefaultHttpClient();
+    const client = new DefaultHttpClient(httpMock.getFetch());
+
     const promise = client.sendRequest(request);
     try {
       await promise;
@@ -132,7 +134,8 @@ describe("defaultHttpClient", function() {
         controller.signal
       )
     ];
-    const client = new DefaultHttpClient();
+    const client = new DefaultHttpClient(httpMock.getFetch());
+
     const promises = requests.map((r) => client.sendRequest(r));
     controller.abort();
     // Ensure each promise is individually rejected
@@ -184,7 +187,8 @@ describe("defaultHttpClient", function() {
         (ev) => listener(download, ev)
       );
 
-      const client = new DefaultHttpClient();
+      const client = new DefaultHttpClient(httpMock.getFetch());
+
       const response = await client.sendRequest(request);
       response.should.exist;
       response.status.should.equal(251);
@@ -207,7 +211,8 @@ describe("defaultHttpClient", function() {
       undefined,
       100
     );
-    const client = new DefaultHttpClient();
+    const client = new DefaultHttpClient(httpMock.getFetch());
+
     try {
       await client.sendRequest(request);
       throw new Error("request did not fail as expected");
@@ -221,8 +226,9 @@ describe("defaultHttpClient", function() {
     // eslint-disable-next-line no-invalid-this
     this.timeout(10000);
     const requestUrl = "http://fake.domain";
-    httpMock.passThrough();
     const request = new WebResource(requestUrl, "GET");
+    httpMock.passThrough();
+    // restoring unstubbed fetch behavior, so not passing the local mock
     const client = new DefaultHttpClient();
     try {
       await client.sendRequest(request);
@@ -249,7 +255,8 @@ describe("defaultHttpClient", function() {
     });
 
     const request = new WebResource(requestUrl, "PUT");
-    const client = new DefaultHttpClient();
+    const client = new DefaultHttpClient(httpMock.getFetch());
+
     const response = await client.sendRequest(request);
     response.status.should.equal(200, response.bodyAsText!);
   });

sdk/core/core-http/test/mockHttp.ts

@@ -5,6 +5,7 @@ import xhrMock, { proxy } from "xhr-mock";
 import { isNode, HttpMethods } from "../src/coreHttp";
 import fetchMock, * as fetch from "fetch-mock";
 import { Readable } from "stream";
+import node_fetch from "node-fetch";
 
 export type UrlFilter = string | RegExp;
 
@@ -32,31 +33,44 @@ export interface HttpMockFacade {
   get(url: UrlFilter, response: MockResponse): void;
   post(url: UrlFilter, response: MockResponse): void;
   put(url: UrlFilter, response: MockResponse): void;
+  getFetch(): typeof node_fetch | undefined;
 }
 
 export function getHttpMock(): HttpMockFacade {
   return isNode ? new FetchHttpMock() : new BrowserHttpMock();
 }
 
 class FetchHttpMock implements HttpMockFacade {
+  private _fetch: fetchMock.FetchMockSandbox;
+
+  constructor() {
+    this._fetch = fetchMock.sandbox();
+  }
+
+  // returns the locally mocked fetch instance
+  getFetch(): typeof node_fetch {
+    /// @ts-ignore
+    return this._fetch as typeof node_fetch;
+  }
+
   setup(): void {
-    fetchMock.resetHistory();
+    this._fetch.resetHistory();
   }
 
   teardown(): void {
-    fetchMock.resetHistory();
+    this._fetch.resetHistory();
   }
 
   passThrough(_url?: string | RegExp | undefined): void {
-    fetchMock.reset();
+    this._fetch.reset();
   }
 
   timeout(_method: HttpMethods, url: UrlFilter): void {
     const delay = new Promise((resolve) => {
       setTimeout(() => resolve({ $uri: url, delay: 500 }), 2500);
     });
 
-    fetchMock.mock(url, delay);
+    this._fetch.mock(url, delay);
   }
 
   convertStreamToBuffer(stream: Readable): Promise<any> {
@@ -89,7 +103,7 @@ class FetchHttpMock implements HttpMockFacade {
 
     const matcher = (_url: string, opts: fetch.MockRequest): boolean =>
       url === _url && opts.method === method;
-    fetchMock.mock(matcher, mockResponse);
+    this._fetch.mock(matcher, mockResponse);
   }
 
   get(url: UrlFilter, response: MockResponse): void {
@@ -162,4 +176,8 @@ export class BrowserHttpMock implements HttpMockFacade {
       throw new Error("Timeout");
     });
   }
+
+  getFetch(): undefined {
+    return undefined;
+  }
 }