Skip to content

Conversation

@surminus
Copy link
Contributor

@surminus surminus commented Feb 25, 2025

This implements ADR-119[1], which specifies the client connection options to update requests to the endpoints implemented as part of ADR-042[2].

The endpoint may be one of the following:

  • a routing policy name (such as main)
  • a nonprod routing policy name (such as nonprod:sandbox)
  • a FQDN such as foo.example.com

The endpoint option is not valid with any of environment, restHost or realtimeHost, but we still intend to support the legacy options.

If the client has been configured to use any of these legacy options, then they should continue to work in the same way, using the same primary and fallback hostnames.

If the client has not been explicitly configured, then the hostnames will change to the new ably.net domain when the package is upgraded.

Notes on implementation

As per previous guidance, I have not updated tests to use the endpoint client option, but since the hostnames are changing, then the assertions and default environment must be updated (from sandbox to nonprod:sandbox).

I have not added any explicit tests for the endpoint client option, but I can do this if we think it's necessary, although using environment is essentially the same as using endpoint.

See related Go (merged), Ruby and Python PRs:

[1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3428810778/ADR-119+ClientOptions+for+new+DNS+structure
[2] https://ably.atlassian.net/wiki/spaces/ENG/pages/1791754276/ADR-042+DNS+Restructure

Summary by CodeRabbit

  • New Features
    • Introduced a new endpoint option for specifying connection routing policies or fully qualified domain names.
  • Bug Fixes
    • Unified host resolution under the endpoint concept, replacing separate environment and host options for improved consistency.
    • Consolidated host management by replacing separate HTTP and WebSocket hosts with a single primary domain.
  • Documentation
    • Deprecated environment, restHost, and realtimeHost options with updated guidance to use endpoint.
    • Updated environment variable references from ABLY_ENV to ABLY_ENDPOINT.
  • Tests
    • Added tests for endpoint normalization, fallback host behavior, and validation of conflicting client options.
    • Updated existing tests to reflect the new endpoint and primaryDomain configuration model.

@coderabbitai
Copy link

coderabbitai bot commented Feb 25, 2025

Walkthrough

The changes introduce a new endpoint property to replace the legacy environment and host configuration for Ably client connections. Default hostnames, fallback host resolution, and related test cases are updated to use this endpoint-based approach, deprecating previous environment-based mechanisms and aligning the codebase and tests with the new domain and fallback logic.

Changes

File(s) Change Summary
ably.d.ts Added optional endpoint property to ClientOptions. Marked environment, restHost, and realtimeHost as deprecated, recommending use of endpoint instead.
src/common/lib/util/defaults.ts Refactored to use endpoint-based host and fallback logic. Added ENDPOINT default, new helpers for endpoint host and fallback generation, removed environment-based logic, and updated normalization. Added validation for mutually exclusive client options.
test/common/globals/environment.js
test/package/browser/template/playwright/index.tsx
test/package/browser/template/src/index-default.ts
test/package/browser/template/src/index-modular.ts
test/package/browser/template/src/index-objects.ts
Changed default or configured environment string from 'sandbox' to 'nonprod:sandbox'. Replaced environment with endpoint in client configuration.
test/common/modules/testapp_manager.js Replaced environment-based domain prefixing with endpoint-based hostname construction. Introduced getHostname helper replacing prefixDomainWithEnvironment.
test/package/browser/template/src/sandbox.ts Updated API URL from sandbox-rest.ably.io to sandbox.realtime.ably-nonprod.net.
test/realtime/connectivity.test.js Changed test URL from sandbox-rest.ably.io/time to sandbox.realtime.ably-nonprod.net/time.
test/realtime/init.test.js Updated expected default HTTP host from rest.ably.io to main.realtime.ably.net. Modified tests to use AblyRealtimeWithoutEndpoint helper for some fallback tests.
test/rest/defaults.test.js Updated tests to verify new endpoint-based host and fallback logic, changed expected domains, expanded test coverage for endpoint normalization including FQDN and IP addresses, and adjusted behavior for restHost and realtimeHost precedence.
test/common/modules/private_api_recorder.js Added 'write.Defaults.ENDPOINT' to recognized private API identifiers.
test/common/modules/client_module.js Added helper ablyRealtimeWithoutEndpoint to instantiate clients without endpoint option.
test/common/modules/shared_helper.js Added AblyRealtimeWithoutEndpoint method to SharedHelper class using the new client creation helper.
test/realtime/api.test.js Added test case verifying error thrown when conflicting client options (endpoint with environment or restHost/realtimeHost) are provided.
test/realtime/failure.test.js Updated tests to use AblyRealtimeWithoutEndpoint helper instead of AblyRealtime for failure scenarios.
test/realtime/transports.test.js Replaced realtimeHost option with endpoint in tests configuring custom realtime hosts.
test/rest/fallbacks.test.js Updated to use endpoint option instead of restHost. Added test verifying primary domain is always the first attempted host on connection attempts.
test/rest/auth.test.js Changed embedded JWT test environment checks from environment to endpoint, stripping "nonprod:" prefix as needed.
test/rest/batch.test.js Relaxed batch publish test assertions to only require non-empty messageId instead of substring check.
test/rest/request.test.js Replaced restHost option with endpoint in REST client instantiation in tests.
scripts/moduleReport.ts Increased raw size threshold for minimal useful Realtime bundle from 102 KiB to 103 KiB.
src/common/lib/transport/comettransport.ts Changed host selection for comet transport to use params.host or fallback to options.primaryDomain, removing previous utility call.
src/common/lib/transport/connectionmanager.ts Consolidated httpHosts and wsHosts into single domains array in ConnectionManager. Updated all references and fallback logic accordingly.
src/common/types/ClientOptions.ts Replaced realtimeHost and restHost properties in NormalisedClientOptions with single primaryDomain property.
test/rest/message.test.js Updated test constants from restHost to primaryDomain. Updated environment variable reference from ABLY_ENV to ABLY_ENDPOINT.
test/rest/push.test.js Removed calls to Defaults.getHost; used primaryDomain directly for base URI derivation.
.vscode/launch.json Changed debug environment variable from ABLY_ENV to ABLY_ENDPOINT with value nonprod:sandbox.
CONTRIBUTING.md Updated documentation to replace ABLY_ENV, ABLY_REALTIME_HOST, and ABLY_REST_HOST with single ABLY_ENDPOINT.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant AblyClient
    participant Defaults

    User->>AblyClient: Initialize with ClientOptions (may include endpoint)
    AblyClient->>Defaults: Normalize options (derive endpoint)
    Defaults->>Defaults: Resolve restHost, realtimeHost, fallbackHosts using endpoint
    Defaults-->>AblyClient: Normalized hostnames and fallbacks
    AblyClient-->>User: Client ready with endpoint-based hosts
Loading

Possibly related PRs

Suggested reviewers

  • lawrence-forooghian

Poem

🐰
A hop, a skip, a brand new route,
Endpoints now, without a doubt!
Old hosts retire, new names arise,
Fallbacks dance in fresh disguise.
Sandbox and main, with joy we greet—
This rabbit loves a tidy suite!
☁️🌐

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

npm warn EBADENGINE Unsupported engine {
npm warn EBADENGINE package: '@es-joy/[email protected]',
npm warn EBADENGINE required: { node: '^14 || ^16 || ^17 || ^18 || ^19 || ^20' },
npm warn EBADENGINE current: { node: 'v24.2.0', npm: '11.3.0' }
npm warn EBADENGINE }
npm warn EBADENGINE Unsupported engine {
npm warn EBADENGINE package: '[email protected]',
npm warn EBADENGINE required: { node: '^14 || ^16 || ^17 || ^18 || ^19' },
npm warn EBADENGINE current: { node: 'v24.2.0', npm: '11.3.0' }
npm warn EBADENGINE }
npm error Exit handler never called!
npm error This is an error with npm itself. Please report this error at:
npm error https://github.com/npm/cli/issues
npm error A complete log of this run can be found in: /.npm/_logs/2025-06-24T08_50_36_521Z-debug-0.log


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2989879 and 1f1a811.

📒 Files selected for processing (2)
  • test/realtime/init.test.js (6 hunks)
  • test/rest/push.test.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/rest/push.test.js
  • test/realtime/init.test.js
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: test-browser (chromium)
  • GitHub Check: test-browser (firefox)
  • GitHub Check: test-browser (webkit)
  • GitHub Check: test-node (18.x)
  • GitHub Check: test-node (16.x)
  • GitHub Check: test-node (20.x)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate Unit Tests
  • Create PR with Unit Tests
  • Post Copyable Unit Tests in Comment
  • Commit Unit Tests in branch laura/endpoint-option

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai auto-generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot temporarily deployed to staging/pull/1973/bundle-report February 25, 2025 17:10 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/features February 25, 2025 17:11 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc February 25, 2025 17:11 Inactive
@lawrence-forooghian
Copy link
Collaborator

@surminus as discussed on Slack, please could you make sure that this PR implements / tests the updates to REC1b2 from ably/specification#302? Thanks!

ably.d.ts Outdated
endpoint?: string;

/**
* Enables a [custom environment](https://ably.com/docs/platform-customization) to be used with the Ably service.
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian Apr 24, 2025

Choose a reason for hiding this comment

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

I think we need to mark some options e.g. this one as deprecated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(see e.g. 36162c4 for an example)

@surminus surminus force-pushed the laura/endpoint-option branch from 6516e44 to 4ab1c98 Compare May 22, 2025 11:30
@github-actions github-actions bot temporarily deployed to staging/pull/1973/features May 22, 2025 11:30 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/bundle-report May 22, 2025 11:31 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc May 22, 2025 11:31 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/bundle-report May 22, 2025 14:26 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/features May 22, 2025 14:26 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc May 22, 2025 14:26 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/bundle-report May 22, 2025 14:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/features May 22, 2025 14:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc May 22, 2025 14:36 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/features May 22, 2025 14:53 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/bundle-report May 22, 2025 14:53 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc May 22, 2025 14:54 Inactive
@surminus
Copy link
Contributor Author

@lawrence-forooghian I've pushed some changes including some new tests. One test is failing but seems unrelated to my changes?

@surminus surminus marked this pull request as ready for review May 22, 2025 14:56
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

♻️ Duplicate comments (2)
test/rest/defaults.test.js (2)

134-138: Same issue for IPv6 – host should remain unchanged

-      expect(Defaults.getHost(normalisedOptions, '::1', false)).to.deep.equal('example.com');
-      expect(Defaults.getHost(normalisedOptions, '::1', true)).to.deep.equal('example.com');
+      expect(Defaults.getHost(normalisedOptions, '::1', false)).to.deep.equal('::1');
+      expect(Defaults.getHost(normalisedOptions, '::1', true)).to.deep.equal('::1');

154-158: localhost expectation incorrect

-      expect(Defaults.getHost(normalisedOptions, 'localhost', false)).to.deep.equal('example.com');
-      expect(Defaults.getHost(normalisedOptions, 'localhost', true)).to.deep.equal('example.com');
+      expect(Defaults.getHost(normalisedOptions, 'localhost', false)).to.deep.equal('localhost');
+      expect(Defaults.getHost(normalisedOptions, 'localhost', true)).to.deep.equal('localhost');
🧹 Nitpick comments (1)
src/common/lib/util/defaults.ts (1)

60-71: Fallback-host constants look stale & inconsistent with helper logic

Defaults.FALLBACK_HOSTS now hard-codes the main.*.ably-realtime.com pattern, but the newly-added endpointFallbacks() helper generates hosts dynamically.
Hard-coding these values means:

  • Future updates to the fallback generation rules will require touching two places.
  • The list may become out-of-sync with endpointFallbacks('main', …) (e.g. if the id order or domain changes).

Consider deriving the production list directly from endpointFallbacks() to keep a single source of truth:

-  FALLBACK_HOSTS: [
-    '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',
-  ],
+  // Keep in sync with `ENDPOINT`
+  FALLBACK_HOSTS: endpointFallbacks('main', 'ably-realtime.com'),
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6fbe35 and d6a6245.

📒 Files selected for processing (11)
  • ably.d.ts (2 hunks)
  • src/common/lib/util/defaults.ts (6 hunks)
  • test/common/globals/environment.js (1 hunks)
  • test/common/modules/testapp_manager.js (2 hunks)
  • test/package/browser/template/playwright/index.tsx (1 hunks)
  • test/package/browser/template/src/index-default.ts (1 hunks)
  • test/package/browser/template/src/index-modular.ts (1 hunks)
  • test/package/browser/template/src/sandbox.ts (1 hunks)
  • test/realtime/connectivity.test.js (1 hunks)
  • test/realtime/init.test.js (1 hunks)
  • test/rest/defaults.test.js (8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: test-browser (chromium)
  • GitHub Check: test-browser (webkit)
  • GitHub Check: test-browser (firefox)
  • GitHub Check: test-node (18.x)
  • GitHub Check: test-node (20.x)
  • GitHub Check: test-node (16.x)
🔇 Additional comments (12)
test/realtime/connectivity.test.js (1)

139-139: URL updated to match new endpoint convention

The connectivity test URL has been updated from sandbox-rest.ably.io/time to sandbox.realtime.ably-nonprod.net/time to align with the new endpoint structure as defined in ADR-119. This change correctly implements the new domain naming scheme for non-production environments.

test/common/globals/environment.js (1)

6-6: Default environment updated to match endpoint convention

The default environment value has been changed from 'sandbox' to 'nonprod:sandbox' to align with the new endpoint-based approach where non-production environments use the nonprod: prefix. This change is consistent with the implementation described in ADR-119.

Let's verify that this default environment is correctly handled in the codebase:

#!/bin/bash
# Check how this environment value is used in the codebase
rg "environment\s*:\s*['\"]nonprod:sandbox['\"]" --type js --type ts
test/package/browser/template/playwright/index.tsx (1)

12-12: Environment value updated to match endpoint convention

The environment value in the Ably client initialization has been updated from 'sandbox' to 'nonprod:sandbox' to align with the new endpoint-based approach. This change is consistent with the implementation in other test files and with ADR-119.

test/realtime/init.test.js (1)

282-282: Default host correctly updated to use the new endpoint-based domain

The expected default host has been correctly updated to use the new endpoint-based domain format, aligning with ADR-119. This confirms that clients without explicit configuration will use the new ably.net domain.

test/package/browser/template/src/index-modular.ts (1)

27-31: Environment value correctly updated to use nonprod prefix

The environment parameter has been properly updated from 'sandbox' to 'nonprod:sandbox' to align with the new endpoint naming convention in ADR-119, where non-production routing policy names use the 'nonprod:' prefix.

test/package/browser/template/src/sandbox.ts (1)

8-8: API URL updated to use new endpoint-based domain

The URL has been properly updated to use the new domain structure 'sandbox.realtime.ably-nonprod.net' instead of the legacy 'sandbox-rest.ably.io', consistent with the endpoint naming changes in ADR-119.

test/common/modules/testapp_manager.js (2)

6-6: Updated host resolution to use endpoint-based approach

The restHost resolution logic has been correctly updated to use the new setHostname function, supporting the endpoint-based configuration approach introduced in ADR-119.


26-32: New setHostname function correctly implements endpoint-based hostname resolution

The setHostname function correctly implements the endpoint-based hostname resolution as specified in ADR-119, handling both standard endpoints ('main' → 'main.realtime.ably.net') and non-production endpoints ('nonprod:sandbox' → 'sandbox.realtime.ably-nonprod.net').

ably.d.ts (4)

393-396: New endpoint property clearly documents connection options

The new endpoint property introduces a more flexible approach to specify connection endpoints through routing policies or FQDNs, as outlined in ADR-119. The documentation refers users to the platform customization documentation for details.


398-401: Appropriate deprecation notice for environment property

The environment property is correctly marked as deprecated with a reference to the new endpoint property, maintaining backward compatibility while encouraging migration to the new approach.


431-433: Appropriate deprecation notice for restHost property

The restHost property is correctly marked as deprecated with a reference to the new endpoint property, ensuring consistency with other deprecated connection options.


436-438: Appropriate deprecation notice for realtimeHost property

The realtimeHost property is correctly marked as deprecated with a reference to the new endpoint property, completing the consistent deprecation of all legacy connection options.

@lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian I've pushed some changes including some new tests. One test is failing but seems unrelated to my changes?

@surminus we do have some flaky tests, but in e.g. https://github.com/ably/ably-js/actions/runs/15189867099/job/42719585570?pr=1973 there seem to be quite a few failing tests related to hostnames?

@github-actions github-actions bot temporarily deployed to staging/pull/1973/features May 28, 2025 16:50 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/bundle-report May 28, 2025 16:50 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc May 28, 2025 16:50 Inactive
@surminus
Copy link
Contributor Author

@lawrence-forooghian I've fixed all but one test, which I don't understand. The restHost option is being passed but is undefined in the normaliseOptions function. Any pointers would be appreciated.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/common/lib/util/defaults.ts (2)

124-135: Add 'production' shortcut for backward compatibility.

The function is missing handling for the 'production' endpoint which should resolve to 'main.realtime.ably.net' for backward compatibility. Users passing { endpoint: 'production' } or { environment: 'production' } would currently get an invalid hostname.

 export function getEndpointHostname(endpoint: string): string {
+  if (endpoint === 'production') {
+    return 'main.realtime.ably.net';
+  }
   if (endpoint.includes('.') || endpoint.includes('::') || endpoint.includes('localhost')) {
     return endpoint;
   }

266-271: Fix production environment handling and empty fallbacks assignment.

The current logic incorrectly treats environment: 'production' as a literal endpoint and unconditionally assigns fallbackHosts even when empty.

-  const endpoint = options.endpoint || options.environment || Defaults.ENDPOINT;
+  let endpoint =
+    options.endpoint ??
+    (options.environment && options.environment !== 'production' ? options.environment : undefined) ??
+    Defaults.ENDPOINT;

   if (!options.fallbackHosts && !options.restHost && !options.realtimeHost && !options.port && !options.tlsPort) {
-    options.fallbackHosts = getEndpointFallbackHosts(endpoint);
+    const fallbacks = getEndpointFallbackHosts(endpoint);
+    if (fallbacks.length) {
+      options.fallbackHosts = fallbacks;
+    }
   }
🧹 Nitpick comments (1)
test/rest/defaults.test.js (1)

114-114: Remove debugging console.log statement.

This console.log statement should be removed as it's not needed in production test code.

-      console.log(Defaults.getHosts(normalisedOptions));
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6a6245 and 2650a51.

📒 Files selected for processing (3)
  • src/common/lib/util/defaults.ts (6 hunks)
  • test/common/modules/private_api_recorder.js (1 hunks)
  • test/rest/defaults.test.js (6 hunks)
✅ Files skipped from review due to trivial changes (1)
  • test/common/modules/private_api_recorder.js
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: test-node (20.x)
  • GitHub Check: test-node (18.x)
  • GitHub Check: test-node (16.x)
  • GitHub Check: test-browser (webkit)
  • GitHub Check: test-browser (firefox)
  • GitHub Check: test-browser (chromium)
🔇 Additional comments (6)
test/rest/defaults.test.js (4)

27-28: LGTM! Hostnames correctly updated to new endpoint-based domains.

The test correctly expects the new main.realtime.ably.net domain instead of the legacy rest.ably.io and realtime.ably.io domains, aligning with the endpoint-based architecture.

Also applies to: 38-41


55-81: Well-structured test for endpoint-based configuration.

The test correctly validates the new endpoint: 'nonprod:sandbox' configuration and verifies the expected hostname resolution and fallback generation using the new endpoint-based functions.


87-122: Comprehensive test coverage for FQDN and IP address endpoints.

These new test cases properly validate that custom endpoints (FQDN, IPv4, IPv6, localhost) resolve correctly and don't generate fallback hosts, which aligns with the REC1b2 specification requirements.


343-344: Correct migration from ENVIRONMENT to ENDPOINT default.

The test correctly updates from setting Defaults.ENVIRONMENT to Defaults.ENDPOINT and properly resets the value, maintaining test isolation.

Also applies to: 370-371

src/common/lib/util/defaults.ts (2)

61-61: LGTM! Endpoint property and fallback hosts correctly updated.

The new ENDPOINT property with default 'main' and the updated fallback host naming scheme using endpoint prefixes align well with the new architecture.

Also applies to: 66-70


150-152: Clean and efficient fallback generation helper.

The endpointFallbacks helper function provides a clean way to generate the fallback hostnames with the correct pattern.

@github-actions github-actions bot temporarily deployed to staging/pull/1973/features June 20, 2025 08:29 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc June 20, 2025 08:29 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (3)
src/common/lib/util/defaults.ts (3)

120-122: Fix IPv6 detection to catch all IPv6 addresses.

The current implementation only checks for :: which misses fully-expanded IPv6 addresses. This could cause fallback hosts to be incorrectly generated for IPv6 addresses.

Apply this fix:

-  return endpoint.includes('.') || endpoint.includes('::') || endpoint === 'localhost';
+  return endpoint.includes('.') || endpoint.includes(':') || endpoint === 'localhost';

127-139: Add production environment handling and improve spec annotations.

The function lacks handling for the 'production' environment case and needs better spec annotations for clarity.

Apply this fix:

+/**
+ * @spec REC1b2 - Converts routing policy ID to primary domain
+ */
 export function getPrimaryDomainFromEndpoint(endpoint: string): string {
+  if (endpoint === 'production') {
+    return 'main.realtime.ably.net';
+  }
+
   // 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:', '');
+    const routingPolicyId = endpoint.slice('nonprod:'.length);
     return `${routingPolicyId}.realtime.ably-nonprod.net`;
   }

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

299-312: Fix production environment handling and fallback generation logic.

The current implementation has two critical issues that need to be addressed for proper endpoint-based configuration.

Apply this fix:

   /* 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(options.environment || endpoint);
+    options.fallbackHosts = getEndpointFallbackHosts(endpoint);
   }

-  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);

This ensures that:

  1. Fallback generation uses the endpoint consistently
  2. Production environment is handled correctly without creating invalid domains
🧹 Nitpick comments (1)
src/common/lib/util/defaults.ts (1)

146-158: Improve fallback host generation logic.

The function correctly skips fallbacks for custom endpoints but could benefit from better spec annotations and more efficient string manipulation.

Consider this improvement for consistency:

+/**
+ * @spec REC2c - Generates fallback hosts for routing policy ID
+ */
 export function getEndpointFallbackHosts(endpoint: string): string[] {
   // REC2c2
   if (isFqdnIpOrLocalhost(endpoint)) return [];

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

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

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2990847 and 7bd2254.

📒 Files selected for processing (15)
  • .vscode/launch.json (1 hunks)
  • CONTRIBUTING.md (1 hunks)
  • src/common/lib/transport/comettransport.ts (1 hunks)
  • src/common/lib/transport/connectionmanager.ts (5 hunks)
  • src/common/lib/util/defaults.ts (9 hunks)
  • src/common/types/ClientOptions.ts (1 hunks)
  • test/common/globals/environment.js (3 hunks)
  • test/common/modules/private_api_recorder.js (4 hunks)
  • test/common/modules/testapp_manager.js (2 hunks)
  • test/realtime/init.test.js (6 hunks)
  • test/realtime/transports.test.js (4 hunks)
  • test/rest/defaults.test.js (8 hunks)
  • test/rest/fallbacks.test.js (4 hunks)
  • test/rest/message.test.js (2 hunks)
  • test/rest/push.test.js (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • .vscode/launch.json
🚧 Files skipped from review as they are similar to previous changes (13)
  • test/rest/message.test.js
  • CONTRIBUTING.md
  • src/common/types/ClientOptions.ts
  • test/rest/fallbacks.test.js
  • test/rest/push.test.js
  • src/common/lib/transport/comettransport.ts
  • test/common/modules/private_api_recorder.js
  • test/common/globals/environment.js
  • test/common/modules/testapp_manager.js
  • src/common/lib/transport/connectionmanager.ts
  • test/realtime/init.test.js
  • test/realtime/transports.test.js
  • test/rest/defaults.test.js
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: test-node (16.x)
  • GitHub Check: test-node (20.x)
  • GitHub Check: test-browser (chromium)
  • GitHub Check: test-node (18.x)
  • GitHub Check: test-npm-package
  • GitHub Check: test-browser (webkit)
  • GitHub Check: test-browser (firefox)
🔇 Additional comments (5)
src/common/lib/util/defaults.ts (5)

16-16: LGTM! Basic endpoint infrastructure additions are correct.

The additions of ENDPOINT property, new function signatures, default endpoint value, and updated fallback hosts with "main" prefix align well with the endpoint-based approach specified in ADR-119.

Also applies to: 43-44, 61-61, 66-70, 98-99


160-162: LGTM! Helper function is correctly implemented.

The endpointFallbacks function properly generates the fallback host array using the routing policy ID and domain.


172-174: LGTM! Simplified host resolution using primaryDomain.

The updated getHosts function correctly uses the unified primaryDomain approach instead of separate rest/realtime hosts, which aligns with the new endpoint-based architecture.


250-270: LGTM! Validation logic enforces proper option usage.

The checkIfClientOptionsAreValid function correctly implements the mutual exclusivity requirements between endpoint and legacy options as specified in REC1b and REC1c, with appropriate error codes and messages.


356-356: LGTM! Primary domain assignment is correct.

The assignment of primaryDomain in the returned normalized options object properly consolidates the host configuration into a single unified property.

@github-actions github-actions bot temporarily deployed to staging/pull/1973/features June 20, 2025 08:43 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/bundle-report June 20, 2025 08:43 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc June 20, 2025 08:43 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (2)
src/common/lib/util/defaults.ts (2)

120-122: IPv6 detection is still incomplete.

As noted in previous reviews, the check for '::' only catches compressed IPv6 addresses but misses fully-expanded ones like 2001:0db8:85a3:0000:0000:8a2e:0370:7334.

Use ':' instead of '::' to detect all IPv6 formats:

-  return endpoint.includes('.') || endpoint.includes('::') || endpoint === 'localhost';
+  return endpoint.includes('.') || endpoint.includes(':') || endpoint === 'localhost';

303-303: Fallback generation logic still incorrect.

As noted in previous reviews, this should use endpoint for fallback generation, not options.environment || endpoint which allows environment to override endpoint.

-    options.fallbackHosts = getEndpointFallbackHosts(options.environment || endpoint);
+    options.fallbackHosts = getEndpointFallbackHosts(endpoint);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7bd2254 and 86cb17d.

📒 Files selected for processing (1)
  • src/common/lib/util/defaults.ts (9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: test-browser (webkit)
  • GitHub Check: test-npm-package
  • GitHub Check: test-browser (chromium)
  • GitHub Check: test-browser (firefox)
  • GitHub Check: test-node (20.x)
  • GitHub Check: test-node (16.x)
  • GitHub Check: test-node (18.x)
🔇 Additional comments (6)
src/common/lib/util/defaults.ts (6)

16-16: LGTM - Clean endpoint system integration.

The addition of ENDPOINT property and related function signatures properly establishes the new endpoint-based configuration system.

Also applies to: 43-44, 61-61, 98-99


66-70: LGTM - Fallback hosts correctly updated for endpoint system.

The fallback hosts now properly include the 'main' routing policy ID prefix, aligning with the new endpoint-based configuration.


146-158: LGTM - Fallback host generation logic is correct.

The function properly prevents fallback generation for custom endpoints and handles both production and nonprod routing policies appropriately.


172-173: LGTM - Simplified host resolution aligns with unified domain approach.

The function correctly uses the new primaryDomain concept instead of distinguishing between REST and realtime hosts.


250-270: LGTM - Essential validation for endpoint system.

The validation correctly enforces mutual exclusivity between the new endpoint option and legacy configuration options, preventing invalid configurations.


356-356: LGTM - Primary domain properly included in normalized options.

The primaryDomain is correctly added to the returned normalized options object.

@ttypic ttypic force-pushed the laura/endpoint-option branch from 86cb17d to 1840e8e Compare June 20, 2025 08:52
@github-actions github-actions bot temporarily deployed to staging/pull/1973/bundle-report June 20, 2025 08:53 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/features June 20, 2025 08:53 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc June 20, 2025 08:53 Inactive
ttypic added 5 commits June 20, 2025 10:02
- replaced `restHost` and `realtimeHost` in tests (where it's possible)
- fixed tests according to the new spec
- add client options validation according to the spec
- Refactored tests, using `primaryDomain` instead of `restHost` and `realtimeHost`.
- Updated implementation to align with new naming conventions.
- Adjusted specs and test expectations accordingly.
@ttypic ttypic force-pushed the laura/endpoint-option branch from 1840e8e to 2989879 Compare June 20, 2025 09:03
@github-actions github-actions bot temporarily deployed to staging/pull/1973/bundle-report June 20, 2025 09:03 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/features June 20, 2025 09:03 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1973/typedoc June 20, 2025 09:04 Inactive
* knows about; converted to realtimeHost by the websocketTransport */
expect(hosts[0]).to.equal(realtime.options.realtimeHost, 'Check connected realtime host is the first option');
expect(hosts[0]).to.equal(
realtime.options.primaryDomain,
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a few places where you're reading options.primaryDomain; please could you record this private API usage?

const client = new Ably.Realtime({
key,
environment: 'sandbox',
environment: 'nonprod:sandbox',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds like we need to make some changes in the Go implementation then?

@ttypic ttypic merged commit 6f2b7e6 into main Jun 24, 2025
10 of 14 checks passed
@ttypic ttypic deleted the laura/endpoint-option branch June 24, 2025 10:03
VeskeR added a commit that referenced this pull request Jun 24, 2025
…` client options

These client options have been deprecated in [1] if favor of the new
`ClientOptions.endpoint` parameter. They were marked as deprecated
only in the type declaration file, meaning that non-TypeScript users
wouldn't see the deprecation warning. This PR adds an explicit
deprecation warning log message when using these deprecated client
options.

[1] #1973
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants