From 2f95ed4a5d070e61f0d05c17bf7a6440d30832a8 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 26 Oct 2023 15:32:20 -0300 Subject: [PATCH 1/8] Remove redundant type annotations in PaginatedResource bodyHandler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (I’m also not sure why the compiler was accepting the code that described the `body` parameter as having type `any`, given that in BodyHandler it’s of type `unknown`.) --- src/common/lib/client/channel.ts | 8 ++++---- src/common/lib/client/presence.ts | 16 ++++++---------- src/common/lib/client/push.ts | 30 +++++++++++++++++------------- src/common/lib/client/rest.ts | 8 ++------ 4 files changed, 29 insertions(+), 33 deletions(-) diff --git a/src/common/lib/client/channel.ts b/src/common/lib/client/channel.ts index ac7efe4d94..2922de7adb 100644 --- a/src/common/lib/client/channel.ts +++ b/src/common/lib/client/channel.ts @@ -98,11 +98,11 @@ class Channel extends EventEmitter { const options = this.channelOptions; new PaginatedResource(client, this.basePath + '/messages', headers, envelope, async function ( - body: any, - headers: Record, - unpacked?: boolean + body, + headers, + unpacked ) { - return await Message.fromResponseBody(body, options, client._MsgPack, unpacked ? undefined : format); + return await Message.fromResponseBody(body as Message[], options, client._MsgPack, unpacked ? undefined : format); }).get(params as Record, callback); } diff --git a/src/common/lib/client/presence.ts b/src/common/lib/client/presence.ts index ac756ef9b8..8acbed68bf 100644 --- a/src/common/lib/client/presence.ts +++ b/src/common/lib/client/presence.ts @@ -38,13 +38,9 @@ class Presence extends EventEmitter { Utils.mixin(headers, client.options.headers); const options = this.channel.channelOptions; - new PaginatedResource(client, this.basePath, headers, envelope, async function ( - body: any, - headers: Record, - unpacked?: boolean - ) { + new PaginatedResource(client, this.basePath, headers, envelope, async function (body, headers, unpacked) { return await PresenceMessage.fromResponseBody( - body, + body as Record[], options as CipherOptions, client._MsgPack, unpacked ? undefined : format @@ -83,12 +79,12 @@ class Presence extends EventEmitter { const options = this.channel.channelOptions; new PaginatedResource(client, this.basePath + '/history', headers, envelope, async function ( - body: any, - headers: Record, - unpacked?: boolean + body, + headers, + unpacked ) { return await PresenceMessage.fromResponseBody( - body, + body as Record[], options as CipherOptions, client._MsgPack, unpacked ? undefined : format diff --git a/src/common/lib/client/push.ts b/src/common/lib/client/push.ts index 33054fbd0c..3d0b7b9f44 100644 --- a/src/common/lib/client/push.ts +++ b/src/common/lib/client/push.ts @@ -151,11 +151,15 @@ class DeviceRegistrations { Utils.mixin(headers, client.options.headers); new PaginatedResource(client, '/push/deviceRegistrations', headers, envelope, async function ( - body: any, - headers: Record, - unpacked?: boolean + body, + headers, + unpacked ) { - return DeviceDetails.fromResponseBody(body, client._MsgPack, unpacked ? undefined : format); + return DeviceDetails.fromResponseBody( + body as Record[], + client._MsgPack, + unpacked ? undefined : format + ); }).get(params, callback); } @@ -269,11 +273,15 @@ class ChannelSubscriptions { Utils.mixin(headers, client.options.headers); new PaginatedResource(client, '/push/channelSubscriptions', headers, envelope, async function ( - body: any, - headers: Record, - unpacked?: boolean + body, + headers, + unpacked ) { - return PushChannelSubscription.fromResponseBody(body, client._MsgPack, unpacked ? undefined : format); + return PushChannelSubscription.fromResponseBody( + body as Record[], + client._MsgPack, + unpacked ? undefined : format + ); }).get(params, callback); } @@ -310,11 +318,7 @@ class ChannelSubscriptions { if (client.options.pushFullWait) Utils.mixin(params, { fullWait: 'true' }); - new PaginatedResource(client, '/push/channels', headers, envelope, async function ( - body: unknown, - headers: Record, - unpacked?: boolean - ) { + new PaginatedResource(client, '/push/channels', headers, envelope, async function (body, headers, unpacked) { const parsedBody = ( !unpacked && format ? Utils.decodeBody(body, client._MsgPack, format) : body ) as Array; diff --git a/src/common/lib/client/rest.ts b/src/common/lib/client/rest.ts index f58ef1b493..2f398d0a50 100644 --- a/src/common/lib/client/rest.ts +++ b/src/common/lib/client/rest.ts @@ -56,11 +56,7 @@ export class Rest { Utils.mixin(headers, this.client.options.headers); - new PaginatedResource(this.client, '/stats', headers, envelope, function ( - body: unknown, - headers: Record, - unpacked?: boolean - ) { + new PaginatedResource(this.client, '/stats', headers, envelope, function (body, headers, unpacked) { const statsValues = unpacked ? body : JSON.parse(body as string); for (let i = 0; i < statsValues.length; i++) statsValues[i] = Stats.fromValues(statsValues[i]); return statsValues; @@ -160,7 +156,7 @@ export class Rest { path, headers, envelope, - async function (resbody: unknown, headers: Record, unpacked?: boolean) { + async function (resbody, headers, unpacked) { return Utils.ensureArray(unpacked ? resbody : decoder(resbody as string & Buffer)); }, /* useHttpPaginatedResponse: */ true From 98f630a4280a4da4aa725dec8581aa6bd7616883 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 26 Oct 2023 15:33:13 -0300 Subject: [PATCH 2/8] Convert http.d.ts to .ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes some compilation errors that were missed by the fact that it was a .d.ts file. (See #1445 for the issue that aims to convert all .d.ts files to .ts.) This improved type information introduced a compiler error resulting from us trying to directly access HTTP headers on DOM’s Headers object with header names as object property names. I can’t see how the existing code could have been working properly, so I’ve changed the response header handling in fetchrequest.ts. --- src/common/lib/client/paginatedresource.ts | 9 +- src/common/lib/client/resource.ts | 48 ++++------ src/common/lib/client/rest.ts | 9 +- src/common/platform.ts | 4 +- src/common/types/{http.d.ts => http.ts} | 23 +++-- src/platform/nodejs/lib/util/http.ts | 76 ++++++--------- .../web/lib/transport/fetchrequest.ts | 17 +++- src/platform/web/lib/util/http.ts | 92 +++++++------------ tsconfig.json | 2 +- 9 files changed, 116 insertions(+), 164 deletions(-) rename src/common/types/{http.d.ts => http.ts} (71%) diff --git a/src/common/lib/client/paginatedresource.ts b/src/common/lib/client/paginatedresource.ts index fd899c3e14..d224d17403 100644 --- a/src/common/lib/client/paginatedresource.ts +++ b/src/common/lib/client/paginatedresource.ts @@ -4,8 +4,9 @@ import Resource from './resource'; import ErrorInfo, { IPartialErrorInfo } from '../types/errorinfo'; import { PaginatedResultCallback } from '../../types/utils'; import BaseClient from './baseclient'; +import { RequestCallbackHeaders } from 'common/types/http'; -export type BodyHandler = (body: unknown, headers: Record, unpacked?: boolean) => Promise; +export type BodyHandler = (body: unknown, headers: RequestCallbackHeaders, unpacked?: boolean) => Promise; function getRelParams(linkUrl: string) { const urlMatch = linkUrl.match(/^\.\/(\w+)\?(.*)$/); @@ -135,7 +136,7 @@ class PaginatedResource { handlePage( err: IPartialErrorInfo | null, body: unknown, - headers: Record | undefined, + headers: RequestCallbackHeaders | undefined, unpacked: boolean | undefined, statusCode: number | undefined, callback: PaginatedResultCallback @@ -249,14 +250,14 @@ export class PaginatedResult { export class HttpPaginatedResponse extends PaginatedResult { statusCode: number; success: boolean; - headers: Record; + headers: RequestCallbackHeaders; errorCode?: number | null; errorMessage?: string | null; constructor( resource: PaginatedResource, items: T[], - headers: Record, + headers: RequestCallbackHeaders, statusCode: number, relParams: any, err: IPartialErrorInfo | null diff --git a/src/common/lib/client/resource.ts b/src/common/lib/client/resource.ts index 7d506d369e..0fa0b93c45 100644 --- a/src/common/lib/client/resource.ts +++ b/src/common/lib/client/resource.ts @@ -5,12 +5,12 @@ import Auth from './auth'; import HttpMethods from '../../constants/HttpMethods'; import ErrorInfo, { IPartialErrorInfo, PartialErrorInfo } from '../types/errorinfo'; import BaseClient from './baseclient'; -import { ErrnoException } from '../../types/http'; import { MsgPack } from 'common/types/msgpack'; +import { RequestCallbackHeaders } from 'common/types/http'; function withAuthDetails( client: BaseClient, - headers: Record, + headers: RequestCallbackHeaders | undefined, params: Record, errCallback: Function, opCallback: Function @@ -130,7 +130,7 @@ function logResponseHandler( export type ResourceCallback = ( err: IPartialErrorInfo | null, body?: T, - headers?: Record, + headers?: RequestCallbackHeaders, unpacked?: boolean, statusCode?: number ) => void; @@ -245,35 +245,21 @@ class Resource { ); } - client.http.do( - method, - client, - path, - headers, - body, - params, - function ( - err: ErrorInfo | ErrnoException | null | undefined, - res: any, - headers: Record, - unpacked?: boolean, - statusCode?: number - ) { - if (err && Auth.isTokenErr(err as ErrorInfo)) { - /* token has expired, so get a new one */ - client.auth.authorize(null, null, function (err: ErrorInfo) { - if (err) { - callback(err); - return; - } - /* retry ... */ - withAuthDetails(client, headers, params, callback, doRequest); - }); - return; - } - callback(err as ErrorInfo, res, headers, unpacked, statusCode); + client.http.do(method, client, 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) { + if (err) { + callback(err); + return; + } + /* retry ... */ + withAuthDetails(client, headers, params, callback, doRequest); + }); + return; } - ); + callback(err as ErrorInfo, res as T | undefined, headers, unpacked, statusCode); + }); } withAuthDetails(client, headers, params, callback, doRequest); diff --git a/src/common/lib/client/rest.ts b/src/common/lib/client/rest.ts index 2f398d0a50..6d1c08c4d0 100644 --- a/src/common/lib/client/rest.ts +++ b/src/common/lib/client/rest.ts @@ -9,7 +9,7 @@ import Stats from '../types/stats'; import HttpMethods from '../../constants/HttpMethods'; import { ChannelOptions } from '../../types/channel'; import { PaginatedResultCallback, StandardCallback } from '../../types/utils'; -import { ErrnoException, RequestParams } from '../../types/http'; +import { RequestParams } from '../../types/http'; import * as API from '../../../../ably'; import Resource from './resource'; @@ -88,12 +88,7 @@ export class Rest { headers, null, params as RequestParams, - ( - err?: ErrorInfo | ErrnoException | null, - res?: unknown, - headers?: Record, - unpacked?: boolean - ) => { + (err, res, headers, unpacked) => { if (err) { _callback(err); return; diff --git a/src/common/platform.ts b/src/common/platform.ts index 609b232687..6d5a6245bf 100644 --- a/src/common/platform.ts +++ b/src/common/platform.ts @@ -1,5 +1,5 @@ import { IPlatformConfig } from './types/IPlatformConfig'; -import { IHttp } from './types/http'; +import { IHttpStatic } from './types/http'; import { TransportInitialiser } from './lib/transport/connectionmanager'; import IDefaults from './types/IDefaults'; import IWebStorage from './types/IWebStorage'; @@ -31,7 +31,7 @@ export default class Platform { comment above. */ static Crypto: IUntypedCryptoStatic | null; - static Http: typeof IHttp; + static Http: IHttpStatic; static Transports: { order: TransportName[]; // Transport implementations that always come with this platform diff --git a/src/common/types/http.d.ts b/src/common/types/http.ts similarity index 71% rename from src/common/types/http.d.ts rename to src/common/types/http.ts index 0727978b52..404fb6c156 100644 --- a/src/common/types/http.d.ts +++ b/src/common/types/http.ts @@ -1,23 +1,28 @@ import HttpMethods from '../constants/HttpMethods'; -import { BaseClient } from '../lib/client/baseclient'; -import ErrorInfo from '../lib/types/errorinfo'; +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>; export type RequestCallback = ( error?: ErrnoException | IPartialErrorInfo | null, body?: unknown, - headers?: IncomingHttpHeaders, + headers?: RequestCallbackHeaders, unpacked?: boolean, statusCode?: number ) => void; export type RequestParams = Record | null; -export declare class IHttp { - constructor(options: NormalisedClientOptions); - static methods: Array; - static methodsWithBody: Array; - static methodsWithoutBody: Array; +export interface IHttpStatic { + new (options: NormalisedClientOptions): IHttp; + methods: Array; + methodsWithBody: Array; + methodsWithoutBody: Array; +} + +export interface IHttp { supportsAuthHeaders: boolean; supportsLinkHeaders: boolean; agent?: Agents | null; @@ -32,7 +37,7 @@ export declare class IHttp { body: unknown, callback: RequestCallback ) => void; - _getHosts: (client: BaseClient | Realtime) => string[]; + _getHosts: (client: BaseClient) => string[]; do( method: HttpMethods, client: BaseClient | null, diff --git a/src/platform/nodejs/lib/util/http.ts b/src/platform/nodejs/lib/util/http.ts index 7f53e13638..e24628fcc9 100644 --- a/src/platform/nodejs/lib/util/http.ts +++ b/src/platform/nodejs/lib/util/http.ts @@ -1,7 +1,13 @@ import Platform from 'common/platform'; import Defaults from 'common/lib/util/defaults'; import ErrorInfo from 'common/lib/types/errorinfo'; -import { ErrnoException, IHttp, PathParameter, RequestCallback, RequestParams } from '../../../../common/types/http'; +import { + ErrnoException, + IHttpStatic, + PathParameter, + RequestCallback, + RequestParams, +} from '../../../../common/types/http'; import HttpMethods from '../../../../common/constants/HttpMethods'; import got, { Response, Options, CancelableRequest, Agents } from 'got'; import http from 'http'; @@ -91,7 +97,7 @@ function getHosts(client: BaseClient): string[] { return Defaults.getHosts(client.options); } -const Http: typeof IHttp = class { +const Http: IHttpStatic = class { static methods = [HttpMethods.Get, HttpMethods.Delete, HttpMethods.Post, HttpMethods.Put, HttpMethods.Patch]; static methodsWithoutBody = [HttpMethods.Get, HttpMethods.Delete]; static methodsWithBody = [HttpMethods.Post, HttpMethods.Put, HttpMethods.Patch]; @@ -126,23 +132,15 @@ const Http: typeof IHttp = class { if (currentFallback) { if (currentFallback.validUntil > Date.now()) { /* Use stored fallback */ - this.doUri( - method, - client, - uriFromHost(currentFallback.host), - headers, - body, - params, - (err?: ErrnoException | ErrorInfo | null, ...args: unknown[]) => { - 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); - return; - } - callback(err, ...args); + this.doUri(method, client, 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); + return; } - ); + callback(err, ...args); + }); return; } else { /* Fallback expired; remove it and fallthrough to normal sequence */ @@ -160,28 +158,20 @@ const Http: typeof IHttp = class { const tryAHost = (candidateHosts: Array, persistOnSuccess?: boolean) => { const host = candidateHosts.shift(); - this.doUri( - method, - client, - uriFromHost(host as string), - headers, - body, - params, - function (err?: ErrnoException | ErrorInfo | null, ...args: unknown[]) { - if (err && shouldFallback(err as ErrnoException) && candidateHosts.length) { - tryAHost(candidateHosts, true); - return; - } - if (persistOnSuccess) { - /* RSC15f */ - client._currentFallback = { - host: host as string, - validUntil: Date.now() + client.options.timeouts.fallbackRetryTimeout, - }; - } - callback(err, ...args); + this.doUri(method, client, uriFromHost(host as string), headers, body, params, function (err, ...args) { + if (err && shouldFallback(err as ErrnoException) && candidateHosts.length) { + tryAHost(candidateHosts, true); + return; } - ); + if (persistOnSuccess) { + /* RSC15f */ + client._currentFallback = { + host: host as string, + validUntil: Date.now() + client.options.timeouts.fallbackRetryTimeout, + }; + } + callback(err, ...args); + }); }; tryAHost(hosts); } @@ -260,13 +250,7 @@ const Http: typeof IHttp = class { null, null, connectivityCheckParams, - function ( - err?: ErrnoException | ErrorInfo | null, - responseText?: unknown, - headers?: any, - unpacked?: boolean, - statusCode?: number - ) { + function (err, responseText, headers, unpacked, statusCode) { if (!err && !connectivityUrlIsDefault) { callback(null, isSuccessCode(statusCode as number)); return; diff --git a/src/platform/web/lib/transport/fetchrequest.ts b/src/platform/web/lib/transport/fetchrequest.ts index 7203ff62a6..68a9febf0f 100644 --- a/src/platform/web/lib/transport/fetchrequest.ts +++ b/src/platform/web/lib/transport/fetchrequest.ts @@ -1,7 +1,7 @@ import HttpMethods from 'common/constants/HttpMethods'; import BaseClient from 'common/lib/client/baseclient'; import ErrorInfo, { PartialErrorInfo } from 'common/lib/types/errorinfo'; -import { RequestCallback, RequestParams } from 'common/types/http'; +import { RequestCallback, RequestCallbackHeaders, RequestParams } from 'common/types/http'; import Platform from 'common/platform'; import Defaults from 'common/lib/util/defaults'; import * as Utils from 'common/lib/util/utils'; @@ -17,6 +17,16 @@ function getAblyError(responseBody: unknown, headers: Headers) { } } +function convertHeaders(headers: Headers) { + const result: RequestCallbackHeaders = {}; + + headers.forEach((value, key) => { + result[key] = value; + }); + + return result; +} + export default function fetchRequest( method: HttpMethods, client: BaseClient | null, @@ -64,6 +74,7 @@ export default function fetchRequest( } prom.then((body) => { const unpacked = !!contentType && contentType.indexOf('application/x-msgpack') === -1; + const headers = convertHeaders(res.headers); if (!res.ok) { const err = getAblyError(body, res.headers) || @@ -72,9 +83,9 @@ export default function fetchRequest( null, res.status ); - callback(err, body, res.headers, unpacked, res.status); + callback(err, body, headers, unpacked, res.status); } else { - callback(null, body, res.headers, unpacked, res.status); + callback(null, body, headers, unpacked, res.status); } }); }) diff --git a/src/platform/web/lib/util/http.ts b/src/platform/web/lib/util/http.ts index a417d5f83c..d6cb949357 100644 --- a/src/platform/web/lib/util/http.ts +++ b/src/platform/web/lib/util/http.ts @@ -2,7 +2,7 @@ import Platform from 'common/platform'; import * as Utils from 'common/lib/util/utils'; import Defaults from 'common/lib/util/defaults'; import ErrorInfo, { PartialErrorInfo } from 'common/lib/types/errorinfo'; -import { ErrnoException, IHttp, RequestCallback, RequestParams } from 'common/types/http'; +import { IHttpStatic, RequestCallback, RequestParams } from 'common/types/http'; import HttpMethods from 'common/constants/HttpMethods'; import BaseClient from 'common/lib/client/baseclient'; import BaseRealtime from 'common/lib/client/baserealtime'; @@ -40,7 +40,7 @@ function getHosts(client: BaseClient): string[] { return Defaults.getHosts(client.options); } -const Http: typeof IHttp = class { +const Http: IHttpStatic = class { static methods = [HttpMethods.Get, HttpMethods.Delete, HttpMethods.Post, HttpMethods.Put, HttpMethods.Patch]; static methodsWithoutBody = [HttpMethods.Get, HttpMethods.Delete]; static methodsWithBody = [HttpMethods.Post, HttpMethods.Put, HttpMethods.Patch]; @@ -95,13 +95,7 @@ const Http: typeof IHttp = class { null, null, connectivityCheckParams, - function ( - err?: ErrorInfo | ErrnoException | null, - responseText?: unknown, - headers?: any, - unpacked?: boolean, - statusCode?: number - ) { + function (err, responseText, headers, unpacked, statusCode) { let result = false; if (!connectivityUrlIsDefault) { result = !err && isSuccessCode(statusCode as number); @@ -119,19 +113,11 @@ const Http: typeof IHttp = class { this.Request = fetchRequest; 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?: ErrorInfo | ErrnoException | null, responseText?: unknown) { - const result = !err && (responseText as string)?.replace(/\n/, '') == 'yes'; - Logger.logAction(Logger.LOG_MICRO, '(Fetch)Http.checkConnectivity()', 'Result: ' + result); - callback(null, result); - } - ); + this.doUri(HttpMethods.Get, null as any, 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) => { @@ -165,24 +151,16 @@ const Http: typeof IHttp = class { callback?.(new PartialErrorInfo('Request invoked before assigned to', null, 500)); return; } - this.Request( - method, - client, - uriFromHost(currentFallback.host), - headers, - params, - body, - (err?: ErrnoException | ErrorInfo | null, ...args: unknown[]) => { - // 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); - return; - } - callback?.(err, ...args); + this.Request(method, client, 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); + return; } - ); + callback?.(err, ...args); + }); return; } else { /* Fallback expired; remove it and fallthrough to normal sequence */ @@ -201,29 +179,21 @@ const Http: typeof IHttp = class { /* hosts is an array with preferred host plus at least one fallback */ const tryAHost = (candidateHosts: Array, persistOnSuccess?: boolean) => { const host = candidateHosts.shift(); - this.doUri( - method, - client, - uriFromHost(host as string), - headers, - body, - params, - function (err?: ErrnoException | ErrorInfo | null, ...args: unknown[]) { - // This typecast is safe because ErrnoExceptions are only thrown in NodeJS - if (err && shouldFallback(err as ErrorInfo) && candidateHosts.length) { - tryAHost(candidateHosts, true); - return; - } - if (persistOnSuccess) { - /* RSC15f */ - client._currentFallback = { - host: host as string, - validUntil: Utils.now() + client.options.timeouts.fallbackRetryTimeout, - }; - } - callback?.(err, ...args); + this.doUri(method, client, 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); + return; + } + if (persistOnSuccess) { + /* RSC15f */ + client._currentFallback = { + host: host as string, + validUntil: Utils.now() + client.options.timeouts.fallbackRetryTimeout, + }; } - ); + callback?.(err, ...args); + }); }; tryAHost(hosts); } diff --git a/tsconfig.json b/tsconfig.json index b3a547de9d..1146fcb184 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -2,7 +2,7 @@ "compilerOptions": { "target": "es5", "module": "commonjs", - "lib": ["ES5", "DOM", "webworker"], + "lib": ["ES5", "DOM", "DOM.Iterable", "webworker"], "strict": true, "esModuleInterop": true, "skipLibCheck": true, From fbf30e69df658a409cd726971bc0be61848af82c Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 26 Oct 2023 09:03:12 -0300 Subject: [PATCH 3/8] Introduce a directory structure for web HTTP stuff Similar to that used for transports. --- src/platform/nativescript/index.ts | 2 +- src/platform/react-native/index.ts | 2 +- src/platform/web-noencryption/index.ts | 2 +- src/platform/web/index.ts | 2 +- src/platform/web/lib/{util => http}/http.ts | 4 ++-- .../web/lib/{transport => http/request}/fetchrequest.ts | 0 .../web/lib/{transport => http/request}/xhrrequest.ts | 0 src/platform/web/lib/transport/xhrpollingtransport.ts | 2 +- src/platform/web/lib/transport/xhrstreamingtransport.ts | 2 +- src/platform/web/modules.ts | 2 +- 10 files changed, 9 insertions(+), 9 deletions(-) rename src/platform/web/lib/{util => http}/http.ts (98%) rename src/platform/web/lib/{transport => http/request}/fetchrequest.ts (100%) rename src/platform/web/lib/{transport => http/request}/xhrrequest.ts (100%) diff --git a/src/platform/nativescript/index.ts b/src/platform/nativescript/index.ts index f1ea2e7285..c8354aa69d 100644 --- a/src/platform/nativescript/index.ts +++ b/src/platform/nativescript/index.ts @@ -8,7 +8,7 @@ import ErrorInfo from '../../common/lib/types/errorinfo'; import BufferUtils from '../web/lib/util/bufferutils'; // @ts-ignore import { createCryptoClass } from '../web/lib/util/crypto'; -import Http from '../web/lib/util/http'; +import Http from '../web/lib/http/http'; // @ts-ignore import Config from './config'; // @ts-ignore diff --git a/src/platform/react-native/index.ts b/src/platform/react-native/index.ts index 3b7d6debeb..dc27bb62c8 100644 --- a/src/platform/react-native/index.ts +++ b/src/platform/react-native/index.ts @@ -8,7 +8,7 @@ import ErrorInfo from '../../common/lib/types/errorinfo'; import BufferUtils from '../web/lib/util/bufferutils'; // @ts-ignore import { createCryptoClass } from '../web/lib/util/crypto'; -import Http from '../web/lib/util/http'; +import Http from '../web/lib/http/http'; import configFactory from './config'; // @ts-ignore import Transports from '../web/lib/transport'; diff --git a/src/platform/web-noencryption/index.ts b/src/platform/web-noencryption/index.ts index 4b8d3ed9de..dcb4120123 100644 --- a/src/platform/web-noencryption/index.ts +++ b/src/platform/web-noencryption/index.ts @@ -7,7 +7,7 @@ import ErrorInfo from '../../common/lib/types/errorinfo'; // Platform Specific import BufferUtils from '../web/lib/util/bufferutils'; // @ts-ignore -import Http from '../web/lib/util/http'; +import Http from '../web/lib/http/http'; import Config from '../web/config'; // @ts-ignore import Transports from '../web/lib/transport'; diff --git a/src/platform/web/index.ts b/src/platform/web/index.ts index d4566814d5..3e40e123b7 100644 --- a/src/platform/web/index.ts +++ b/src/platform/web/index.ts @@ -8,7 +8,7 @@ import ErrorInfo from '../../common/lib/types/errorinfo'; import BufferUtils from './lib/util/bufferutils'; // @ts-ignore import { createCryptoClass } from './lib/util/crypto'; -import Http from './lib/util/http'; +import Http from './lib/http/http'; import Config from './config'; // @ts-ignore import Transports from './lib/transport'; diff --git a/src/platform/web/lib/util/http.ts b/src/platform/web/lib/http/http.ts similarity index 98% rename from src/platform/web/lib/util/http.ts rename to src/platform/web/lib/http/http.ts index d6cb949357..f7dcdd1285 100644 --- a/src/platform/web/lib/util/http.ts +++ b/src/platform/web/lib/http/http.ts @@ -6,11 +6,11 @@ import { IHttpStatic, RequestCallback, RequestParams } from 'common/types/http'; import HttpMethods from 'common/constants/HttpMethods'; import BaseClient from 'common/lib/client/baseclient'; import BaseRealtime from 'common/lib/client/baserealtime'; -import XHRRequest from '../transport/xhrrequest'; +import XHRRequest from './request/xhrrequest'; import XHRStates from 'common/constants/XHRStates'; import Logger from 'common/lib/util/logger'; import { StandardCallback } from 'common/types/utils'; -import fetchRequest from '../transport/fetchrequest'; +import fetchRequest from './request/fetchrequest'; import { NormalisedClientOptions } from 'common/types/ClientOptions'; import { isSuccessCode } from 'common/constants/HttpStatusCodes'; diff --git a/src/platform/web/lib/transport/fetchrequest.ts b/src/platform/web/lib/http/request/fetchrequest.ts similarity index 100% rename from src/platform/web/lib/transport/fetchrequest.ts rename to src/platform/web/lib/http/request/fetchrequest.ts diff --git a/src/platform/web/lib/transport/xhrrequest.ts b/src/platform/web/lib/http/request/xhrrequest.ts similarity index 100% rename from src/platform/web/lib/transport/xhrrequest.ts rename to src/platform/web/lib/http/request/xhrrequest.ts diff --git a/src/platform/web/lib/transport/xhrpollingtransport.ts b/src/platform/web/lib/transport/xhrpollingtransport.ts index 4d3b2110d3..bf0f13befd 100644 --- a/src/platform/web/lib/transport/xhrpollingtransport.ts +++ b/src/platform/web/lib/transport/xhrpollingtransport.ts @@ -1,6 +1,6 @@ import Platform from '../../../../common/platform'; import CometTransport from '../../../../common/lib/transport/comettransport'; -import XHRRequest from './xhrrequest'; +import XHRRequest from '../http/request/xhrrequest'; import ConnectionManager, { TransportParams, TransportStorage } from 'common/lib/transport/connectionmanager'; import Auth from 'common/lib/client/auth'; import { RequestParams } from 'common/types/http'; diff --git a/src/platform/web/lib/transport/xhrstreamingtransport.ts b/src/platform/web/lib/transport/xhrstreamingtransport.ts index 9ef8631fec..5dd77e6f6d 100644 --- a/src/platform/web/lib/transport/xhrstreamingtransport.ts +++ b/src/platform/web/lib/transport/xhrstreamingtransport.ts @@ -1,6 +1,6 @@ import CometTransport from '../../../../common/lib/transport/comettransport'; import Platform from '../../../../common/platform'; -import XHRRequest from './xhrrequest'; +import XHRRequest from '../http/request/xhrrequest'; import ConnectionManager, { TransportParams, TransportStorage } from 'common/lib/transport/connectionmanager'; import Auth from 'common/lib/client/auth'; import { RequestParams } from 'common/types/http'; diff --git a/src/platform/web/modules.ts b/src/platform/web/modules.ts index a3bf018afe..f9ee3a521d 100644 --- a/src/platform/web/modules.ts +++ b/src/platform/web/modules.ts @@ -7,7 +7,7 @@ import ErrorInfo from '../../common/lib/types/errorinfo'; // Platform Specific import BufferUtils from './lib/util/bufferutils'; // @ts-ignore -import Http from './lib/util/http'; +import Http from './lib/http/http'; import Config from './config'; // @ts-ignore import { ModulesTransports } from './lib/transport'; From 1d35274d14f47aa5db969001950cc51295c55084 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 26 Oct 2023 10:20:04 -0300 Subject: [PATCH 4/8] =?UTF-8?q?Make=20IHttpStatic=E2=80=99s=20constructor?= =?UTF-8?q?=20options=20param=20optional?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To correctly reflect the fact that the tests instantiate Platform.Http without any arguments. --- src/common/types/http.ts | 3 +-- src/platform/nodejs/lib/util/http.ts | 14 +++++++------- src/platform/web/lib/http/http.ts | 13 +++++-------- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/common/types/http.ts b/src/common/types/http.ts index 404fb6c156..086f55c2d9 100644 --- a/src/common/types/http.ts +++ b/src/common/types/http.ts @@ -16,7 +16,7 @@ export type RequestCallback = ( export type RequestParams = Record | null; export interface IHttpStatic { - new (options: NormalisedClientOptions): IHttp; + new (options?: NormalisedClientOptions): IHttp; methods: Array; methodsWithBody: Array; methodsWithoutBody: Array; @@ -26,7 +26,6 @@ export interface IHttp { supportsAuthHeaders: boolean; supportsLinkHeaders: boolean; agent?: Agents | null; - options: NormalisedClientOptions; Request?: ( method: HttpMethods, diff --git a/src/platform/nodejs/lib/util/http.ts b/src/platform/nodejs/lib/util/http.ts index e24628fcc9..1edeadef17 100644 --- a/src/platform/nodejs/lib/util/http.ts +++ b/src/platform/nodejs/lib/util/http.ts @@ -105,10 +105,10 @@ const Http: IHttpStatic = class { _getHosts = getHosts; supportsAuthHeaders = true; supportsLinkHeaders = true; - options: NormalisedClientOptions; + private options: NormalisedClientOptions | null; - constructor(options: NormalisedClientOptions) { - this.options = options || {}; + constructor(options?: NormalisedClientOptions) { + this.options = options ?? null; } /* Unlike for doUri, the 'client' param here is mandatory, as it's used to generate the hosts */ @@ -235,13 +235,13 @@ const Http: IHttpStatic = class { } checkConnectivity = (callback: (errorInfo: ErrorInfo | null, connected?: boolean) => void): void => { - if (this.options.disableConnectivityCheck) { + if (this.options?.disableConnectivityCheck) { callback(null, true); return; } - const connectivityCheckUrl = this.options.connectivityCheckUrl || Defaults.connectivityCheckUrl; - const connectivityCheckParams = this.options.connectivityCheckParams; - const connectivityUrlIsDefault = !this.options.connectivityCheckUrl; + const connectivityCheckUrl = this.options?.connectivityCheckUrl || Defaults.connectivityCheckUrl; + const connectivityCheckParams = this.options?.connectivityCheckParams ?? null; + const connectivityUrlIsDefault = !this.options?.connectivityCheckUrl; this.doUri( HttpMethods.Get, diff --git a/src/platform/web/lib/http/http.ts b/src/platform/web/lib/http/http.ts index f7dcdd1285..0b6bd62028 100644 --- a/src/platform/web/lib/http/http.ts +++ b/src/platform/web/lib/http/http.ts @@ -45,14 +45,11 @@ const Http: IHttpStatic = class { static methodsWithoutBody = [HttpMethods.Get, HttpMethods.Delete]; static methodsWithBody = [HttpMethods.Post, HttpMethods.Put, HttpMethods.Patch]; checksInProgress: Array> | null = null; - options: NormalisedClientOptions; - constructor(options: NormalisedClientOptions) { - this.options = options || {}; - - const connectivityCheckUrl = this.options.connectivityCheckUrl || Defaults.connectivityCheckUrl; - const connectivityCheckParams = this.options.connectivityCheckParams; - const connectivityUrlIsDefault = !this.options.connectivityCheckUrl; + constructor(options?: NormalisedClientOptions) { + const connectivityCheckUrl = options?.connectivityCheckUrl || Defaults.connectivityCheckUrl; + const connectivityCheckParams = options?.connectivityCheckParams ?? null; + const connectivityUrlIsDefault = !options?.connectivityCheckUrl; if (Platform.Config.xhrSupported) { this.supportsAuthHeaders = true; this.Request = function ( @@ -77,7 +74,7 @@ const Http: IHttpStatic = class { req.exec(); return req; }; - if (this.options.disableConnectivityCheck) { + if (options?.disableConnectivityCheck) { this.checkConnectivity = function (callback: (err: null, connectivity: true) => void) { callback(null, true); }; From 9ad0a869422f613d52e4f61f7f22988fe5433411 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 26 Oct 2023 10:55:15 -0300 Subject: [PATCH 5/8] =?UTF-8?q?Pass=20client=20to=20IHttpStatic=E2=80=99s?= =?UTF-8?q?=20constructor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/common/lib/client/auth.ts | 3 -- src/common/lib/client/baseclient.ts | 2 +- src/common/lib/client/resource.ts | 2 +- src/common/lib/client/rest.ts | 1 - src/common/types/http.ts | 6 +--- src/platform/nodejs/lib/util/http.ts | 48 +++++++++++++++------------- src/platform/web/lib/http/http.ts | 44 +++++++++++++------------ test/browser/modules.test.js | 2 +- test/realtime/auth.test.js | 2 +- test/rest/http.test.js | 4 +-- test/rest/message.test.js | 4 +-- test/rest/request.test.js | 2 +- 12 files changed, 60 insertions(+), 60 deletions(-) diff --git a/src/common/lib/client/auth.ts b/src/common/lib/client/auth.ts index 3ed509ed6d..b4db9380c4 100644 --- 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 index 939257edb7..fbb2be3475 100644 --- 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 index 0fa0b93c45..3f0a8d68da 100644 --- 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 index 6d1c08c4d0..a3592f9945 100644 --- 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 index 086f55c2d9..a13ba86e49 100644 --- 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>; @@ -16,7 +15,7 @@ export type RequestCallback = ( export type RequestParams = Record | null; export interface IHttpStatic { - new (options?: NormalisedClientOptions): IHttp; + new (client?: BaseClient): IHttp; methods: Array; methodsWithBody: Array; methodsWithoutBody: Array; @@ -29,7 +28,6 @@ export interface IHttp { Request?: ( method: HttpMethods, - client: BaseClient | null, uri: string, headers: Record | null, params: RequestParams, @@ -39,7 +37,6 @@ export interface IHttp { _getHosts: (client: BaseClient) => string[]; do( method: HttpMethods, - client: BaseClient | null, path: PathParameter, headers: Record | null, body: unknown, @@ -48,7 +45,6 @@ export interface IHttp { ): void; doUri( method: HttpMethods, - client: BaseClient | null, uri: string, headers: Record | null, body: unknown, diff --git a/src/platform/nodejs/lib/util/http.ts b/src/platform/nodejs/lib/util/http.ts index 1edeadef17..6ed73b76d5 100644 --- 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 | 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, 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 | 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,7 +219,9 @@ 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 @@ -223,29 +229,28 @@ const Http: IHttpStatic = class { (got[method](doOptions) as CancelableRequest) .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 | null, params: RequestParams, diff --git a/src/platform/web/lib/http/http.ts b/src/platform/web/lib/http/http.ts index 0b6bd62028..b8566c5633 100644 --- 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> | 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 | 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 | 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, 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 | 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 | null, params: RequestParams, diff --git a/test/browser/modules.test.js b/test/browser/modules.test.js index 501bd09f9d..efaf654b3b 100644 --- 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; } diff --git a/test/realtime/auth.test.js b/test/realtime/auth.test.js index 92dde3019a..affb68bae6 100644 --- a/test/realtime/auth.test.js +++ b/test/realtime/auth.test.js @@ -22,7 +22,7 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async */ function getJWT(params, callback) { var authUrl = echoServer + '/createJWT'; - http.doUri('get', null, authUrl, null, null, params, function (err, body) { + http.doUri('get', authUrl, null, null, params, function (err, body) { if (err) { callback(err, null); } diff --git a/test/rest/http.test.js b/test/rest/http.test.js index 315f751551..dcd7ee8262 100644 --- a/test/rest/http.test.js +++ b/test/rest/http.test.js @@ -25,7 +25,7 @@ define(['ably', 'shared_helper', 'chai'], function (Ably, helper, chai) { var originalDo = rest.http.do; // Intercept Http.do with test - function testRequestHandler(method, rest, path, headers, body, params, callback) { + function testRequestHandler(method, path, headers, body, params, callback) { expect('X-Ably-Version' in headers, 'Verify version header exists').to.be.ok; expect('Ably-Agent' in headers, 'Verify agent header exists').to.be.ok; @@ -47,7 +47,7 @@ define(['ably', 'shared_helper', 'chai'], function (Ably, helper, chai) { expect(headers['Ably-Agent'].indexOf('nodejs') > -1, 'Verify agent').to.be.ok; } - originalDo.call(rest.http, method, rest, path, headers, body, params, callback); + originalDo.call(rest.http, method, path, headers, body, params, callback); } rest.http.do = testRequestHandler; diff --git a/test/rest/message.test.js b/test/rest/message.test.js index 389e0b7d74..090e6eba70 100644 --- a/test/rest/message.test.js +++ b/test/rest/message.test.js @@ -157,8 +157,8 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async originalPublish.apply(channel, arguments); }; - Ably.Rest.Platform.Http.doUri = function (method, rest, uri, headers, body, params, callback) { - originalDoUri(method, rest, uri, headers, body, params, function (err) { + Ably.Rest.Platform.Http.doUri = function (method, uri, headers, body, params, callback) { + originalDoUri(method, uri, headers, body, params, function (err) { if (err) { callback(err); return; diff --git a/test/rest/request.test.js b/test/rest/request.test.js index 7055f46191..c123e94606 100644 --- a/test/rest/request.test.js +++ b/test/rest/request.test.js @@ -25,7 +25,7 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async restTestOnJsonMsgpack('request_version', function (rest) { const version = 150; // arbitrarily chosen - function testRequestHandler(_, __, ___, headers) { + function testRequestHandler(_, __, headers) { try { expect('X-Ably-Version' in headers, 'Verify version header exists').to.be.ok; expect(headers['X-Ably-Version']).to.equal(version.toString(), 'Verify version number sent in request'); From 674e88afc2a232aac2b934948f665943fd897534 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Tue, 5 Sep 2023 10:26:07 +0100 Subject: [PATCH 6/8] Make HTTP request implementations tree-shakable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We expose XHRRequest and FetchRequest modules. The user is required to provide an HTTP module, even for Realtime, since it’s used for the internet connectivity check and for making a token request to the authUrl. Resolves #1395. --- scripts/moduleReport.js | 2 + src/common/lib/client/baseclient.ts | 5 ++ src/common/lib/client/modulesmap.ts | 4 ++ src/platform/nativescript/index.ts | 3 + src/platform/react-native/index.ts | 3 + src/platform/web-noencryption/index.ts | 3 + src/platform/web/index.ts | 3 + src/platform/web/lib/http/http.ts | 45 +++++++++--- src/platform/web/lib/http/request/index.ts | 10 +++ src/platform/web/modules.ts | 4 ++ src/platform/web/modules/http.ts | 2 + test/browser/modules.test.js | 84 +++++++++++++++------- 12 files changed, 135 insertions(+), 33 deletions(-) create mode 100644 src/platform/web/lib/http/request/index.ts create mode 100644 src/platform/web/modules/http.ts diff --git a/scripts/moduleReport.js b/scripts/moduleReport.js index ee5cadf0a8..f82a58ea8a 100644 --- a/scripts/moduleReport.js +++ b/scripts/moduleReport.js @@ -9,6 +9,8 @@ const moduleNames = [ 'XHRPolling', 'XHRStreaming', 'WebSocketTransport', + 'XHRRequest', + 'FetchRequest', ]; // List of all free-standing functions exported by the library along with the diff --git a/src/common/lib/client/baseclient.ts b/src/common/lib/client/baseclient.ts index fbb2be3475..ddc726b4d5 100644 --- a/src/common/lib/client/baseclient.ts +++ b/src/common/lib/client/baseclient.ts @@ -15,6 +15,7 @@ import { Rest } from './rest'; import { IUntypedCryptoStatic } from 'common/types/ICryptoStatic'; import { throwMissingModuleError } from '../util/utils'; import { MsgPack } from 'common/types/msgpack'; +import { HTTPRequestImplementations } from 'platform/web/lib/http/http'; type BatchResult = API.Types.BatchResult; type BatchPublishSpec = API.Types.BatchPublishSpec; @@ -41,8 +42,12 @@ class BaseClient { private readonly _rest: Rest | null; readonly _Crypto: IUntypedCryptoStatic | null; readonly _MsgPack: MsgPack | null; + // Extra HTTP request implementations available to this client, in addition to those in web’s Http.bundledRequestImplementations + readonly _additionalHTTPRequestImplementations: HTTPRequestImplementations; constructor(options: ClientOptions | string, modules: ModulesMap) { + this._additionalHTTPRequestImplementations = modules; + if (!options) { const msg = 'no options provided'; Logger.logAction(Logger.LOG_ERROR, 'BaseClient()', msg); diff --git a/src/common/lib/client/modulesmap.ts b/src/common/lib/client/modulesmap.ts index ada7de44ee..dff32a69e4 100644 --- a/src/common/lib/client/modulesmap.ts +++ b/src/common/lib/client/modulesmap.ts @@ -3,6 +3,8 @@ import { IUntypedCryptoStatic } from '../../types/ICryptoStatic'; import { MsgPack } from 'common/types/msgpack'; import RealtimePresence from './realtimepresence'; import { TransportInitialiser } from '../transport/connectionmanager'; +import XHRRequest from 'platform/web/lib/http/request/xhrrequest'; +import fetchRequest from 'platform/web/lib/http/request/fetchrequest'; export interface ModulesMap { Rest?: typeof Rest; @@ -12,6 +14,8 @@ export interface ModulesMap { WebSocketTransport?: TransportInitialiser; XHRPolling?: TransportInitialiser; XHRStreaming?: TransportInitialiser; + XHRRequest?: typeof XHRRequest; + FetchRequest?: typeof fetchRequest; } export const allCommonModules: ModulesMap = { Rest }; diff --git a/src/platform/nativescript/index.ts b/src/platform/nativescript/index.ts index c8354aa69d..119fdcb048 100644 --- a/src/platform/nativescript/index.ts +++ b/src/platform/nativescript/index.ts @@ -19,6 +19,7 @@ import { getDefaults } from '../../common/lib/util/defaults'; import WebStorage from './lib/util/webstorage'; import PlatformDefaults from '../web/lib/util/defaults'; import msgpack from '../web/lib/util/msgpack'; +import { defaultBundledRequestImplementations } from '../web/lib/http/request'; const Crypto = createCryptoClass(Config, BufferUtils); @@ -34,6 +35,8 @@ for (const clientClass of [DefaultRest, DefaultRealtime]) { clientClass._MsgPack = msgpack; } +Http.bundledRequestImplementations = defaultBundledRequestImplementations; + Logger.initLogHandlers(); Platform.Defaults = getDefaults(PlatformDefaults); diff --git a/src/platform/react-native/index.ts b/src/platform/react-native/index.ts index dc27bb62c8..e0539aa92a 100644 --- a/src/platform/react-native/index.ts +++ b/src/platform/react-native/index.ts @@ -17,6 +17,7 @@ import { getDefaults } from '../../common/lib/util/defaults'; import WebStorage from '../web/lib/util/webstorage'; import PlatformDefaults from '../web/lib/util/defaults'; import msgpack from '../web/lib/util/msgpack'; +import { defaultBundledRequestImplementations } from '../web/lib/http/request'; const Config = configFactory(BufferUtils); @@ -34,6 +35,8 @@ for (const clientClass of [DefaultRest, DefaultRealtime]) { clientClass._MsgPack = msgpack; } +Http.bundledRequestImplementations = defaultBundledRequestImplementations; + Logger.initLogHandlers(); Platform.Defaults = getDefaults(PlatformDefaults); diff --git a/src/platform/web-noencryption/index.ts b/src/platform/web-noencryption/index.ts index dcb4120123..64dcf02b5e 100644 --- a/src/platform/web-noencryption/index.ts +++ b/src/platform/web-noencryption/index.ts @@ -16,6 +16,7 @@ import { getDefaults } from '../../common/lib/util/defaults'; import WebStorage from '../web/lib/util/webstorage'; import PlatformDefaults from '../web/lib/util/defaults'; import msgpack from '../web/lib/util/msgpack'; +import { defaultBundledRequestImplementations } from '../web/lib/http/request'; Platform.Crypto = null; Platform.BufferUtils = BufferUtils; @@ -28,6 +29,8 @@ for (const clientClass of [DefaultRest, DefaultRealtime]) { clientClass._MsgPack = msgpack; } +Http.bundledRequestImplementations = defaultBundledRequestImplementations; + Logger.initLogHandlers(); Platform.Defaults = getDefaults(PlatformDefaults); diff --git a/src/platform/web/index.ts b/src/platform/web/index.ts index 3e40e123b7..27d6c9556b 100644 --- a/src/platform/web/index.ts +++ b/src/platform/web/index.ts @@ -17,6 +17,7 @@ import { getDefaults } from '../../common/lib/util/defaults'; import WebStorage from './lib/util/webstorage'; import PlatformDefaults from './lib/util/defaults'; import msgpack from './lib/util/msgpack'; +import { defaultBundledRequestImplementations } from './lib/http/request'; const Crypto = createCryptoClass(Config, BufferUtils); @@ -32,6 +33,8 @@ for (const clientClass of [DefaultRest, DefaultRealtime]) { clientClass._MsgPack = msgpack; } +Http.bundledRequestImplementations = defaultBundledRequestImplementations; + Logger.initLogHandlers(); Platform.Defaults = getDefaults(PlatformDefaults); diff --git a/src/platform/web/lib/http/http.ts b/src/platform/web/lib/http/http.ts index b8566c5633..626a946758 100644 --- a/src/platform/web/lib/http/http.ts +++ b/src/platform/web/lib/http/http.ts @@ -2,16 +2,17 @@ import Platform from 'common/platform'; import * as Utils from 'common/lib/util/utils'; import Defaults from 'common/lib/util/defaults'; import ErrorInfo, { PartialErrorInfo } from 'common/lib/types/errorinfo'; -import { IHttpStatic, RequestCallback, RequestParams } from 'common/types/http'; +import { RequestCallback, RequestParams } from 'common/types/http'; import HttpMethods from 'common/constants/HttpMethods'; import BaseClient from 'common/lib/client/baseclient'; import BaseRealtime from 'common/lib/client/baserealtime'; -import XHRRequest from './request/xhrrequest'; 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 { isSuccessCode } from 'common/constants/HttpStatusCodes'; +import { ModulesMap } from 'common/lib/client/modulesmap'; + +export type HTTPRequestImplementations = Pick; function shouldFallback(errorInfo: ErrorInfo) { const statusCode = errorInfo.statusCode as number; @@ -39,10 +40,20 @@ function getHosts(client: BaseClient): string[] { return Defaults.getHosts(client.options); } -const Http: IHttpStatic = class { +function createMissingImplementationError() { + return new ErrorInfo( + 'No HTTP request module provided. Provide at least one of the FetchRequest or XHRRequest modules.', + 400, + 40000 + ); +} + +const Http = class { static methods = [HttpMethods.Get, HttpMethods.Delete, HttpMethods.Post, HttpMethods.Put, HttpMethods.Patch]; static methodsWithoutBody = [HttpMethods.Get, HttpMethods.Delete]; static methodsWithBody = [HttpMethods.Post, HttpMethods.Put, HttpMethods.Patch]; + // HTTP request implementations that are available even without a BaseClient object (needed by some tests which directly instantiate `Http` without a client) + static bundledRequestImplementations: HTTPRequestImplementations; checksInProgress: Array> | null = null; private client: BaseClient | null; @@ -51,7 +62,20 @@ const Http: IHttpStatic = class { const connectivityCheckUrl = client?.options.connectivityCheckUrl || Defaults.connectivityCheckUrl; const connectivityCheckParams = client?.options.connectivityCheckParams ?? null; const connectivityUrlIsDefault = !client?.options.connectivityCheckUrl; - if (Platform.Config.xhrSupported) { + + const requestImplementations = { + ...Http.bundledRequestImplementations, + ...client?._additionalHTTPRequestImplementations, + }; + const xhrRequestImplementation = requestImplementations.XHRRequest; + const fetchRequestImplementation = requestImplementations.FetchRequest; + const hasImplementation = !!(xhrRequestImplementation || fetchRequestImplementation); + + if (!hasImplementation) { + throw createMissingImplementationError(); + } + + if (Platform.Config.xhrSupported && xhrRequestImplementation) { this.supportsAuthHeaders = true; this.Request = function ( method: HttpMethods, @@ -61,7 +85,7 @@ const Http: IHttpStatic = class { body: unknown, callback: RequestCallback ) { - const req = XHRRequest.createRequest( + const req = xhrRequestImplementation.createRequest( uri, headers, params, @@ -104,10 +128,10 @@ const Http: IHttpStatic = class { ); }; } - } else if (Platform.Config.fetchSupported) { + } else if (Platform.Config.fetchSupported && fetchRequestImplementation) { this.supportsAuthHeaders = true; this.Request = (method, uri, headers, params, body, callback) => { - fetchRequest(method, client ?? null, uri, headers, params, body, callback); + fetchRequestImplementation(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); @@ -119,7 +143,10 @@ const Http: IHttpStatic = class { }; } else { this.Request = (method, uri, headers, params, body, callback) => { - callback(new PartialErrorInfo('no supported HTTP transports available', null, 400), null); + const error = hasImplementation + ? new PartialErrorInfo('no supported HTTP transports available', null, 400) + : createMissingImplementationError(); + callback(error, null); }; } } diff --git a/src/platform/web/lib/http/request/index.ts b/src/platform/web/lib/http/request/index.ts new file mode 100644 index 0000000000..4fccec5b3b --- /dev/null +++ b/src/platform/web/lib/http/request/index.ts @@ -0,0 +1,10 @@ +import { HTTPRequestImplementations } from '../http'; +import XHRRequest from './xhrrequest'; +import fetchRequest from './fetchrequest'; + +export const defaultBundledRequestImplementations: HTTPRequestImplementations = { + XHRRequest: XHRRequest, + FetchRequest: fetchRequest, +}; + +export const modulesBundledRequestImplementations: HTTPRequestImplementations = {}; diff --git a/src/platform/web/modules.ts b/src/platform/web/modules.ts index f9ee3a521d..e12ede52e7 100644 --- a/src/platform/web/modules.ts +++ b/src/platform/web/modules.ts @@ -15,6 +15,7 @@ import Logger from '../../common/lib/util/logger'; import { getDefaults } from '../../common/lib/util/defaults'; import WebStorage from './lib/util/webstorage'; import PlatformDefaults from './lib/util/defaults'; +import { modulesBundledRequestImplementations } from './lib/http/request'; Platform.BufferUtils = BufferUtils; Platform.Http = Http; @@ -22,6 +23,8 @@ Platform.Config = Config; Platform.Transports = ModulesTransports; Platform.WebStorage = WebStorage; +Http.bundledRequestImplementations = modulesBundledRequestImplementations; + Logger.initLogHandlers(); Platform.Defaults = getDefaults(PlatformDefaults); @@ -45,5 +48,6 @@ export * from './modules/presencemessage'; export * from './modules/msgpack'; export * from './modules/realtimepresence'; export * from './modules/transports'; +export * from './modules/http'; export { Rest } from '../../common/lib/client/rest'; export { BaseRest, BaseRealtime, ErrorInfo }; diff --git a/src/platform/web/modules/http.ts b/src/platform/web/modules/http.ts new file mode 100644 index 0000000000..24b664f30d --- /dev/null +++ b/src/platform/web/modules/http.ts @@ -0,0 +1,2 @@ +export { default as XHRRequest } from '../lib/http/request/xhrrequest'; +export { default as FetchRequest } from '../lib/http/request/fetchrequest'; diff --git a/test/browser/modules.test.js b/test/browser/modules.test.js index efaf654b3b..d73b76624e 100644 --- a/test/browser/modules.test.js +++ b/test/browser/modules.test.js @@ -17,6 +17,8 @@ import { XHRPolling, XHRStreaming, WebSocketTransport, + FetchRequest, + XHRRequest, } from '../../build/modules/index.js'; describe('browser/modules', function () { @@ -45,23 +47,21 @@ describe('browser/modules', function () { }); describe('without any modules', () => { - describe('BaseRest', () => { - it('can be constructed', () => { - expect(() => new BaseRest(ablyClientOptions(), {})).not.to.throw(); - }); - }); - - describe('BaseRealtime', () => { - it('throws an error due to absence of a transport module', () => { - expect(() => new BaseRealtime(ablyClientOptions(), {})).to.throw('no requested transports available'); + for (const clientClass of [BaseRest, BaseRealtime]) { + describe(clientClass.name, () => { + it('throws an error due to the absence of an HTTP module', () => { + expect(() => new clientClass(ablyClientOptions(), {})).to.throw( + 'No HTTP request module provided. Provide at least one of the FetchRequest or XHRRequest modules.' + ); + }); }); - }); + } }); describe('Rest', () => { describe('BaseRest without explicit Rest', () => { it('offers REST functionality', async () => { - const client = new BaseRest(ablyClientOptions(), {}); + const client = new BaseRest(ablyClientOptions(), { FetchRequest }); const time = await client.time(); expect(time).to.be.a('number'); }); @@ -69,7 +69,7 @@ describe('browser/modules', function () { describe('BaseRealtime with Rest', () => { it('offers REST functionality', async () => { - const client = new BaseRealtime(ablyClientOptions(), { WebSocketTransport, Rest }); + const client = new BaseRealtime(ablyClientOptions(), { WebSocketTransport, FetchRequest, Rest }); const time = await client.time(); expect(time).to.be.a('number'); }); @@ -77,7 +77,7 @@ describe('browser/modules', function () { describe('BaseRealtime without Rest', () => { it('throws an error when attempting to use REST functionality', async () => { - const client = new BaseRealtime(ablyClientOptions(), { WebSocketTransport }); + const client = new BaseRealtime(ablyClientOptions(), { WebSocketTransport, FetchRequest }); expect(() => client.time()).to.throw('Rest module not provided'); }); }); @@ -214,10 +214,10 @@ describe('browser/modules', function () { describe('Crypto', () => { describe('without Crypto', () => { async function testThrowsAnErrorWhenGivenChannelOptionsWithACipher(clientClassConfig) { - const client = new clientClassConfig.clientClass( - ablyClientOptions(), - clientClassConfig.additionalModules ?? {} - ); + const client = new clientClassConfig.clientClass(ablyClientOptions(), { + ...clientClassConfig.additionalModules, + FetchRequest, + }); const key = await generateRandomKey(); expect(() => client.channels.get('channel', { cipher: { key } })).to.throw('Crypto module not provided'); } @@ -242,7 +242,7 @@ describe('browser/modules', function () { // Publish the message on a channel configured to use encryption, and receive it on one not configured to use encryption - const rxClient = new BaseRealtime(clientOptions, { WebSocketTransport }); + const rxClient = new BaseRealtime(clientOptions, { WebSocketTransport, FetchRequest }); const rxChannel = rxClient.channels.get('channel'); await rxChannel.attach(); @@ -252,7 +252,8 @@ describe('browser/modules', function () { const txMessage = { name: 'message', data: 'data' }; const txClient = new clientClassConfig.clientClass(clientOptions, { - ...(clientClassConfig.additionalModules ?? {}), + ...clientClassConfig.additionalModules, + FetchRequest, Crypto, }); const txChannel = txClient.channels.get('channel', encryptionChannelOptions); @@ -318,7 +319,7 @@ describe('browser/modules', function () { describe('without MsgPack', () => { describe('BaseRest', () => { it('uses JSON', async () => { - const client = new BaseRest(ablyClientOptions({ useBinaryProtocol: true }), {}); + const client = new BaseRest(ablyClientOptions({ useBinaryProtocol: true }), { FetchRequest }); await testRestUsesContentType(client, 'application/json'); }); }); @@ -327,6 +328,7 @@ describe('browser/modules', function () { it('uses JSON', async () => { const client = new BaseRealtime(ablyClientOptions({ useBinaryProtocol: true, autoConnect: false }), { WebSocketTransport, + FetchRequest, }); await testRealtimeUsesFormat(client, 'json'); }); @@ -337,6 +339,7 @@ describe('browser/modules', function () { describe('BaseRest', () => { it('uses MessagePack', async () => { const client = new BaseRest(ablyClientOptions({ useBinaryProtocol: true }), { + FetchRequest, MsgPack, }); await testRestUsesContentType(client, 'application/x-msgpack'); @@ -347,6 +350,7 @@ describe('browser/modules', function () { it('uses MessagePack', async () => { const client = new BaseRealtime(ablyClientOptions({ useBinaryProtocol: true, autoConnect: false }), { WebSocketTransport, + FetchRequest, MsgPack, }); await testRealtimeUsesFormat(client, 'msgpack'); @@ -359,7 +363,7 @@ describe('browser/modules', function () { describe('RealtimePresence', () => { describe('BaseRealtime without RealtimePresence', () => { it('throws an error when attempting to access the `presence` property', () => { - const client = new BaseRealtime(ablyClientOptions(), { WebSocketTransport }); + const client = new BaseRealtime(ablyClientOptions(), { WebSocketTransport, FetchRequest }); const channel = client.channels.get('channel'); expect(() => channel.presence).to.throw('RealtimePresence module not provided'); @@ -368,12 +372,15 @@ describe('browser/modules', function () { describe('BaseRealtime with RealtimePresence', () => { it('offers realtime presence functionality', async () => { - const rxChannel = new BaseRealtime(ablyClientOptions(), { WebSocketTransport, RealtimePresence }).channels.get( - 'channel' - ); + const rxChannel = new BaseRealtime(ablyClientOptions(), { + WebSocketTransport, + FetchRequest, + RealtimePresence, + }).channels.get('channel'); const txClientId = randomString(); const txChannel = new BaseRealtime(ablyClientOptions({ clientId: txClientId }), { WebSocketTransport, + FetchRequest, RealtimePresence, }).channels.get('channel'); @@ -435,6 +442,14 @@ describe('browser/modules', function () { describe('Transports', () => { describe('BaseRealtime', () => { + describe('without a transport module', () => { + it('throws an error due to absence of a transport module', () => { + expect(() => new BaseRealtime(ablyClientOptions(), { FetchRequest })).to.throw( + 'no requested transports available' + ); + }); + }); + for (const scenario of [ { moduleMapKey: 'WebSocketTransport', transportModule: WebSocketTransport, transportName: 'web_socket' }, { moduleMapKey: 'XHRPolling', transportModule: XHRPolling, transportName: 'xhr_polling' }, @@ -445,6 +460,7 @@ describe('browser/modules', function () { const realtime = new BaseRealtime( ablyClientOptions({ autoConnect: false, transports: [scenario.transportName] }), { + FetchRequest, [scenario.moduleMapKey]: scenario.transportModule, } ); @@ -468,4 +484,24 @@ describe('browser/modules', function () { } }); }); + + describe('HTTP request implementations', () => { + describe('with multiple HTTP request implementations', () => { + it('prefers XHR', async () => { + let usedXHR = false; + + const XHRRequestSpy = class XHRRequestSpy extends XHRRequest { + static createRequest(...args) { + usedXHR = true; + return super.createRequest(...args); + } + }; + + const rest = new BaseRest(ablyClientOptions(), { FetchRequest, XHRRequest: XHRRequestSpy }); + await rest.time(); + + expect(usedXHR).to.be.true; + }); + }); + }); }); From 61cf994604a73f36bc0e9ff56102e26f7aa4574a Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Fri, 27 Oct 2023 15:11:04 -0300 Subject: [PATCH 7/8] Extract RealtimeChannel MessageFilter-related code to new class In preparation for #1397 (making MessageFilter functionality tree-shakable). --- .../lib/client/filteredsubscriptions.ts | 112 ++++++++++++++++++ src/common/lib/client/realtimechannel.ts | 108 +---------------- 2 files changed, 117 insertions(+), 103 deletions(-) create mode 100644 src/common/lib/client/filteredsubscriptions.ts diff --git a/src/common/lib/client/filteredsubscriptions.ts b/src/common/lib/client/filteredsubscriptions.ts new file mode 100644 index 0000000000..1588fa4962 --- /dev/null +++ b/src/common/lib/client/filteredsubscriptions.ts @@ -0,0 +1,112 @@ +import * as API from '../../../../ably'; +import RealtimeChannel from './realtimechannel'; +import Message from '../types/message'; + +export class FilteredSubscriptions { + static subscribeFilter( + channel: RealtimeChannel, + filter: API.Types.MessageFilter, + listener: API.Types.messageCallback + ) { + const filteredListener = (m: Message) => { + const mapping: { [key in keyof API.Types.MessageFilter]: any } = { + name: m.name, + refTimeserial: m.extras?.ref?.timeserial, + refType: m.extras?.ref?.type, + isRef: !!m.extras?.ref?.timeserial, + clientId: m.clientId, + }; + // Check if any values are defined in the filter and if they match the value in the message object + if ( + Object.entries(filter).find(([key, value]) => + value !== undefined ? mapping[key as keyof API.Types.MessageFilter] !== value : false + ) + ) { + return; + } + listener(m); + }; + this.addFilteredSubscription(channel, filter, listener, filteredListener); + channel.subscriptions.on(filteredListener); + } + + // Adds a new filtered subscription + static addFilteredSubscription( + channel: RealtimeChannel, + filter: API.Types.MessageFilter, + realListener: API.Types.messageCallback, + filteredListener: API.Types.messageCallback + ) { + if (!channel.filteredSubscriptions) { + channel.filteredSubscriptions = new Map< + API.Types.messageCallback, + Map[]> + >(); + } + if (channel.filteredSubscriptions.has(realListener)) { + const realListenerMap = channel.filteredSubscriptions.get(realListener) as Map< + API.Types.MessageFilter, + API.Types.messageCallback[] + >; + // Add the filtered listener to the map, or append to the array if this filter has already been used + realListenerMap.set(filter, realListenerMap?.get(filter)?.concat(filteredListener) || [filteredListener]); + } else { + channel.filteredSubscriptions.set( + realListener, + new Map[]>([[filter, [filteredListener]]]) + ); + } + } + + static getAndDeleteFilteredSubscriptions( + channel: RealtimeChannel, + filter: API.Types.MessageFilter | undefined, + realListener: API.Types.messageCallback | undefined + ): API.Types.messageCallback[] { + // No filtered subscriptions map means there has been no filtered subscriptions yet, so return nothing + if (!channel.filteredSubscriptions) { + return []; + } + // Only a filter is passed in with no specific listener + if (!realListener && filter) { + // Return each listener which is attached to the specified filter object + return Array.from(channel.filteredSubscriptions.entries()) + .map(([key, filterMaps]) => { + // Get (then delete) the maps matching this filter + let listenerMaps = filterMaps.get(filter); + filterMaps.delete(filter); + // Clear the parent if nothing is left + if (filterMaps.size === 0) { + channel.filteredSubscriptions?.delete(key); + } + return listenerMaps; + }) + .reduce( + (prev, cur) => (cur ? (prev as API.Types.messageCallback[]).concat(...cur) : prev), + [] + ) as API.Types.messageCallback[]; + } + + // No subscriptions for this listener + if (!realListener || !channel.filteredSubscriptions.has(realListener)) { + return []; + } + const realListenerMap = channel.filteredSubscriptions.get(realListener) as Map< + API.Types.MessageFilter, + API.Types.messageCallback[] + >; + // If no filter is specified return all listeners using that function + if (!filter) { + // array.flat is not available unless we support es2019 or higher + const listeners = Array.from(realListenerMap.values()).reduce((prev, cur) => prev.concat(...cur), []); + // remove the listener from the map + channel.filteredSubscriptions.delete(realListener); + return listeners; + } + + let listeners = realListenerMap.get(filter); + realListenerMap.delete(filter); + + return listeners || []; + } +} diff --git a/src/common/lib/client/realtimechannel.ts b/src/common/lib/client/realtimechannel.ts index ee403f7bc5..7080066f05 100644 --- a/src/common/lib/client/realtimechannel.ts +++ b/src/common/lib/client/realtimechannel.ts @@ -14,6 +14,7 @@ import ConnectionManager from '../transport/connectionmanager'; import ConnectionStateChange from './connectionstatechange'; import { ErrCallback, PaginatedResultCallback, StandardCallback } from '../../types/utils'; import BaseRealtime from './baserealtime'; +import { FilteredSubscriptions } from './filteredsubscriptions'; interface RealtimeHistoryParams { start?: number; @@ -438,7 +439,7 @@ class RealtimeChannel extends Channel { // Filtered if (event && typeof event === 'object' && !Array.isArray(event)) { - this._subscribeFilter(event, listener); + FilteredSubscriptions.subscribeFilter(this, event, listener); } else { this.subscriptions.on(event, listener); } @@ -446,113 +447,14 @@ class RealtimeChannel extends Channel { return this.attach(callback || noop); } - _subscribeFilter(filter: API.Types.MessageFilter, listener: API.Types.messageCallback) { - const filteredListener = (m: Message) => { - const mapping: { [key in keyof API.Types.MessageFilter]: any } = { - name: m.name, - refTimeserial: m.extras?.ref?.timeserial, - refType: m.extras?.ref?.type, - isRef: !!m.extras?.ref?.timeserial, - clientId: m.clientId, - }; - // Check if any values are defined in the filter and if they match the value in the message object - if ( - Object.entries(filter).find(([key, value]) => - value !== undefined ? mapping[key as keyof API.Types.MessageFilter] !== value : false - ) - ) { - return; - } - listener(m); - }; - this._addFilteredSubscription(filter, listener, filteredListener); - this.subscriptions.on(filteredListener); - } - - // Adds a new filtered subscription - _addFilteredSubscription( - filter: API.Types.MessageFilter, - realListener: API.Types.messageCallback, - filteredListener: API.Types.messageCallback - ) { - if (!this.filteredSubscriptions) { - this.filteredSubscriptions = new Map< - API.Types.messageCallback, - Map[]> - >(); - } - if (this.filteredSubscriptions.has(realListener)) { - const realListenerMap = this.filteredSubscriptions.get(realListener) as Map< - API.Types.MessageFilter, - API.Types.messageCallback[] - >; - // Add the filtered listener to the map, or append to the array if this filter has already been used - realListenerMap.set(filter, realListenerMap?.get(filter)?.concat(filteredListener) || [filteredListener]); - } else { - this.filteredSubscriptions.set( - realListener, - new Map[]>([[filter, [filteredListener]]]) - ); - } - } - - _getAndDeleteFilteredSubscriptions( - filter: API.Types.MessageFilter | undefined, - realListener: API.Types.messageCallback | undefined - ): API.Types.messageCallback[] { - // No filtered subscriptions map means there has been no filtered subscriptions yet, so return nothing - if (!this.filteredSubscriptions) { - return []; - } - // Only a filter is passed in with no specific listener - if (!realListener && filter) { - // Return each listener which is attached to the specified filter object - return Array.from(this.filteredSubscriptions.entries()) - .map(([key, filterMaps]) => { - // Get (then delete) the maps matching this filter - let listenerMaps = filterMaps.get(filter); - filterMaps.delete(filter); - // Clear the parent if nothing is left - if (filterMaps.size === 0) { - this.filteredSubscriptions?.delete(key); - } - return listenerMaps; - }) - .reduce( - (prev, cur) => (cur ? (prev as API.Types.messageCallback[]).concat(...cur) : prev), - [] - ) as API.Types.messageCallback[]; - } - - // No subscriptions for this listener - if (!realListener || !this.filteredSubscriptions.has(realListener)) { - return []; - } - const realListenerMap = this.filteredSubscriptions.get(realListener) as Map< - API.Types.MessageFilter, - API.Types.messageCallback[] - >; - // If no filter is specified return all listeners using that function - if (!filter) { - // array.flat is not available unless we support es2019 or higher - const listeners = Array.from(realListenerMap.values()).reduce((prev, cur) => prev.concat(...cur), []); - // remove the listener from the map - this.filteredSubscriptions.delete(realListener); - return listeners; - } - - let listeners = realListenerMap.get(filter); - realListenerMap.delete(filter); - - return listeners || []; - } - unsubscribe(...args: unknown[] /* [event], listener */): void { const [event, listener] = RealtimeChannel.processListenerArgs(args); // If we either have a filtered listener, a filter or both we need to do additional processing to find the original function(s) if ((typeof event === 'object' && !listener) || this.filteredSubscriptions?.has(listener)) { - this._getAndDeleteFilteredSubscriptions(event, listener).forEach((l) => this.subscriptions.off(l)); + FilteredSubscriptions.getAndDeleteFilteredSubscriptions(this, event, listener).forEach((l) => + this.subscriptions.off(l) + ); return; } From 6a9647212a4e085b85650e40c4ad382c21a18452 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Fri, 27 Oct 2023 15:24:48 -0300 Subject: [PATCH 8/8] Make subscription filtering tree-shakable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We expose a MessageIteractions module which allows users to pass a MessageFilter object to RealtimeChannel’s `subscribe` and `unsubscribe`. Resolves #1397. --- scripts/moduleReport.js | 1 + src/common/lib/client/baseclient.ts | 10 +++ src/common/lib/client/defaultrealtime.ts | 2 + src/common/lib/client/modulesmap.ts | 2 + src/common/lib/client/realtimechannel.ts | 9 ++- src/platform/web/modules.ts | 1 + test/browser/modules.test.js | 89 ++++++++++++++++++++++++ 7 files changed, 109 insertions(+), 5 deletions(-) diff --git a/scripts/moduleReport.js b/scripts/moduleReport.js index f82a58ea8a..dd1ec550f5 100644 --- a/scripts/moduleReport.js +++ b/scripts/moduleReport.js @@ -11,6 +11,7 @@ const moduleNames = [ 'WebSocketTransport', 'XHRRequest', 'FetchRequest', + 'MessageInteractions', ]; // List of all free-standing functions exported by the library along with the diff --git a/src/common/lib/client/baseclient.ts b/src/common/lib/client/baseclient.ts index ddc726b4d5..482075fa23 100644 --- a/src/common/lib/client/baseclient.ts +++ b/src/common/lib/client/baseclient.ts @@ -16,6 +16,7 @@ import { IUntypedCryptoStatic } from 'common/types/ICryptoStatic'; import { throwMissingModuleError } from '../util/utils'; import { MsgPack } from 'common/types/msgpack'; import { HTTPRequestImplementations } from 'platform/web/lib/http/http'; +import { FilteredSubscriptions } from './filteredsubscriptions'; type BatchResult = API.Types.BatchResult; type BatchPublishSpec = API.Types.BatchPublishSpec; @@ -44,6 +45,7 @@ class BaseClient { readonly _MsgPack: MsgPack | null; // Extra HTTP request implementations available to this client, in addition to those in web’s Http.bundledRequestImplementations readonly _additionalHTTPRequestImplementations: HTTPRequestImplementations; + private readonly __FilteredSubscriptions: typeof FilteredSubscriptions | null; constructor(options: ClientOptions | string, modules: ModulesMap) { this._additionalHTTPRequestImplementations = modules; @@ -98,6 +100,7 @@ class BaseClient { this._rest = modules.Rest ? new modules.Rest(this) : null; this._Crypto = modules.Crypto ?? null; + this.__FilteredSubscriptions = modules.MessageInteractions ?? null; } private get rest(): Rest { @@ -107,6 +110,13 @@ class BaseClient { return this._rest; } + get _FilteredSubscriptions(): typeof FilteredSubscriptions { + if (!this.__FilteredSubscriptions) { + throwMissingModuleError('MessageInteractions'); + } + return this.__FilteredSubscriptions; + } + get channels() { return this.rest.channels; } diff --git a/src/common/lib/client/defaultrealtime.ts b/src/common/lib/client/defaultrealtime.ts index 2204ab71ac..1dd3d402dd 100644 --- a/src/common/lib/client/defaultrealtime.ts +++ b/src/common/lib/client/defaultrealtime.ts @@ -10,6 +10,7 @@ import { MsgPack } from 'common/types/msgpack'; import RealtimePresence from './realtimepresence'; import { DefaultPresenceMessage } from '../types/defaultpresencemessage'; import initialiseWebSocketTransport from '../transport/websockettransport'; +import { FilteredSubscriptions } from './filteredsubscriptions'; /** `DefaultRealtime` is the class that the non tree-shakable version of the SDK exports as `Realtime`. It ensures that this version of the SDK includes all of the functionality which is optionally available in the tree-shakable version. @@ -27,6 +28,7 @@ export class DefaultRealtime extends BaseRealtime { MsgPack, RealtimePresence, WebSocketTransport: initialiseWebSocketTransport, + MessageInteractions: FilteredSubscriptions, }); } diff --git a/src/common/lib/client/modulesmap.ts b/src/common/lib/client/modulesmap.ts index dff32a69e4..dab5c8b718 100644 --- a/src/common/lib/client/modulesmap.ts +++ b/src/common/lib/client/modulesmap.ts @@ -5,6 +5,7 @@ import RealtimePresence from './realtimepresence'; import { TransportInitialiser } from '../transport/connectionmanager'; import XHRRequest from 'platform/web/lib/http/request/xhrrequest'; import fetchRequest from 'platform/web/lib/http/request/fetchrequest'; +import { FilteredSubscriptions } from './filteredsubscriptions'; export interface ModulesMap { Rest?: typeof Rest; @@ -16,6 +17,7 @@ export interface ModulesMap { XHRStreaming?: TransportInitialiser; XHRRequest?: typeof XHRRequest; FetchRequest?: typeof fetchRequest; + MessageInteractions?: typeof FilteredSubscriptions; } export const allCommonModules: ModulesMap = { Rest }; diff --git a/src/common/lib/client/realtimechannel.ts b/src/common/lib/client/realtimechannel.ts index 7080066f05..898377dded 100644 --- a/src/common/lib/client/realtimechannel.ts +++ b/src/common/lib/client/realtimechannel.ts @@ -14,7 +14,6 @@ import ConnectionManager from '../transport/connectionmanager'; import ConnectionStateChange from './connectionstatechange'; import { ErrCallback, PaginatedResultCallback, StandardCallback } from '../../types/utils'; import BaseRealtime from './baserealtime'; -import { FilteredSubscriptions } from './filteredsubscriptions'; interface RealtimeHistoryParams { start?: number; @@ -439,7 +438,7 @@ class RealtimeChannel extends Channel { // Filtered if (event && typeof event === 'object' && !Array.isArray(event)) { - FilteredSubscriptions.subscribeFilter(this, event, listener); + this.client._FilteredSubscriptions.subscribeFilter(this, event, listener); } else { this.subscriptions.on(event, listener); } @@ -452,9 +451,9 @@ class RealtimeChannel extends Channel { // If we either have a filtered listener, a filter or both we need to do additional processing to find the original function(s) if ((typeof event === 'object' && !listener) || this.filteredSubscriptions?.has(listener)) { - FilteredSubscriptions.getAndDeleteFilteredSubscriptions(this, event, listener).forEach((l) => - this.subscriptions.off(l) - ); + this.client._FilteredSubscriptions + .getAndDeleteFilteredSubscriptions(this, event, listener) + .forEach((l) => this.subscriptions.off(l)); return; } diff --git a/src/platform/web/modules.ts b/src/platform/web/modules.ts index e12ede52e7..0465405a90 100644 --- a/src/platform/web/modules.ts +++ b/src/platform/web/modules.ts @@ -50,4 +50,5 @@ export * from './modules/realtimepresence'; export * from './modules/transports'; export * from './modules/http'; export { Rest } from '../../common/lib/client/rest'; +export { FilteredSubscriptions as MessageInteractions } from '../../common/lib/client/filteredsubscriptions'; export { BaseRest, BaseRealtime, ErrorInfo }; diff --git a/test/browser/modules.test.js b/test/browser/modules.test.js index d73b76624e..bf56cf4fda 100644 --- a/test/browser/modules.test.js +++ b/test/browser/modules.test.js @@ -19,6 +19,7 @@ import { WebSocketTransport, FetchRequest, XHRRequest, + MessageInteractions, } from '../../build/modules/index.js'; describe('browser/modules', function () { @@ -504,4 +505,92 @@ describe('browser/modules', function () { }); }); }); + + describe('MessageInteractions', () => { + describe('BaseRealtime', () => { + describe('without MessageInteractions', () => { + it('is able to subscribe to and unsubscribe from channel events, as long as a MessageFilter isn’t passed', async () => { + const realtime = new BaseRealtime(ablyClientOptions(), { WebSocketTransport, FetchRequest }); + const channel = realtime.channels.get('channel'); + await channel.attach(); + + const subscribeReceivedMessagePromise = new Promise((resolve) => channel.subscribe(resolve)); + + await channel.publish('message', 'body'); + + const subscribeReceivedMessage = await subscribeReceivedMessagePromise; + expect(subscribeReceivedMessage.data).to.equal('body'); + }); + + it('throws an error when attempting to subscribe to channel events using a MessageFilter', async () => { + const realtime = new BaseRealtime(ablyClientOptions(), { WebSocketTransport, FetchRequest }); + const channel = realtime.channels.get('channel'); + + let thrownError = null; + try { + await channel.subscribe({ clientId: 'someClientId' }, () => {}); + } catch (error) { + thrownError = error; + } + + expect(thrownError).not.to.be.null; + expect(thrownError.message).to.equal('MessageInteractions module not provided'); + }); + }); + + describe('with MessageInteractions', () => { + it('can take a MessageFilter argument when subscribing to and unsubscribing from channel events', async () => { + const realtime = new BaseRealtime(ablyClientOptions(), { + WebSocketTransport, + FetchRequest, + MessageInteractions, + }); + const channel = realtime.channels.get('channel'); + + await channel.attach(); + + // Test `subscribe` with a filter: send two messages with different clientIds, and check that unfiltered subscription receives both messages but clientId-filtered subscription only receives the matching one. + const messageFilter = { clientId: 'someClientId' }; // note that `unsubscribe` compares filter by reference, I found that a bit surprising + + const filteredSubscriptionReceivedMessages = []; + channel.subscribe(messageFilter, (message) => { + filteredSubscriptionReceivedMessages.push(message); + }); + + const unfilteredSubscriptionReceivedFirstTwoMessagesPromise = new Promise((resolve) => { + const receivedMessages = []; + channel.subscribe(function listener(message) { + receivedMessages.push(message); + if (receivedMessages.length === 2) { + channel.unsubscribe(listener); + resolve(); + } + }); + }); + + await channel.publish(await decodeMessage({ clientId: 'someClientId' })); + await channel.publish(await decodeMessage({ clientId: 'someOtherClientId' })); + await unfilteredSubscriptionReceivedFirstTwoMessagesPromise; + + expect(filteredSubscriptionReceivedMessages.length).to.equal(1); + expect(filteredSubscriptionReceivedMessages[0].clientId).to.equal('someClientId'); + + // Test `unsubscribe` with a filter: call `unsubscribe` with the clientId filter, publish a message matching the filter, check that only the unfiltered listener recieves it + channel.unsubscribe(messageFilter); + + const unfilteredSubscriptionReceivedNextMessagePromise = new Promise((resolve) => { + channel.subscribe(function listener() { + channel.unsubscribe(listener); + resolve(); + }); + }); + + await channel.publish(await decodeMessage({ clientId: 'someClientId' })); + await unfilteredSubscriptionReceivedNextMessagePromise; + + expect(filteredSubscriptionReceivedMessages.length).to./* (still) */ equal(1); + }); + }); + }); + }); });