Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"request": "launch",
"name": "Debug Test",
"env": {
"ABLY_ENV": "sandbox"
"ABLY_ENDPOINT": "nonprod:sandbox"
},
"cwd": "${workspaceFolder}",
"runtimeExecutable": "node",
Expand Down
4 changes: 1 addition & 3 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,7 @@ Run the following command to fix linting/formatting issues

All tests are run against the sandbox environment by default. However, the following environment variables can be set before running the Karma server to change the environment the tests are run against.

- `ABLY_ENV` - defaults to sandbox, however this can be set to another known environment such as 'staging'
- `ABLY_REALTIME_HOST` - explicitly tell the client library to use an alternate host for real-time websocket communication.
- `ABLY_REST_HOST` - explicitly tell the client library to use an alternate host for REST communication.
- `ABLY_ENDPOINT` - defaults to nonprod:sandbox, however this can be set to another known prod / nonprod routing policy id or primary domain
- `ABLY_PORT` - non-TLS port to use for the tests, defaults to 80
- `ABLY_TLS_PORT` - TLS port to use for the tests, defaults to 443
- `ABLY_USE_TLS` - true or false to enable/disable use of TLS respectively
Expand Down
15 changes: 8 additions & 7 deletions ably.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,12 @@ export interface ClientOptions<Plugins = CorePlugins> extends AuthOptions {
echoMessages?: boolean;

/**
* Enables a [custom environment](https://ably.com/docs/platform-customization) to be used with the Ably service.
* Set a routing policy or FQDN to connect to Ably. See [platform customization](https://ably.com/docs/platform-customization).
*/
endpoint?: string;

/**
* @deprecated This property is deprecated and will be removed in a future version. Use the {@link ClientOptions.endpoint} client option instead.
*/
environment?: string;

Expand Down Expand Up @@ -423,16 +428,12 @@ export interface ClientOptions<Plugins = CorePlugins> extends AuthOptions {
queueMessages?: boolean;

/**
* Enables a non-default Ably host to be specified. For development environments only. The default value is `rest.ably.io`.
*
* @defaultValue `"rest.ably.io"`
* @deprecated This property is deprecated and will be removed in a future version. Use the {@link ClientOptions.endpoint} client option instead.
*/
restHost?: string;

/**
* Enables a non-default Ably host to be specified for realtime connections. For development environments only. The default value is `realtime.ably.io`.
*
* @defaultValue `"realtime.ably.io"`
* @deprecated This property is deprecated and will be removed in a future version. Use the {@link ClientOptions.endpoint} client option instead.
*/
realtimeHost?: string;

Expand Down
2 changes: 1 addition & 1 deletion scripts/moduleReport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { gzip } from 'zlib';
import Table from 'cli-table';

// The maximum size we allow for a minimal useful Realtime bundle (i.e. one that can subscribe to a channel)
const minimalUsefulRealtimeBundleSizeThresholdsKiB = { raw: 102, gzip: 31 };
const minimalUsefulRealtimeBundleSizeThresholdsKiB = { raw: 103, gzip: 31 };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have to increase a little, bundle become 102.3kb


const baseClientNames = ['BaseRest', 'BaseRealtime'];

Expand Down
2 changes: 1 addition & 1 deletion src/common/lib/transport/comettransport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ abstract class CometTransport extends Transport {
Transport.prototype.connect.call(this);
const params = this.params;
const options = params.options;
const host = Defaults.getHost(options, params.host);
const host = params.host || options.primaryDomain;
const port = Defaults.getPort(options);
const cometScheme = options.tls ? 'https://' : 'http://';

Expand Down
12 changes: 5 additions & 7 deletions src/common/lib/transport/connectionmanager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,7 @@ class ConnectionManager extends EventEmitter {
baseTransport?: TransportName;
webSocketTransportAvailable?: true;
transportPreference: string | null;
httpHosts: string[];
wsHosts: string[];
domains: string[];
activeProtocol: null | Protocol;
pendingTransport?: Transport;
proposedTransport?: Transport;
Expand Down Expand Up @@ -293,8 +292,7 @@ class ConnectionManager extends EventEmitter {
this.baseTransport = TransportNames.Comet;
}

this.httpHosts = Defaults.getHosts(options);
this.wsHosts = Defaults.getHosts(options, true);
this.domains = Defaults.getHosts(options);
this.activeProtocol = null;
this.host = null;
this.lastAutoReconnectAttempt = null;
Expand Down Expand Up @@ -323,7 +321,7 @@ class ConnectionManager extends EventEmitter {
this.logger,
Logger.LOG_MICRO,
'Realtime.ConnectionManager()',
'http hosts = [' + this.httpHosts + ']',
'http domains = [' + this.domains + ']',
);

if (!this.transports.length) {
Expand Down Expand Up @@ -835,7 +833,7 @@ class ConnectionManager extends EventEmitter {
* setting an instance variable to force fallback hosts to be used (if
* any) here. Bit of a kludge, but no real better alternatives without
* rewriting the entire thing */
if (state === 'disconnected' && error && (error.statusCode as number) > 500 && this.httpHosts.length > 1) {
if (state === 'disconnected' && error && (error.statusCode as number) > 500 && this.domains.length > 1) {
this.unpersistTransportPreference();
this.forceFallbackHost = true;
/* and try to connect again to try a fallback host without waiting for the usual 15s disconnectedRetryTimeout */
Expand Down Expand Up @@ -1533,7 +1531,7 @@ class ConnectionManager extends EventEmitter {
this.notifyState({ state: this.states.connecting.failState as string, error: err });
};

const candidateHosts = ws ? this.wsHosts.slice() : this.httpHosts.slice();
const candidateHosts = this.domains.slice();

const hostAttemptCb = (fatal: boolean, transport: Transport) => {
if (connectCount !== this.connectCounter) {
Expand Down
148 changes: 91 additions & 57 deletions src/common/lib/util/defaults.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { ModularPlugins } from '../client/modularplugins';
let agent = 'ably-js/' + version;

type CompleteDefaults = IDefaults & {
ENDPOINT: string;
ENVIRONMENT: string;
REST_HOST: string;
REALTIME_HOST: string;
Expand All @@ -37,10 +38,10 @@ type CompleteDefaults = IDefaults & {
version: string;
protocolVersion: number;
agent: string;
getHost(options: ClientOptions, host?: string | null, ws?: boolean): string;
getPort(options: ClientOptions, tls?: boolean): number | undefined;
getHttpScheme(options: ClientOptions): string;
environmentFallbackHosts(environment: string): string[];
getPrimaryDomainFromEndpoint(endpoint: string): string;
getEndpointFallbackHosts(endpoint: string): string[];
getFallbackHosts(options: NormalisedClientOptions): string[];
getHosts(options: NormalisedClientOptions, ws?: boolean): string[];
checkHost(host: string): void;
Expand All @@ -57,15 +58,16 @@ type CompleteDefaults = IDefaults & {
};

const Defaults = {
ENDPOINT: 'main',
ENVIRONMENT: '',
REST_HOST: 'rest.ably.io',
REALTIME_HOST: 'realtime.ably.io',
FALLBACK_HOSTS: [
'A.ably-realtime.com',
'B.ably-realtime.com',
'C.ably-realtime.com',
'D.ably-realtime.com',
'E.ably-realtime.com',
'main.a.fallback.ably-realtime.com',
'main.b.fallback.ably-realtime.com',
'main.c.fallback.ably-realtime.com',
'main.d.fallback.ably-realtime.com',
'main.e.fallback.ably-realtime.com',
],
PORT: 80,
TLS_PORT: 443,
Expand All @@ -91,10 +93,10 @@ const Defaults = {
version,
protocolVersion: 3,
agent,
getHost,
getPort,
getHttpScheme,
environmentFallbackHosts,
getPrimaryDomainFromEndpoint,
getEndpointFallbackHosts,
getFallbackHosts,
getHosts,
checkHost,
Expand All @@ -104,13 +106,6 @@ const Defaults = {
defaultPostHeaders,
};

export function getHost(options: ClientOptions, host?: string | null, ws?: boolean): string {
if (ws) host = (host == options.restHost && options.realtimeHost) || host || options.realtimeHost;
else host = host || options.restHost;

return host as string;
}

export function getPort(options: ClientOptions, tls?: boolean): number | undefined {
return tls || options.tls ? options.tlsPort : options.port;
}
Expand All @@ -119,15 +114,51 @@ export function getHttpScheme(options: ClientOptions): string {
return options.tls ? 'https://' : 'http://';
}

// construct environment fallback hosts as per RSC15i
export function environmentFallbackHosts(environment: string): string[] {
return [
environment + '-a-fallback.ably-realtime.com',
environment + '-b-fallback.ably-realtime.com',
environment + '-c-fallback.ably-realtime.com',
environment + '-d-fallback.ably-realtime.com',
environment + '-e-fallback.ably-realtime.com',
];
/**
* REC1b2
*/
function isFqdnIpOrLocalhost(endpoint: string): boolean {
return endpoint.includes('.') || endpoint.includes('::') || endpoint === 'localhost';
}

/**
* REC1b
*/
export function getPrimaryDomainFromEndpoint(endpoint: string): string {
// REC1b2 (endpoint is a valid hostname)
if (isFqdnIpOrLocalhost(endpoint)) return endpoint;

// REC1b3 (endpoint in form "nonprod:[id]")
if (endpoint.startsWith('nonprod:')) {
const routingPolicyId = endpoint.replace('nonprod:', '');
return `${routingPolicyId}.realtime.ably-nonprod.net`;
}

// REC1b4 (endpoint in form "[id]")
return `${endpoint}.realtime.ably.net`;
}

/**
* REC2c
*
* @returns default callbacks based on endpoint client option
*/
export function getEndpointFallbackHosts(endpoint: string): string[] {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you annotate with the relevant spec points?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// REC2c2
if (isFqdnIpOrLocalhost(endpoint)) return [];

// REC2c3
if (endpoint.startsWith('nonprod:')) {
const routingPolicyId = endpoint.replace('nonprod:', '');
return endpointFallbacks(routingPolicyId, 'ably-realtime-nonprod.com');
}

// REC2c1
return endpointFallbacks(endpoint, 'ably-realtime.com');
}

export function endpointFallbacks(routingPolicyId: string, domain: string): string[] {
return ['a', 'b', 'c', 'd', 'e'].map((id) => `${routingPolicyId}.${id}.fallback.${domain}`);
}

export function getFallbackHosts(options: NormalisedClientOptions): string[] {
Expand All @@ -138,9 +169,8 @@ export function getFallbackHosts(options: NormalisedClientOptions): string[] {
return fallbackHosts ? Utils.arrChooseN(fallbackHosts, httpMaxRetryCount) : [];
}

export function getHosts(options: NormalisedClientOptions, ws?: boolean): string[] {
const hosts = [options.restHost].concat(getFallbackHosts(options));
return ws ? hosts.map((host) => getHost(options, host, true)) : hosts;
export function getHosts(options: NormalisedClientOptions): string[] {
return [options.primaryDomain].concat(getFallbackHosts(options));
}

function checkHost(host: string): void {
Expand All @@ -152,26 +182,6 @@ function checkHost(host: string): void {
}
}

function getRealtimeHost(options: ClientOptions, production: boolean, environment: string, logger: Logger): string {
if (options.realtimeHost) return options.realtimeHost;
/* prefer setting realtimeHost to restHost as a custom restHost typically indicates
* a development environment is being used that can't be inferred by the library */
if (options.restHost) {
Logger.logAction(
logger,
Logger.LOG_MINOR,
'Defaults.normaliseOptions',
'restHost is set to "' +
options.restHost +
'" but realtimeHost is not set, so setting realtimeHost to "' +
options.restHost +
'" too. If this is not what you want, please set realtimeHost explicitly.',
);
return options.restHost;
}
return production ? Defaults.REALTIME_HOST : environment + '-' + Defaults.REALTIME_HOST;
}

function getTimeouts(options: ClientOptions) {
/* Allow values passed in options to override default timeouts */
const timeouts: Record<string, number> = {};
Expand Down Expand Up @@ -237,11 +247,35 @@ export function objectifyOptions(
return optionsObj;
}

function checkIfClientOptionsAreValid(options: ClientOptions) {
// REC1b
if (options.endpoint && (options.environment || options.restHost || options.realtimeHost)) {
// RSC1b
throw new ErrorInfo(
'The `endpoint` option cannot be used in conjunction with the `environment`, `restHost`, or `realtimeHost` options.',
40106,
400,
);
}

// REC1c
if (options.environment && (options.restHost || options.realtimeHost)) {
// RSC1b
throw new ErrorInfo(
'The `environment` option cannot be used in conjunction with the `restHost`, or `realtimeHost` options.',
40106,
400,
);
}
}

export function normaliseOptions(
options: ClientOptions,
MsgPack: MsgPack | null,
logger: Logger | null, // should only be omitted by tests
): NormalisedClientOptions {
checkIfClientOptionsAreValid(options);

const loggerToUse = logger ?? Logger.defaultLogger;

if (typeof options.recover === 'function' && options.closeOnUnload === true) {
Expand All @@ -262,18 +296,19 @@ export function normaliseOptions(

if (!('queueMessages' in options)) options.queueMessages = true;

/* infer hosts and fallbacks based on the configured environment */
const environment = (options.environment && String(options.environment).toLowerCase()) || Defaults.ENVIRONMENT;
const production = !environment || environment === 'production';
/* infer hosts and fallbacks based on the specified endpoint */
const endpoint = options.endpoint || Defaults.ENDPOINT;

if (!options.fallbackHosts && !options.restHost && !options.realtimeHost && !options.port && !options.tlsPort) {
options.fallbackHosts = production ? Defaults.FALLBACK_HOSTS : environmentFallbackHosts(environment);
options.fallbackHosts = getEndpointFallbackHosts(options.environment || endpoint);
}
Comment on lines +299 to 304
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incorrect fallback generation - should use endpoint, not environment.

The fallback host generation is using options.environment || endpoint but should prioritize the endpoint logic. The environment option should not directly influence fallback generation when endpoint is available.

-  const endpoint = options.endpoint || Defaults.ENDPOINT;
+  const endpoint = options.endpoint || Defaults.ENDPOINT;

   if (!options.fallbackHosts && !options.restHost && !options.realtimeHost && !options.port && !options.tlsPort) {
-    options.fallbackHosts = getEndpointFallbackHosts(options.environment || endpoint);
+    options.fallbackHosts = getEndpointFallbackHosts(endpoint);
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/* infer hosts and fallbacks based on the specified endpoint */
const endpoint = options.endpoint || Defaults.ENDPOINT;
if (!options.fallbackHosts && !options.restHost && !options.realtimeHost && !options.port && !options.tlsPort) {
options.fallbackHosts = production ? Defaults.FALLBACK_HOSTS : environmentFallbackHosts(environment);
options.fallbackHosts = getEndpointFallbackHosts(options.environment || endpoint);
}
/* infer hosts and fallbacks based on the specified endpoint */
const endpoint = options.endpoint || Defaults.ENDPOINT;
if (!options.fallbackHosts && !options.restHost && !options.realtimeHost && !options.port && !options.tlsPort) {
options.fallbackHosts = getEndpointFallbackHosts(endpoint);
}
🤖 Prompt for AI Agents
In src/common/lib/util/defaults.ts around lines 299 to 304, the fallback host
generation incorrectly uses options.environment || endpoint, which causes
environment to override endpoint. Modify the code to use only the endpoint value
for generating fallback hosts, ensuring that options.fallbackHosts is set by
calling getEndpointFallbackHosts with the endpoint variable instead of
options.environment or endpoint.


const restHost = options.restHost || (production ? Defaults.REST_HOST : environment + '-' + Defaults.REST_HOST);
const realtimeHost = getRealtimeHost(options, production, environment, loggerToUse);
const primaryDomainFromEnvironment = options.environment && `${options.environment}.realtime.ably.net`;
const primaryDomainFromLegacyOptions = options.restHost || options.realtimeHost || primaryDomainFromEnvironment;

const primaryDomain = primaryDomainFromLegacyOptions || getPrimaryDomainFromEndpoint(endpoint);

(options.fallbackHosts || []).concat(restHost, realtimeHost).forEach(checkHost);
(options.fallbackHosts || []).concat(primaryDomain).forEach(checkHost);

Comment on lines +306 to 312
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Primary domain resolution logic needs clarity and production handling.

The logic for resolving the primary domain is complex and may not handle the 'production' environment case correctly. The environment-based logic should be simplified.

-  const primaryDomainFromEnvironment = options.environment && `${options.environment}.realtime.ably.net`;
+  const primaryDomainFromEnvironment = options.environment && options.environment !== 'production' 
+    ? `${options.environment}.realtime.ably.net`
+    : undefined;
   const primaryDomainFromLegacyOptions = options.restHost || options.realtimeHost || primaryDomainFromEnvironment;

   const primaryDomain = primaryDomainFromLegacyOptions || getPrimaryDomainFromEndpoint(endpoint);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const primaryDomainFromEnvironment = options.environment && `${options.environment}.realtime.ably.net`;
const primaryDomainFromLegacyOptions = options.restHost || options.realtimeHost || primaryDomainFromEnvironment;
const primaryDomain = primaryDomainFromLegacyOptions || getPrimaryDomainFromEndpoint(endpoint);
(options.fallbackHosts || []).concat(restHost, realtimeHost).forEach(checkHost);
(options.fallbackHosts || []).concat(primaryDomain).forEach(checkHost);
const primaryDomainFromEnvironment =
options.environment && options.environment !== 'production'
? `${options.environment}.realtime.ably.net`
: undefined;
const primaryDomainFromLegacyOptions =
options.restHost || options.realtimeHost || primaryDomainFromEnvironment;
const primaryDomain =
primaryDomainFromLegacyOptions || getPrimaryDomainFromEndpoint(endpoint);
(options.fallbackHosts || []).concat(primaryDomain).forEach(checkHost);
🤖 Prompt for AI Agents
In src/common/lib/util/defaults.ts around lines 306 to 312, the primary domain
resolution logic is overly complex and may mishandle the 'production'
environment case. Simplify the logic by explicitly checking if the environment
is 'production' and setting the primary domain accordingly, avoiding unnecessary
concatenations or fallbacks. Refactor to make the domain resolution
straightforward and clear for all environment cases.

options.port = options.port || Defaults.PORT;
options.tlsPort = options.tlsPort || Defaults.TLS_PORT;
Expand Down Expand Up @@ -318,8 +353,7 @@ export function normaliseOptions(

return {
...options,
realtimeHost,
restHost,
primaryDomain: primaryDomain,
maxMessageSize: options.maxMessageSize || Defaults.maxMessageSize,
timeouts,
connectivityCheckParams,
Expand Down
3 changes: 1 addition & 2 deletions src/common/types/ClientOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ export default interface ClientOptions extends API.ClientOptions<API.CorePlugins
export type NormalisedClientOptions = Modify<
ClientOptions,
{
realtimeHost: string;
restHost: string;
primaryDomain: string;
keyName?: string;
keySecret?: string;
timeouts: Record<string, number>;
Expand Down
Loading
Loading