From f320e0aa42a8aa44703445c665cf080d220b7d68 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 17 Aug 2023 17:01:11 -0300 Subject: [PATCH] Make MessagePack functionality tree-shakable We move the MessagePack functionality into a tree-shakable MsgPack module. Resolves #1375. --- scripts/moduleReport.js | 2 +- src/common/lib/client/baseclient.ts | 5 +- src/common/lib/client/defaultrealtime.ts | 10 +++- src/common/lib/client/defaultrest.ts | 14 ++++- src/common/lib/client/modulesmap.ts | 2 + src/common/types/IPlatformConfig.d.ts | 3 - src/platform/nativescript/config.js | 2 - src/platform/nativescript/index.ts | 1 + src/platform/nodejs/config.ts | 1 - src/platform/nodejs/index.ts | 2 + src/platform/react-native/config.ts | 2 - src/platform/react-native/index.ts | 1 + src/platform/web/config.ts | 2 - src/platform/web/index.ts | 1 + src/platform/web/modules.ts | 1 + src/platform/web/modules/msgpack.ts | 1 + test/browser/modules.test.js | 71 ++++++++++++++++++++++++ 17 files changed, 106 insertions(+), 15 deletions(-) create mode 100644 src/platform/web/modules/msgpack.ts diff --git a/scripts/moduleReport.js b/scripts/moduleReport.js index 1937afc34d..577371359f 100644 --- a/scripts/moduleReport.js +++ b/scripts/moduleReport.js @@ -1,7 +1,7 @@ const esbuild = require('esbuild'); // List of all modules accepted in ModulesMap -const moduleNames = ['Rest', 'Crypto']; +const moduleNames = ['Rest', 'Crypto', 'MsgPack']; // List of all free-standing functions exported by the library along with the // ModulesMap entries that we expect them to transitively import diff --git a/src/common/lib/client/baseclient.ts b/src/common/lib/client/baseclient.ts index 876fb71904..71582f5996 100644 --- a/src/common/lib/client/baseclient.ts +++ b/src/common/lib/client/baseclient.ts @@ -31,11 +31,11 @@ class BaseClient { private readonly _rest: Rest | null; readonly _Crypto: IUntypedCryptoStatic | null; - private readonly __MsgPack: MsgPack | null = Platform.Config.msgpack; + private readonly __MsgPack: MsgPack | null; get _MsgPack(): MsgPack { if (!this.__MsgPack) { - throw new ErrorInfo('MsgPack not available', 400, 40000); + throwMissingModuleError('MsgPack'); } return this.__MsgPack; } @@ -55,6 +55,7 @@ class BaseClient { 'initialized with clientOptions ' + Platform.Config.inspect(options) ); + this.__MsgPack = modules.MsgPack ?? null; const normalOptions = (this.options = Defaults.normaliseOptions(optionsObj, this.__MsgPack)); /* process options */ diff --git a/src/common/lib/client/defaultrealtime.ts b/src/common/lib/client/defaultrealtime.ts index e8b66f878e..8a905b449d 100644 --- a/src/common/lib/client/defaultrealtime.ts +++ b/src/common/lib/client/defaultrealtime.ts @@ -6,13 +6,19 @@ import ConnectionManager from '../transport/connectionmanager'; import ProtocolMessage from '../types/protocolmessage'; import Platform from 'common/platform'; import { DefaultMessage } from '../types/defaultmessage'; +import { MsgPack } from 'common/types/msgpack'; /** `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. */ export class DefaultRealtime extends BaseRealtime { constructor(options: ClientOptions) { - super(options, { ...allCommonModules, Crypto: DefaultRealtime.Crypto ?? undefined }); + const MsgPack = DefaultRealtime._MsgPack; + if (!MsgPack) { + throw new Error('Expected DefaultRealtime._MsgPack to have been set'); + } + + super(options, { ...allCommonModules, Crypto: DefaultRealtime.Crypto ?? undefined, MsgPack }); } static Utils = Utils; @@ -32,4 +38,6 @@ export class DefaultRealtime extends BaseRealtime { } static Message = DefaultMessage; + + static _MsgPack: MsgPack | null = null; } diff --git a/src/common/lib/client/defaultrest.ts b/src/common/lib/client/defaultrest.ts index 1b7df607a6..e70a9eecf6 100644 --- a/src/common/lib/client/defaultrest.ts +++ b/src/common/lib/client/defaultrest.ts @@ -3,13 +3,23 @@ import ClientOptions from '../../types/ClientOptions'; import { allCommonModules } from './modulesmap'; import Platform from 'common/platform'; import { DefaultMessage } from '../types/defaultmessage'; +import { MsgPack } from 'common/types/msgpack'; /** `DefaultRest` is the class that the non tree-shakable version of the SDK exports as `Rest`. It ensures that this version of the SDK includes all of the functionality which is optionally available in the tree-shakable version. */ export class DefaultRest extends BaseRest { constructor(options: ClientOptions | string) { - super(options, { ...allCommonModules, Crypto: DefaultRest.Crypto ?? undefined }); + const MsgPack = DefaultRest._MsgPack; + if (!MsgPack) { + throw new Error('Expected DefaultRest._MsgPack to have been set'); + } + + super(options, { + ...allCommonModules, + Crypto: DefaultRest.Crypto ?? undefined, + MsgPack: DefaultRest._MsgPack ?? undefined, + }); } private static _Crypto: typeof Platform.Crypto = null; @@ -25,4 +35,6 @@ export class DefaultRest extends BaseRest { } static Message = DefaultMessage; + + static _MsgPack: MsgPack | null = null; } diff --git a/src/common/lib/client/modulesmap.ts b/src/common/lib/client/modulesmap.ts index 4133bcc3a3..12ac9bc7f6 100644 --- a/src/common/lib/client/modulesmap.ts +++ b/src/common/lib/client/modulesmap.ts @@ -1,9 +1,11 @@ import { Rest } from './rest'; import { IUntypedCryptoStatic } from '../../types/ICryptoStatic'; +import { MsgPack } from 'common/types/msgpack'; export interface ModulesMap { Rest?: typeof Rest; Crypto?: IUntypedCryptoStatic; + MsgPack?: MsgPack; } export const allCommonModules: ModulesMap = { Rest }; diff --git a/src/common/types/IPlatformConfig.d.ts b/src/common/types/IPlatformConfig.d.ts index 4a83e20a39..9d678f23bb 100644 --- a/src/common/types/IPlatformConfig.d.ts +++ b/src/common/types/IPlatformConfig.d.ts @@ -1,12 +1,9 @@ -import { MsgPack } from './msgpack'; - export interface IPlatformConfig { agent: string; logTimestamps: boolean; binaryType: BinaryType; WebSocket: typeof WebSocket | typeof import('ws'); useProtocolHeartbeats: boolean; - msgpack: MsgPack; supportsBinary: boolean; preferBinary: boolean; nextTick: process.nextTick; diff --git a/src/platform/nativescript/config.js b/src/platform/nativescript/config.js index 31090e0240..ccdece4dd5 100644 --- a/src/platform/nativescript/config.js +++ b/src/platform/nativescript/config.js @@ -1,5 +1,4 @@ /* eslint-disable no-undef */ -import msgpack from '../web/lib/util/msgpack'; require('nativescript-websockets'); var randomBytes; @@ -28,7 +27,6 @@ var Config = { allowComet: true, streamingSupported: false, useProtocolHeartbeats: true, - msgpack: msgpack, supportsBinary: typeof TextDecoder !== 'undefined' && TextDecoder, preferBinary: false, ArrayBuffer: ArrayBuffer, diff --git a/src/platform/nativescript/index.ts b/src/platform/nativescript/index.ts index d6f5e133cf..7fc4a1a65a 100644 --- a/src/platform/nativescript/index.ts +++ b/src/platform/nativescript/index.ts @@ -30,6 +30,7 @@ Platform.WebStorage = WebStorage; for (const clientClass of [DefaultRest, DefaultRealtime]) { clientClass.Crypto = Crypto; + clientClass._MsgPack = msgpack; } Logger.initLogHandlers(); diff --git a/src/platform/nodejs/config.ts b/src/platform/nodejs/config.ts index 716be3b12a..4947994239 100644 --- a/src/platform/nodejs/config.ts +++ b/src/platform/nodejs/config.ts @@ -10,7 +10,6 @@ const Config: IPlatformConfig = { binaryType: 'nodebuffer' as BinaryType, WebSocket, useProtocolHeartbeats: false, - msgpack: require('@ably/msgpack-js'), supportsBinary: true, preferBinary: true, nextTick: process.nextTick, diff --git a/src/platform/nodejs/index.ts b/src/platform/nodejs/index.ts index 796596f083..6c1756037d 100644 --- a/src/platform/nodejs/index.ts +++ b/src/platform/nodejs/index.ts @@ -14,6 +14,7 @@ import Transports from './lib/transport'; import Logger from '../../common/lib/util/logger'; import { getDefaults } from '../../common/lib/util/defaults'; import PlatformDefaults from './lib/util/defaults'; +import msgpack = require('@ably/msgpack-js'); const Crypto = createCryptoClass(BufferUtils); @@ -26,6 +27,7 @@ Platform.WebStorage = null; for (const clientClass of [DefaultRest, DefaultRealtime]) { clientClass.Crypto = Crypto; + clientClass._MsgPack = msgpack; } Logger.initLogHandlers(); diff --git a/src/platform/react-native/config.ts b/src/platform/react-native/config.ts index f6e6f29a2e..6a2f0107b8 100644 --- a/src/platform/react-native/config.ts +++ b/src/platform/react-native/config.ts @@ -1,4 +1,3 @@ -import msgpack from '../web/lib/util/msgpack'; import { IPlatformConfig } from '../../common/types/IPlatformConfig'; import BufferUtils from '../web/lib/util/bufferutils'; @@ -13,7 +12,6 @@ export default function (bufferUtils: typeof BufferUtils): IPlatformConfig { allowComet: true, streamingSupported: true, useProtocolHeartbeats: true, - msgpack: msgpack, supportsBinary: !!(typeof TextDecoder !== 'undefined' && TextDecoder), preferBinary: false, ArrayBuffer: typeof ArrayBuffer !== 'undefined' && ArrayBuffer, diff --git a/src/platform/react-native/index.ts b/src/platform/react-native/index.ts index 88158e1bb8..b02dc5d50a 100644 --- a/src/platform/react-native/index.ts +++ b/src/platform/react-native/index.ts @@ -30,6 +30,7 @@ Platform.WebStorage = WebStorage; for (const clientClass of [DefaultRest, DefaultRealtime]) { clientClass.Crypto = Crypto; + clientClass._MsgPack = msgpack; } Logger.initLogHandlers(); diff --git a/src/platform/web/config.ts b/src/platform/web/config.ts index 87536361ad..f3aac12c93 100644 --- a/src/platform/web/config.ts +++ b/src/platform/web/config.ts @@ -1,4 +1,3 @@ -import msgpack from './lib/util/msgpack'; import { IPlatformConfig } from '../../common/types/IPlatformConfig'; import * as Utils from 'common/lib/util/utils'; @@ -37,7 +36,6 @@ const Config: IPlatformConfig = { allowComet: allowComet(), streamingSupported: true, useProtocolHeartbeats: true, - msgpack: msgpack, supportsBinary: !!globalObject.TextDecoder, preferBinary: false, ArrayBuffer: globalObject.ArrayBuffer, diff --git a/src/platform/web/index.ts b/src/platform/web/index.ts index e1c1831c39..d3c3184420 100644 --- a/src/platform/web/index.ts +++ b/src/platform/web/index.ts @@ -28,6 +28,7 @@ Platform.WebStorage = WebStorage; for (const clientClass of [DefaultRest, DefaultRealtime]) { clientClass.Crypto = Crypto; + clientClass._MsgPack = msgpack; } Logger.initLogHandlers(); diff --git a/src/platform/web/modules.ts b/src/platform/web/modules.ts index 06d4d692de..bb701d1fdd 100644 --- a/src/platform/web/modules.ts +++ b/src/platform/web/modules.ts @@ -40,5 +40,6 @@ if (Platform.Config.noUpgrade) { export * from './modules/crypto'; export * from './modules/message'; +export * from './modules/msgpack'; export { Rest } from '../../common/lib/client/rest'; export { BaseRest, BaseRealtime }; diff --git a/src/platform/web/modules/msgpack.ts b/src/platform/web/modules/msgpack.ts new file mode 100644 index 0000000000..698b09e34c --- /dev/null +++ b/src/platform/web/modules/msgpack.ts @@ -0,0 +1 @@ +export { default as MsgPack } from '../lib/util/msgpack'; diff --git a/test/browser/modules.test.js b/test/browser/modules.test.js index 79033e907b..5cb2da7168 100644 --- a/test/browser/modules.test.js +++ b/test/browser/modules.test.js @@ -9,6 +9,7 @@ import { decodeMessages, decodeEncryptedMessages, Crypto, + MsgPack, } from '../../build/modules/index.js'; describe('browser/modules', function () { @@ -232,4 +233,74 @@ describe('browser/modules', function () { } }); }); + + describe('MsgPack', () => { + async function testRestUsesContentType(rest, expectedContentType) { + 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) => { + if (!(method == 'post' && path == `/channels/${channelName}/messages`)) { + return; + } + resolve(headers['content-type']); + callback(null); + }; + }); + + await channel.publish('message', 'body'); + + const contentTypeUsedForPublish = await contentTypeUsedForPublishPromise; + expect(contentTypeUsedForPublish).to.equal(expectedContentType); + } + + async function testRealtimeUsesFormat(realtime, expectedFormat) { + const formatUsedForConnectionPromise = new Promise((resolve, reject) => { + realtime.connection.connectionManager.connectImpl = (transportParams) => { + resolve(transportParams.format); + }; + }); + realtime.connect(); + + const formatUsedForConnection = await formatUsedForConnectionPromise; + expect(formatUsedForConnection).to.equal(expectedFormat); + } + + // TODO once https://github.com/ably/ably-js/issues/1424 is fixed, this should also test the case where the useBinaryProtocol option is not specified + describe('with useBinaryProtocol client option', () => { + describe('without MsgPack', () => { + describe('BaseRest', () => { + it('uses JSON', async () => { + const client = new BaseRest(ablyClientOptions({ useBinaryProtocol: true }), {}); + await testRestUsesContentType(client, 'application/json'); + }); + }); + + describe('BaseRealtime', () => { + it('uses JSON', async () => { + const client = new BaseRealtime(ablyClientOptions({ useBinaryProtocol: true, autoConnect: false }), {}); + await testRealtimeUsesFormat(client, 'json'); + }); + }); + }); + + describe('with MsgPack', () => { + describe('BaseRest', () => { + it('uses MessagePack', async () => { + const client = new BaseRest(ablyClientOptions({ useBinaryProtocol: true }), { MsgPack }); + await testRestUsesContentType(client, 'application/x-msgpack'); + }); + }); + + describe('BaseRealtime', () => { + it('uses MessagePack', async () => { + const client = new BaseRealtime(ablyClientOptions({ useBinaryProtocol: true, autoConnect: false }), { + MsgPack, + }); + await testRealtimeUsesFormat(client, 'msgpack'); + }); + }); + }); + }); + }); });