Pass client to IHttpStatic’s constructor

Preparation for tree-shakable HTTP request implementations, which will be exposed via the client. The constructor needs access to the list of available request implementations so that it can populate the supportsAuthHeaders property.
ably · Nov 8, 2023 · 0752f1b · 0752f1b
1 parent f19d52f
commit 0752f1b
Show file tree

Hide file tree

Showing 12 changed files with 60 additions and 60 deletions.
diff --git a/src/common/lib/client/auth.ts b/src/common/lib/client/auth.ts
@@ -538,7 +538,6 @@ class Auth {
           const body = Utils.toQueryString(authParams).slice(1); /* slice is to remove the initial '?' */
           this.client.http.doUri(
             HttpMethods.Post,
-            client,
             authOptions.authUrl,
             headers,
             body,
@@ -548,7 +547,6 @@ class Auth {
         } else {
           this.client.http.doUri(
             HttpMethods.Get,
-            client,
             authOptions.authUrl,
             authHeaders || {},
             null,
@@ -594,7 +592,6 @@ class Auth {
       );
       this.client.http.do(
         HttpMethods.Post,
-        client,
         tokenUri,
         requestHeaders,
         JSON.stringify(signedTokenParams),

diff --git a/src/common/lib/client/baseclient.ts b/src/common/lib/client/baseclient.ts
@@ -88,7 +88,7 @@ class BaseClient {
     this._currentFallback = null;
 
     this.serverTimeOffset = null;
-    this.http = new Platform.Http(normalOptions);
+    this.http = new Platform.Http(this);
     this.auth = new Auth(this, normalOptions);
 
     this._rest = modules.Rest ? new modules.Rest(this) : null;

diff --git a/src/common/lib/client/resource.ts b/src/common/lib/client/resource.ts
@@ -245,7 +245,7 @@ class Resource {
         );
       }
 
-      client.http.do(method, client, path, headers, body, params, function (err, res, headers, unpacked, statusCode) {
+      client.http.do(method, path, headers, body, params, function (err, res, headers, unpacked, statusCode) {
         if (err && Auth.isTokenErr(err as ErrorInfo)) {
           /* token has expired, so get a new one */
           client.auth.authorize(null, null, function (err: ErrorInfo) {

diff --git a/src/common/lib/client/rest.ts b/src/common/lib/client/rest.ts
@@ -83,7 +83,6 @@ export class Rest {
     };
     this.client.http.do(
       HttpMethods.Get,
-      this.client,
       timeUri,
       headers,
       null,

diff --git a/src/common/types/http.ts b/src/common/types/http.ts
@@ -2,7 +2,6 @@ import HttpMethods from '../constants/HttpMethods';
 import BaseClient from '../lib/client/baseclient';
 import ErrorInfo, { IPartialErrorInfo } from '../lib/types/errorinfo';
 import { Agents } from 'got';
-import { NormalisedClientOptions } from './ClientOptions';
 
 export type PathParameter = string | ((host: string) => string);
 export type RequestCallbackHeaders = Partial<Record<string, string | string[]>>;
@@ -16,7 +15,7 @@ export type RequestCallback = (
 export type RequestParams = Record<string, string> | null;
 
 export interface IHttpStatic {
-  new (options?: NormalisedClientOptions): IHttp;
+  new (client?: BaseClient): IHttp;
   methods: Array<HttpMethods>;
   methodsWithBody: Array<HttpMethods>;
   methodsWithoutBody: Array<HttpMethods>;
@@ -29,7 +28,6 @@ export interface IHttp {
 
   Request?: (
     method: HttpMethods,
-    client: BaseClient | null,
     uri: string,
     headers: Record<string, string> | null,
     params: RequestParams,
@@ -39,7 +37,6 @@ export interface IHttp {
   _getHosts: (client: BaseClient) => string[];
   do(
     method: HttpMethods,
-    client: BaseClient | null,
     path: PathParameter,
     headers: Record<string, string> | null,
     body: unknown,
@@ -48,7 +45,6 @@ export interface IHttp {
   ): void;
   doUri(
     method: HttpMethods,
-    client: BaseClient | null,
     uri: string,
     headers: Record<string, string> | null,
     body: unknown,

diff --git a/src/platform/nodejs/lib/util/http.ts b/src/platform/nodejs/lib/util/http.ts
@@ -14,7 +14,7 @@ import http from 'http';
 import https from 'https';
 import BaseClient from 'common/lib/client/baseclient';
 import BaseRealtime from 'common/lib/client/baserealtime';
-import { NormalisedClientOptions, RestAgentOptions } from 'common/types/ClientOptions';
+import { RestAgentOptions } from 'common/types/ClientOptions';
 import { isSuccessCode } from 'common/constants/HttpStatusCodes';
 import { shallowEquals, throwMissingModuleError } from 'common/lib/util/utils';
 
@@ -105,22 +105,26 @@ const Http: IHttpStatic = class {
   _getHosts = getHosts;
   supportsAuthHeaders = true;
   supportsLinkHeaders = true;
-  private options: NormalisedClientOptions | null;
+  private client: BaseClient | null;
 
-  constructor(options?: NormalisedClientOptions) {
-    this.options = options ?? null;
+  constructor(client?: BaseClient) {
+    this.client = client ?? null;
   }
 
-  /* Unlike for doUri, the 'client' param here is mandatory, as it's used to generate the hosts */
   do(
     method: HttpMethods,
-    client: BaseClient,
     path: PathParameter,
     headers: Record<string, string> | null,
     body: unknown,
     params: RequestParams,
     callback: RequestCallback
   ): void {
+    /* Unlike for doUri, the presence of `this.client` here is mandatory, as it's used to generate the hosts */
+    const client = this.client;
+    if (!client) {
+      throw new Error('http.do called without client');
+    }
+
     const uriFromHost =
       typeof path === 'function'
         ? path
@@ -132,11 +136,11 @@ const Http: IHttpStatic = class {
     if (currentFallback) {
       if (currentFallback.validUntil > Date.now()) {
         /* Use stored fallback */
-        this.doUri(method, client, uriFromHost(currentFallback.host), headers, body, params, (err, ...args) => {
+        this.doUri(method, uriFromHost(currentFallback.host), headers, body, params, (err, ...args) => {
           if (err && shouldFallback(err as ErrnoException)) {
             /* unstore the fallback and start from the top with the default sequence */
             client._currentFallback = null;
-            this.do(method, client, path, headers, body, params, callback);
+            this.do(method, path, headers, body, params, callback);
             return;
           }
           callback(err, ...args);
@@ -152,13 +156,13 @@ const Http: IHttpStatic = class {
 
     /* see if we have one or more than one host */
     if (hosts.length === 1) {
-      this.doUri(method, client, uriFromHost(hosts[0]), headers, body, params, callback);
+      this.doUri(method, uriFromHost(hosts[0]), headers, body, params, callback);
       return;
     }
 
     const tryAHost = (candidateHosts: Array<string>, persistOnSuccess?: boolean) => {
       const host = candidateHosts.shift();
-      this.doUri(method, client, uriFromHost(host as string), headers, body, params, function (err, ...args) {
+      this.doUri(method, uriFromHost(host as string), headers, body, params, function (err, ...args) {
         if (err && shouldFallback(err as ErrnoException) && candidateHosts.length) {
           tryAHost(candidateHosts, true);
           return;
@@ -178,7 +182,6 @@ const Http: IHttpStatic = class {
 
   doUri(
     method: HttpMethods,
-    client: BaseClient,
     uri: string,
     headers: Record<string, string> | null,
     body: unknown,
@@ -188,7 +191,8 @@ const Http: IHttpStatic = class {
     /* Will generally be making requests to one or two servers exclusively
      * (Ably and perhaps an auth server), so for efficiency, use the
      * foreverAgent to keep the TCP stream alive between requests where possible */
-    const agentOptions = (client && client.options.restAgentOptions) || (Defaults.restAgentOptions as RestAgentOptions);
+    const agentOptions =
+      (this.client && this.client.options.restAgentOptions) || (Defaults.restAgentOptions as RestAgentOptions);
     const doOptions: Options = { headers: headers || undefined, responseType: 'buffer' };
 
     if (!this.agent) {
@@ -215,37 +219,38 @@ const Http: IHttpStatic = class {
     doOptions.agent = this.agent;
 
     doOptions.url = uri;
-    doOptions.timeout = { request: ((client && client.options.timeouts) || Defaults.TIMEOUTS).httpRequestTimeout };
+    doOptions.timeout = {
+      request: ((this.client && this.client.options.timeouts) || Defaults.TIMEOUTS).httpRequestTimeout,
+    };
     // We have our own logic that retries appropriate statuscodes to fallback endpoints,
     // with timeouts constructed appropriately. Don't want `got` doing its own retries to
     // the same endpoint, inappropriately retrying 429s, etc
     doOptions.retry = { limit: 0 };
 
     (got[method](doOptions) as CancelableRequest<Response>)
       .then((res: Response) => {
-        handler(uri, params, client, callback)(null, res, res.body);
+        handler(uri, params, this.client, callback)(null, res, res.body);
       })
       .catch((err: ErrnoException) => {
         if (err instanceof got.HTTPError) {
-          handler(uri, params, client, callback)(null, err.response, err.response.body);
+          handler(uri, params, this.client, callback)(null, err.response, err.response.body);
           return;
         }
-        handler(uri, params, client, callback)(err);
+        handler(uri, params, this.client, callback)(err);
       });
   }
 
   checkConnectivity = (callback: (errorInfo: ErrorInfo | null, connected?: boolean) => void): void => {
-    if (this.options?.disableConnectivityCheck) {
+    if (this.client?.options.disableConnectivityCheck) {
       callback(null, true);
       return;
     }
-    const connectivityCheckUrl = this.options?.connectivityCheckUrl || Defaults.connectivityCheckUrl;
-    const connectivityCheckParams = this.options?.connectivityCheckParams ?? null;
-    const connectivityUrlIsDefault = !this.options?.connectivityCheckUrl;
+    const connectivityCheckUrl = this.client?.options.connectivityCheckUrl || Defaults.connectivityCheckUrl;
+    const connectivityCheckParams = this.client?.options.connectivityCheckParams ?? null;
+    const connectivityUrlIsDefault = !this.client?.options.connectivityCheckUrl;
 
     this.doUri(
       HttpMethods.Get,
-      null as any,
       connectivityCheckUrl,
       null,
       null,
@@ -262,7 +267,6 @@ const Http: IHttpStatic = class {
 
   Request?: (
     method: HttpMethods,
-    client: BaseClient | null,
     uri: string,
     headers: Record<string, string> | null,
     params: RequestParams,

diff --git a/src/platform/web/lib/http/http.ts b/src/platform/web/lib/http/http.ts
@@ -11,7 +11,6 @@ import XHRStates from 'common/constants/XHRStates';
 import Logger from 'common/lib/util/logger';
 import { StandardCallback } from 'common/types/utils';
 import fetchRequest from './request/fetchrequest';
-import { NormalisedClientOptions } from 'common/types/ClientOptions';
 import { isSuccessCode } from 'common/constants/HttpStatusCodes';
 
 function shouldFallback(errorInfo: ErrorInfo) {
@@ -45,16 +44,17 @@ const Http: IHttpStatic = class {
   static methodsWithoutBody = [HttpMethods.Get, HttpMethods.Delete];
   static methodsWithBody = [HttpMethods.Post, HttpMethods.Put, HttpMethods.Patch];
   checksInProgress: Array<StandardCallback<boolean>> | null = null;
+  private client: BaseClient | null;
 
-  constructor(options?: NormalisedClientOptions) {
-    const connectivityCheckUrl = options?.connectivityCheckUrl || Defaults.connectivityCheckUrl;
-    const connectivityCheckParams = options?.connectivityCheckParams ?? null;
-    const connectivityUrlIsDefault = !options?.connectivityCheckUrl;
+  constructor(client?: BaseClient) {
+    this.client = client ?? null;
+    const connectivityCheckUrl = client?.options.connectivityCheckUrl || Defaults.connectivityCheckUrl;
+    const connectivityCheckParams = client?.options.connectivityCheckParams ?? null;
+    const connectivityUrlIsDefault = !client?.options.connectivityCheckUrl;
     if (Platform.Config.xhrSupported) {
       this.supportsAuthHeaders = true;
       this.Request = function (
         method: HttpMethods,
-        client: BaseClient | null,
         uri: string,
         headers: Record<string, string> | null,
         params: RequestParams,
@@ -67,14 +67,14 @@ const Http: IHttpStatic = class {
           params,
           body,
           XHRStates.REQ_SEND,
-          client && client.options.timeouts,
+          (client && client.options.timeouts) ?? null,
           method
         );
         req.once('complete', callback);
         req.exec();
         return req;
       };
-      if (options?.disableConnectivityCheck) {
+      if (client?.options.disableConnectivityCheck) {
         this.checkConnectivity = function (callback: (err: null, connectivity: true) => void) {
           callback(null, true);
         };
@@ -87,7 +87,6 @@ const Http: IHttpStatic = class {
           );
           this.doUri(
             HttpMethods.Get,
-            null as any,
             connectivityCheckUrl,
             null,
             null,
@@ -107,17 +106,19 @@ const Http: IHttpStatic = class {
       }
     } else if (Platform.Config.fetchSupported) {
       this.supportsAuthHeaders = true;
-      this.Request = fetchRequest;
+      this.Request = (method, uri, headers, params, body, callback) => {
+        fetchRequest(method, client ?? null, uri, headers, params, body, callback);
+      };
       this.checkConnectivity = function (callback: (err: ErrorInfo | null, connectivity: boolean) => void) {
         Logger.logAction(Logger.LOG_MICRO, '(Fetch)Http.checkConnectivity()', 'Sending; ' + connectivityCheckUrl);
-        this.doUri(HttpMethods.Get, null as any, connectivityCheckUrl, null, null, null, function (err, responseText) {
+        this.doUri(HttpMethods.Get, connectivityCheckUrl, null, null, null, function (err, responseText) {
           const result = !err && (responseText as string)?.replace(/\n/, '') == 'yes';
           Logger.logAction(Logger.LOG_MICRO, '(Fetch)Http.checkConnectivity()', 'Result: ' + result);
           callback(null, result);
         });
       };
     } else {
-      this.Request = (method, client, uri, headers, params, body, callback) => {
+      this.Request = (method, uri, headers, params, body, callback) => {
         callback(new PartialErrorInfo('no supported HTTP transports available', null, 400), null);
       };
     }
@@ -126,13 +127,18 @@ const Http: IHttpStatic = class {
   /* Unlike for doUri, the 'client' param here is mandatory, as it's used to generate the hosts */
   do(
     method: HttpMethods,
-    client: BaseClient,
     path: string,
     headers: Record<string, string> | null,
     body: unknown,
     params: RequestParams,
     callback?: RequestCallback
   ): void {
+    /* Unlike for doUri, the presence of `this.client` here is mandatory, as it's used to generate the hosts */
+    const client = this.client;
+    if (!client) {
+      throw new Error('http.do called without client');
+    }
+
     const uriFromHost =
       typeof path == 'function'
         ? path
@@ -148,12 +154,12 @@ const Http: IHttpStatic = class {
           callback?.(new PartialErrorInfo('Request invoked before assigned to', null, 500));
           return;
         }
-        this.Request(method, client, uriFromHost(currentFallback.host), headers, params, body, (err?, ...args) => {
+        this.Request(method, uriFromHost(currentFallback.host), headers, params, body, (err?, ...args) => {
           // This typecast is safe because ErrnoExceptions are only thrown in NodeJS
           if (err && shouldFallback(err as ErrorInfo)) {
             /* unstore the fallback and start from the top with the default sequence */
             client._currentFallback = null;
-            this.do(method, client, path, headers, body, params, callback);
+            this.do(method, path, headers, body, params, callback);
             return;
           }
           callback?.(err, ...args);
@@ -169,14 +175,14 @@ const Http: IHttpStatic = class {
 
     /* if there is only one host do it */
     if (hosts.length === 1) {
-      this.doUri(method, client, uriFromHost(hosts[0]), headers, body, params, callback as RequestCallback);
+      this.doUri(method, uriFromHost(hosts[0]), headers, body, params, callback as RequestCallback);
       return;
     }
 
     /* hosts is an array with preferred host plus at least one fallback */
     const tryAHost = (candidateHosts: Array<string>, persistOnSuccess?: boolean) => {
       const host = candidateHosts.shift();
-      this.doUri(method, client, uriFromHost(host as string), headers, body, params, function (err, ...args) {
+      this.doUri(method, uriFromHost(host as string), headers, body, params, function (err, ...args) {
         // This typecast is safe because ErrnoExceptions are only thrown in NodeJS
         if (err && shouldFallback(err as ErrorInfo) && candidateHosts.length) {
           tryAHost(candidateHosts, true);
@@ -197,7 +203,6 @@ const Http: IHttpStatic = class {
 
   doUri(
     method: HttpMethods,
-    client: BaseClient | null,
     uri: string,
     headers: Record<string, string> | null,
     body: unknown,
@@ -208,12 +213,11 @@ const Http: IHttpStatic = class {
       callback(new PartialErrorInfo('Request invoked before assigned to', null, 500));
       return;
     }
-    this.Request(method, client, uri, headers, params, body, callback);
+    this.Request(method, uri, headers, params, body, callback);
   }
 
   Request?: (
     method: HttpMethods,
-    client: BaseClient | null,
     uri: string,
     headers: Record<string, string> | null,
     params: RequestParams,

diff --git a/test/browser/modules.test.js b/test/browser/modules.test.js
@@ -286,7 +286,7 @@ describe('browser/modules', function () {
       const channelName = 'channel';
       const channel = rest.channels.get(channelName);
       const contentTypeUsedForPublishPromise = new Promise((resolve, reject) => {
-        rest.http.do = (method, client, path, headers, body, params, callback) => {
+        rest.http.do = (method, path, headers, body, params, callback) => {
           if (!(method == 'post' && path == `/channels/${channelName}/messages`)) {
             return;
           }