From 673aa5b3319c130a43701110844f983f1a8edfe9 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Mon, 18 Mar 2024 18:01:35 -0300 Subject: [PATCH] Improve errors for incorrect constructor args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes the regression that I introduced in e8c3012, which stopped the user from getting a useful error message when not passing any args to the constructor. I’ve also introduced a wider range of error messages to give the user useful guidance depending on which variant of the library they’re using. Resolves #1700. --- src/common/lib/client/baseclient.ts | 6 ---- src/common/lib/client/baserealtime.ts | 12 +++++-- src/common/lib/client/baserest.ts | 12 +++++-- src/common/lib/client/defaultrealtime.ts | 5 +-- src/common/lib/client/defaultrest.ts | 5 +-- src/common/lib/util/defaults.ts | 44 +++++++++++++++++++++--- test/browser/modular.test.js | 34 ++++++++++++++++++ test/realtime/api.test.js | 6 ++++ test/rest/api.test.js | 6 ++++ 9 files changed, 112 insertions(+), 18 deletions(-) diff --git a/src/common/lib/client/baseclient.ts b/src/common/lib/client/baseclient.ts index 5cfed84b70..f4351902a2 100644 --- a/src/common/lib/client/baseclient.ts +++ b/src/common/lib/client/baseclient.ts @@ -48,12 +48,6 @@ class BaseClient { constructor(options: ClientOptions) { this._additionalHTTPRequestImplementations = options.plugins ?? null; - if (!options) { - const msg = 'no options provided'; - Logger.logAction(Logger.LOG_ERROR, 'BaseClient()', msg); - throw new Error(msg); - } - Logger.setLog(options.logLevel, options.logHandler); Logger.logAction( Logger.LOG_MICRO, diff --git a/src/common/lib/client/baserealtime.ts b/src/common/lib/client/baserealtime.ts index 359ad7bc6f..39fee93e21 100644 --- a/src/common/lib/client/baserealtime.ts +++ b/src/common/lib/client/baserealtime.ts @@ -24,8 +24,16 @@ class BaseRealtime extends BaseClient { _channels: any; connection: Connection; - constructor(options: ClientOptions | string) { - super(Defaults.objectifyOptions(options)); + /* + * The public typings declare that this only accepts an object, but since we want to emit a good error message in the case where a non-TypeScript user does one of these things: + * + * 1. passes a string (which is quite likely if they’re e.g. migrating from the default variant to the modular variant) + * 2. passes no argument at all + * + * tell the compiler that these cases are possible so that it forces us to handle them. + */ + constructor(options?: ClientOptions | string) { + super(Defaults.objectifyOptions(options, false, 'BaseRealtime')); Logger.logAction(Logger.LOG_MINOR, 'Realtime()', ''); this._additionalTransportImplementations = BaseRealtime.transportImplementationsFromPlugins(this.options.plugins); this._RealtimePresence = this.options.plugins?.RealtimePresence ?? null; diff --git a/src/common/lib/client/baserest.ts b/src/common/lib/client/baserest.ts index 5c823755eb..46137a08cb 100644 --- a/src/common/lib/client/baserest.ts +++ b/src/common/lib/client/baserest.ts @@ -9,7 +9,15 @@ import Defaults from '../util/defaults'; It always includes the `Rest` plugin. */ export class BaseRest extends BaseClient { - constructor(options: ClientOptions | string) { - super(Defaults.objectifyOptions(options, { Rest })); + /* + * The public typings declare that this only accepts an object, but since we want to emit a good error message in the case where a non-TypeScript user does one of these things: + * + * 1. passes a string (which is quite likely if they’re e.g. migrating from the default variant to the modular variant) + * 2. passes no argument at all + * + * tell the compiler that these cases are possible so that it forces us to handle them. + */ + constructor(options?: ClientOptions | string) { + super(Defaults.objectifyOptions(options, false, 'BaseRest', { Rest })); } } diff --git a/src/common/lib/client/defaultrealtime.ts b/src/common/lib/client/defaultrealtime.ts index d1fa3f4ce2..a966a69d32 100644 --- a/src/common/lib/client/defaultrealtime.ts +++ b/src/common/lib/client/defaultrealtime.ts @@ -22,14 +22,15 @@ import Defaults from '../util/defaults'; `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 | string) { + // The public typings declare that this requires an argument to be passed, but since we want to emit a good error message in the case where a non-TypeScript user does not pass an argument, tell the compiler that this is possible so that it forces us to handle it. + constructor(options?: ClientOptions | string) { const MsgPack = DefaultRealtime._MsgPack; if (!MsgPack) { throw new Error('Expected DefaultRealtime._MsgPack to have been set'); } super( - Defaults.objectifyOptions(options, { + Defaults.objectifyOptions(options, true, 'Realtime', { ...allCommonModularPlugins, Crypto: DefaultRealtime.Crypto ?? undefined, MsgPack, diff --git a/src/common/lib/client/defaultrest.ts b/src/common/lib/client/defaultrest.ts index b6359d9bb8..110aef6fa3 100644 --- a/src/common/lib/client/defaultrest.ts +++ b/src/common/lib/client/defaultrest.ts @@ -12,14 +12,15 @@ import Defaults from '../util/defaults'; `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) { + // The public typings declare that this requires an argument to be passed, but since we want to emit a good error message in the case where a non-TypeScript user does not pass an argument, tell the compiler that this is possible so that it forces us to handle it. + constructor(options?: ClientOptions | string) { const MsgPack = DefaultRest._MsgPack; if (!MsgPack) { throw new Error('Expected DefaultRest._MsgPack to have been set'); } super( - Defaults.objectifyOptions(options, { + Defaults.objectifyOptions(options, true, 'Rest', { ...allCommonModularPlugins, Crypto: DefaultRest.Crypto ?? undefined, MsgPack: DefaultRest._MsgPack ?? undefined, diff --git a/src/common/lib/util/defaults.ts b/src/common/lib/util/defaults.ts index cde1668181..48920ca6cf 100644 --- a/src/common/lib/util/defaults.ts +++ b/src/common/lib/util/defaults.ts @@ -44,7 +44,12 @@ type CompleteDefaults = IDefaults & { getHosts(options: NormalisedClientOptions): string[]; checkHost(host: string): void; getRealtimeHost(options: ClientOptions, production: boolean, environment: string): string; - objectifyOptions(options: ClientOptions | string, modularPluginsToInclude?: ModularPlugins): ClientOptions; + objectifyOptions( + options: undefined | ClientOptions | string, + allowKeyOrToken: boolean, + sourceForErrorMessage: string, + modularPluginsToInclude?: ModularPlugins, + ): ClientOptions; normaliseOptions(options: ClientOptions, MsgPack: MsgPack | null): NormalisedClientOptions; defaultGetHeaders(options: NormalisedClientOptions, headersOptions?: HeadersOptions): Record; defaultPostHeaders(options: NormalisedClientOptions, headersOptions?: HeadersOptions): Record; @@ -183,11 +188,42 @@ export function getAgentString(options: ClientOptions): string { } export function objectifyOptions( - options: ClientOptions | string, + options: undefined | ClientOptions | string, + allowKeyOrToken: boolean, + sourceForErrorMessage: string, modularPluginsToInclude?: ModularPlugins, ): ClientOptions { - let optionsObj = - typeof options === 'string' ? (options.indexOf(':') == -1 ? { token: options } : { key: options }) : options; + if (options === undefined) { + const msg = allowKeyOrToken + ? `${sourceForErrorMessage} must be initialized with either a client options object, an Ably API key, or an Ably Token` + : `${sourceForErrorMessage} must be initialized with a client options object`; + Logger.logAction(Logger.LOG_ERROR, `${sourceForErrorMessage}()`, msg); + throw new Error(msg); + } + + let optionsObj: ClientOptions; + + if (typeof options === 'string') { + if (options.indexOf(':') == -1) { + if (!allowKeyOrToken) { + const msg = `${sourceForErrorMessage} cannot be initialized with just an Ably Token; you must provide a client options object with a \`plugins\` property. (Set this Ably Token as the object’s \`token\` property.)`; + Logger.logAction(Logger.LOG_ERROR, `${sourceForErrorMessage}()`, msg); + throw new Error(msg); + } + + optionsObj = { token: options }; + } else { + if (!allowKeyOrToken) { + const msg = `${sourceForErrorMessage} cannot be initialized with just an Ably API key; you must provide a client options object with a \`plugins\` property. (Set this Ably API key as the object’s \`key\` property.)`; + Logger.logAction(Logger.LOG_ERROR, `${sourceForErrorMessage}()`, msg); + throw new Error(msg); + } + + optionsObj = { key: options }; + } + } else { + optionsObj = options; + } if (modularPluginsToInclude) { optionsObj = { ...optionsObj, plugins: { ...modularPluginsToInclude, ...optionsObj.plugins } }; diff --git a/test/browser/modular.test.js b/test/browser/modular.test.js index 594b14671d..2de10279b2 100644 --- a/test/browser/modular.test.js +++ b/test/browser/modular.test.js @@ -42,6 +42,40 @@ function registerAblyModularTests(helper) { helper.setupApp(done); }); + describe('attempting to initialize with no client options', () => { + for (const clientClass of [BaseRest, BaseRealtime]) { + describe(clientClass.name, () => { + it('throws an error', () => { + expect(() => new clientClass()).to.throw('must be initialized with a client options object'); + }); + }); + } + }); + + describe('attempting to initialize with just an API key', () => { + for (const clientClass of [BaseRest, BaseRealtime]) { + describe(clientClass.name, () => { + it('throws an error', () => { + expect(() => new clientClass('foo:bar')).to.throw( + 'cannot be initialized with just an Ably API key; you must provide a client options object with a `plugins` property', + ); + }); + }); + } + }); + + describe('attempting to initialize with just a token', () => { + for (const clientClass of [BaseRest, BaseRealtime]) { + describe(clientClass.name, () => { + it('throws an error', () => { + expect(() => new clientClass('foo')).to.throw( + 'cannot be initialized with just an Ably Token; you must provide a client options object with a `plugins` property', + ); + }); + }); + } + }); + describe('without any plugins', () => { for (const clientClass of [BaseRest, BaseRealtime]) { describe(clientClass.name, () => { diff --git a/test/realtime/api.test.js b/test/realtime/api.test.js index 951c0d0669..ab5e3695e4 100644 --- a/test/realtime/api.test.js +++ b/test/realtime/api.test.js @@ -8,6 +8,12 @@ define(['ably', 'chai'], function (Ably, chai) { expect(typeof Ably.Realtime).to.equal('function'); }); + it('constructor without any arguments', function () { + expect(() => new Ably.Realtime()).to.throw( + 'must be initialized with either a client options object, an Ably API key, or an Ably Token', + ); + }); + it('Crypto', function () { expect(typeof Ably.Realtime.Crypto).to.equal('function'); expect(typeof Ably.Realtime.Crypto.getDefaultParams).to.equal('function'); diff --git a/test/rest/api.test.js b/test/rest/api.test.js index fcc19aa3df..5bd351b4b0 100644 --- a/test/rest/api.test.js +++ b/test/rest/api.test.js @@ -8,6 +8,12 @@ define(['ably', 'chai'], function (Ably, chai) { expect(typeof Ably.Rest).to.equal('function'); }); + it('constructor without any arguments', function () { + expect(() => new Ably.Rest()).to.throw( + 'must be initialized with either a client options object, an Ably API key, or an Ably Token', + ); + }); + it('Crypto', function () { expect(typeof Ably.Rest.Crypto).to.equal('function'); expect(typeof Ably.Rest.Crypto.getDefaultParams).to.equal('function');