Skip to content

chat-client: v1 prep PR 1 — AbortSignal, ChatError, options-bag, api-extractor#892

Merged
vicancy merged 10 commits into
Azure:mainfrom
vicancy:api-v1-p0
Jun 9, 2026
Merged

chat-client: v1 prep PR 1 — AbortSignal, ChatError, options-bag, api-extractor#892
vicancy merged 10 commits into
Azure:mainfrom
vicancy:api-v1-p0

Conversation

@vicancy

@vicancy vicancy commented May 27, 2026

Copy link
Copy Markdown
Member

Summary

First of two release-prep PRs for @azure/web-pubsub-chat-client v1. Scope kept deliberately small: cancellation, error uniformity, and an options-bag tidy-up — orthogonal to the upcoming model PR that will rewrite the public type re-exports.

Public-surface changes

AbortSignal across the public surface

  • New base OperationOptions { abortSignal?: AbortSignalLike } and per-method aliases (StartOptions, StopOptions, GetRoomOptions, CreateRoomOptions, SendToRoomOptions, GetUserInfoOptions, AddUserToRoomOptions, RemoveUserFromRoomOptions, ListRoomMessagesOptions).
  • Threaded through every public async method, invokeWithReturnType, and connection.start / LOGIN / GET_ROOM invocations inside startCore. Per-page listRoomMessages invocations are also cancellable.
  • @azure/abort-controller@^2.1.2 added as an explicit dependency so AbortSignalLike is reachable from consumer typings.

Uniform ChatError

  • ensureStarted, the userId getter, unknown-room rejections in sendToRoom / listRoomMessages, and the malformed-response guard in sendToConversation now throw ChatError (name: "ChatError", code: string) instead of plain Error.
  • New runtime constants object KnownChatErrorCode re-exports ERRORS (matches the Azure SDK Known<Name> convention used by KnownErrorCode, etc.). Three new codes added: NotStarted, UnknownRoom, InvalidServerResponse.

Options-naming convention: <methodName>Options

  • SendMessageOptionsSendToRoomOptions
  • RoomMemberOperationOptions → split into AddUserToRoomOptions and RemoveUserFromRoomOptions
  • Private internals (sendToConversation, manageRoomMember) use the base OperationOptions rather than synthesising one-off non-exported aliases.

Event-listener surface: drop Disposable, rename MessageEvent

api-extractor flagged two local types that collide with global names, surfacing as _2-suffixed re-exports in the report:

  • MessageEventChatMessageEvent — clashed with the DOM MessageEvent interface in lib.dom.d.ts.
  • Disposable dropped entirely — clashed with the TypeScript 5.2+ built-in Disposable interface ({ [Symbol.dispose](): void }). Rather than rename, the unsubscribe-by-returned-callable pattern was removed. All on*() listener methods now return void and are removed via off(event, callback), matching WebPubSubClient.on/off on the underlying transport. This eliminates having two competing unsubscribe idioms on adjacent APIs (client.on(...) vs client.connection.on(...)).
// Before
const dispose = client.on("message", cb);
dispose();

// After
client.on("message", cb);
client.off("message", cb);

Additionally, ChatMessageEvent is tightened: conversationId is dropped entirely (last public reference to a concept already privatised everywhere else by 515f5f2 / a9e31a5), and roomId flips from roomId?: string to roomId: string. The previous optional was defensive coding for two anomalous paths — a forward-protocol-reservation null in the notification path, and a narrow RoomLeft-during-send race in the sender-echo path — both of which now log a warning and skip emission inside the SDK rather than pushing the undefined onto every caller. The sender-echo path still returns the msgId so the send result is unaffected.

Move trailing optional positionals into the options bag

  • getRoom(roomId, withMembers, options?)getRoom(roomId, options?: { withMembers?, abortSignal? })withMembers defaults to false.
  • createRoom(title, members, roomId?, options?)createRoom(title, members, options?: { roomId?, abortSignal? }).

ListRoomMessagesOptions.pageSizemaxPageSize

Matches the @azure/core-paging PageSettings.maxPageSize convention already used by listRoomMessages(...).byPage({ maxPageSize }). The same name now applies at both the iterator-level default and the per-page override, so the two knobs stop looking like unrelated concepts.

Static ChatClient.start() accepts StartOptions

The instance start(options?: StartOptions) has propagated abortSignal end-to-end since the original AbortSignal pass, but the static one-shot helper was still calling chatClient.start() with no arguments — so a stuck construct-and-start couldn't be cancelled. Construction options and start options are now kept as separate parameters on the URL/credential overloads (the wpsClient overload only takes start options since the transport is already built):

static start(clientAccessUrl, webPubSubClientOptions?: WebPubSubClientOptions, options?: StartOptions): Promise<ChatClient>
static start(credential,      webPubSubClientOptions?: WebPubSubClientOptions, options?: StartOptions): Promise<ChatClient>
static start(wpsClient,                                                        options?: StartOptions): Promise<ChatClient>

Why two parameters instead of WebPubSubClientOptions & StartOptions? Upstream WebPubSubClientOptions lives in a separate package (@azure/web-pubsub-client) on a separate release cadence, and upstream itself deliberately separates construction options from operation options (its own StartOptions interface carries abortSignal). Intersecting the two would be fragile against upstream field additions. Keeping them positional preserves the full transport-config surface (autoReconnect, reconnectRetryOptions, keep-alive intervals, etc.) without exposing the intersection.

Tighten ChatMessageEvent: drop conversationId, require roomId

The pre-existing event shape leaked the very conversationId concept that the rest of this PR is hiding (sendToConversation → private; _conversationIds → private; unknown-room is now a ChatError). It also marked roomId optional with a TODO-flavoured comment ("Undefined for non-room conversations (currently unused)"), forcing every consumer to null-check a field that always carries a value in supported paths.

// Before
interface ChatMessageEvent { conversationId: string; roomId?: string; message: ChatMessage }

// After
interface ChatMessageEvent { roomId: string; message: ChatMessage }

Notification dispatch now warns and skips if the wire frame lacks roomId, rather than handing callers an inconsistent event.

Privatize isStarted

The public isStarted getter was a thin wrapper over the private _isStarted backing field. It was removed because:

  • TOCTOU-racy. if (client.isStarted) { await client.getRoom(...) } gives a false sense of safety — a stop() from another caller or a network drop between the check and the call still throws. The check cannot be made meaningful without a lock the SDK does not own.
  • Redundant. Every operation method calls ensureStarted() and throws ChatError(KnownChatErrorCode.NotStarted). Callers must handle that exception anyway, so the proactive check pays no rent.
  • No sibling precedent. Upstream WebPubSubClient exposes no equivalent state-check getter; it is exception-driven throughout.

userId is left as-is: it's a getter-only property (TypeScript renders this to consumers as effectively readonly userId: string — assignment is a compile error), and its lazy throw-on-not-started guard is preferable to a string | undefined field that would force null-checks at every call site.

Mirror WebPubSubClient.on/off shape: overload-per-event, On<X>Args, kebab-case

The previous generic indexed-map shape (on<K extends ChatEventName>(event: K, callback: ChatEventListener<K>)) plus on<Verb>(cb) convenience helpers has been replaced with explicit overloads — one per event — so the chat-domain client.on(...) reads identically to the connection-lifecycle client.connection.on(...). Three coupled changes:

  • One overload per event for both on and off (5 events × 2 directions = 10 overloads). Drops ChatEventMap, ChatEventName, ChatEventListener<K> — they only existed to back the generic.
  • Event payloads renamed to On<Event>Args, matching the upstream OnConnectedArgs / OnGroupDataMessageArgs convention: ChatMessageEventOnMessageArgs, RoomJoinedEventOnRoomJoinedArgs, RoomLeftEventOnRoomLeftArgs, MemberJoinedEventOnMemberJoinedArgs, MemberLeftEventOnMemberLeftArgs.
  • Event names switched to kebab-case to match upstream literals: "roomJoined""room-joined", "roomLeft""room-left", "memberJoined""member-joined", "memberLeft""member-left" ("message" unchanged).
  • Dropped the on<Verb>(cb) convenience helpers. Upstream has none, and they had an asymmetric-removal smell: there was never a paired offMessage(cb), so callers still had to use off("message", cb) to unregister. One entry point per event in each direction now.
// Before
client.onMessage((event) => { /* event: ChatMessageEvent */ });
client.onRoomJoined((event) => { /* event: RoomJoinedEvent */ });

// After (mirrors client.connection.on("server-message", ...) on the sibling)
client.on("message",     (event) => { /* event: OnMessageArgs */ });
client.on("room-joined", (event) => { /* event: OnRoomJoinedArgs */ });
client.off("message", handler);

Collapse to single (wpsClient) constructor; drop URL/credential overloads

The URL and credential constructor overloads were a footgun: they left callers with a half-constructed ChatClient that threw ChatError(NotStarted) on every method call until the instance start() was awaited. The static ChatClient.start(url, ...) / start(credential, ...) factories already construct the underlying WebPubSubClient and atomically start the chat client, so there's no reason to keep the parallel path. After this change:

new ChatClient(wpsClient: WebPubSubClient)                       // only constructor

ChatClient.start(url,        wpsOpts?,  startOpts?): Promise<>  // builds & starts
ChatClient.start(credential, wpsOpts?,  startOpts?): Promise<>  // builds & starts
ChatClient.start(wpsClient,             startOpts?): Promise<>  // starts existing

Internally the static URL/credential branches now new WebPubSubClient(...) first and pass the result to new ChatClient(wpsClient), so there is exactly one transport-construction path.

New chat-domain started and stopped events

The pre-existing client.connection.on("connected", ...) and connection.on("stopped", ...) events fire at the transport boundary. They miss the chat-domain transition: connected fires before LOGIN + room hydration complete, so a UI that flips to a "ready" state on connected will still hit ChatError(NotStarted) on subsequent operations. The new events fire on the chat-domain transition:

client.on("started", (e) => {
  console.log(`Chat ready as ${e.userId}`); // OnStartedArgs { userId: string }
});
client.on("stopped", () => {
  console.log("Chat stopped");              // OnStoppedArgs {}
});

OnStartedArgs carries { userId }, mirroring upstream OnConnectedArgs.userId. OnStoppedArgs is empty, matching the upstream OnStoppedArgs shape exactly.

Emission rules — exactly one fire per transition:

  • started fires at the end of a successful startCore(), after _isStarted = true and _userId / _rooms are live. Re-calling start() on an already-started client is a no-op and does not re-emit.
  • stopped fires from inside resetState(), guarded on a local wasStarted = this._isStarted snapshot taken on entry. This guarantees one emission per started→not-started transition, covering both explicit stop() and transport-driven termination (the connection.on("stopped") callback installed in the constructor). Reset paths that run before _isStarted was ever set to true (the pre-start resetState() at the top of startCore() and the post-failure rollback) do not emit.

Two new lifecycle.test.ts cases pin the rules (8/8 green): "started event fires once after start() completes with userId payload" and "stopped event fires on started→not-started transitions only".

Tooling

  • @microsoft/api-extractor@^7.58.7 wired in as a devDependency with a minimal api-extractor.json. Two new scripts:
    • npm run extract-api — update the report
    • npm run check-api — fail in CI if the report is stale
  • The generated review/web-pubsub-chat-client.api.md is committed so every public-surface change shows up as a diff on PR review.
  • _conversationIds: Set<string> demoted from protected to private (caught by api-extractor — internal cache leaking into the API surface).
  • TSDoc cleanup driven by api-extractor warnings (ambiguous {@link ChatClient.start} refs, {@link WebPubSubClientCredential} not re-exported, fenced @example).
  • .gitignore: ignore temp/ and package-lock.json (yarn.lock is the lockfile).

Breaking changes (small, contained)

Change Migration
sendToConversationprivate Use sendToRoom(roomId, message, options?) (already the supported public path).
getRoom(roomId, true) getRoom(roomId, { withMembers: true })
createRoom(title, members, "my-id") createRoom(title, members, { roomId: "my-id" })
listRoomMessages({ pageSize: N }) listRoomMessages({ maxPageSize: N })
SendMessageOptions SendToRoomOptions
RoomMemberOperationOptions AddUserToRoomOptions / RemoveUserFromRoomOptions
MessageEvent (DOM-clashing type name) ChatMessageEvent
Disposable returned from on*() listener registrations on*() return void; remove with off(event, callback)
ChatMessageEvent.conversationId removed; roomId is now required Drop reads of event.conversationId; rely on event.roomId being defined
client.isStarted removed Rely on the ChatError(KnownChatErrorCode.NotStarted) thrown by operation methods (or track state in your own UI layer)
Plain Error thrown for unknown-room / not-started / bad response ChatError with KnownChatErrorCode.* code
ChatEventMap / ChatEventName / ChatEventListener<K> exported types Removed; use the explicit on(event, listener) overloads — each overload narrows the listener type automatically
on<Verb>(cb) convenience helpers (onMessage, onRoomJoined, onRoomLeft, onMemberJoined, onMemberLeft) on("event-name", cb) and off("event-name", cb)
Event-payload types (ChatMessageEvent, RoomJoinedEvent, ...) Renamed to OnMessageArgs, OnRoomJoinedArgs, ... (matches upstream On<X>Args)
Event names ("roomJoined", "roomLeft", "memberJoined", "memberLeft") Kebab-case ("room-joined", "room-left", "member-joined", "member-left")
new ChatClient(clientAccessUrl, options?) constructor Use await ChatClient.start(clientAccessUrl, webPubSubClientOptions?, startOptions?)
new ChatClient(credential, options?) constructor Use await ChatClient.start(credential, webPubSubClientOptions?, startOptions?)

Explicitly out of scope (deferred to the model PR)

  • Hand-curated public Message/Room/UserProfile types (Pick/Omit aliases over generated wire types)
  • Sender-echo createdAt/bodyType synthesis
  • connection getter JSDoc, isWebPubSubClient OR → AND, dead decodeMessageBody
  • Empty ChatMessage extends MessageInfo {} interface cleanup
  • Full README API-table rewrite, LICENSE / CHANGELOG

Validation

  • tsc -p tsconfig.json clean.
  • tsx --test tests/lifecycle.test.ts — 6/6 green.
  • api-extractor run --local — 0 warnings; the committed review/web-pubsub-chat-client.api.md reflects the exact public surface.
  • Integration tests typecheck against the new API; the seven createRoom(..., roomId) call sites, the single getRoom(..., true) call, and the listRoomMessages({ ..., pageSize }) call in tests/integration.test.ts were updated to the new bag form. They require a live Web PubSub backend and were not executed locally — please run in CI.

@vicancy vicancy marked this pull request as draft May 27, 2026 06:12
vicancy and others added 2 commits May 27, 2026 17:20
This is PR 1 of the v1-prep pass. Three orthogonal hardenings:

1. AbortSignal across the public surface
   - Add `OperationOptions { abortSignal? }` and per-method aliases
     (`StartOptions`, `GetRoomOptions`, `CreateRoomOptions`,
     `SendMessageOptions`, `GetUserInfoOptions`,
     `RoomMemberOperationOptions`, `StopOptions`).
   - Extend `ListRoomMessagesOptions` with `OperationOptions` so its
     `abortSignal` reaches every paged `invokeEvent` call inside
     `fetchPage`.
   - Thread the signal through `invokeWithReturnType` and `start` /
     `startCore` (initial connection, login, room hydration).
   - Add `@azure/abort-controller@^2.1.2` as an explicit dependency
     so `AbortSignalLike` is reachable from consumer typings.

2. `sendToConversation` is now `private`
   The conversation concept is an internal implementation detail; the
   only supported entry points are `sendToRoom` /
   `listRoomMessages`. Removing this method from the public surface
   is the one (small) breaking change in this PR.

3. Uniform `ChatError` for thrown failures
   `ensureStarted`, the `userId` getter, unknown-room rejections in
   `sendToRoom` / `listRoomMessages`, and the malformed-response
   guard in `sendToConversation` all now throw `ChatError` with a
   service-style `code` instead of a plain `Error`. Three new codes
   are exposed: `NotStarted`, `UnknownRoom`, `InvalidServerResponse`.

   `index.ts` re-exports the constants object as `KnownChatErrorCode`,
   matching the Azure SDK `Known<Name>` convention used by
   `KnownErrorCode` in cognitive-language and the
   `Known<X>ValidationErrorCode` pattern in ARM packages. Keys are
   PascalCase (`KnownChatErrorCode.UnknownRoom`) so the value used at
   runtime and the wire string returned by the service match exactly.

README:
   Methods table updated to show the trailing `options?` argument,
   plus a new "Errors" section documenting `KnownChatErrorCode`.

Deferred to a follow-up "model" PR:
   public `Message` / `Room` / `UserProfile` types via hand-picked
   `Pick`/`Omit` aliases, `MessageEvent.conversationId` removal,
   ergonomics fixes (`createRoom`/`getRoom` options-bag),
   `pageSize` -> `maxPageSize`, sender-echo synthesis, and the
   `ChatMessage` empty-interface cleanup.

Verification: `tsc --noEmit` clean, `tsx --test` 6/6 lifecycle tests green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tooling: @microsoft/api-extractor

  This is the canonical TypeScript API-surface lock for the Azure SDK
  ecosystem; every package under azure-sdk-for-js ships a generated
  review/<name>.api.md that CI keeps in sync. We adopt the same
  workflow here:

  - Add @microsoft/api-extractor@^7.58.7 as a devDependency.
  - Add api-extractor.json (minimal config: report only, no doc-model
    or dts-rollup since we ship hand-curated dist typings).
  - Wire two npm scripts:
      extract-api  -- api-extractor run --local   (update report)
      check-api    -- api-extractor run           (fail if stale)
  - Commit the generated review/web-pubsub-chat-client.api.md so the
    public surface is reviewable as a diff on every PR.

Bug fix surfaced by the report

  `_conversationIds: Set<string>` was declared `protected`, leaking
  internal state into the API surface. Demoted to `private`; the
  field is purely an internal cache populated during login.

TSDoc cleanup driven by api-extractor warnings

  - Replaced ambiguous {@link ChatClient.start} references (the
    static and instance overloads share the name) with inline code
    refs in the constructor JSDoc and in options.ts.
  - Replaced {@link WebPubSubClientCredential} with plain text -- that
    symbol is not re-exported from this package, so it can't be a link
    target.
  - Replaced {@link on} (ambiguous, also collides with EventEmitter)
    with plain `on()` reference.
  - Wrapped the inline @example in a ```ts code fence so the `=>`
    arrow no longer trips tsdoc-escape-greater-than.

Hygiene

  - .gitignore: ignore api-extractor's `temp/` working directory and
    `package-lock.json` (the package uses yarn.lock as the lockfile).

Verification

  - tsc --noEmit clean.
  - api-extractor run --local: zero warnings.
  - tsx --test tests/lifecycle.test.ts: 6/6 pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@vicancy vicancy marked this pull request as ready for review May 28, 2026 03:17
@vicancy vicancy changed the title chat-client: P0 API surface refactor before v1 release chat-client: v1 prep PR 1 — AbortSignal, ChatError, options-bag, api-extractor May 28, 2026
Three orthogonal ergonomics fixes to the public surface, all on the
options bags. Aside from two small breaking changes on getRoom /
createRoom positional arguments and the pageSize rename, these are
mechanical type-level renames.

1. Options-type naming follows <methodName>Options uniformly

   - `SendMessageOptions`         -> `SendToRoomOptions`
   - `RoomMemberOperationOptions` -> split into `AddUserToRoomOptions`
                                     and `RemoveUserFromRoomOptions`

   The remaining options types (`StartOptions`, `StopOptions`,
   `GetRoomOptions`, `CreateRoomOptions`, `GetUserInfoOptions`,
   `ListRoomMessagesOptions`) already matched the convention. The
   base `OperationOptions` is unchanged.

   `sendToConversation` is private; its parameter is typed as the
   base `OperationOptions` rather than introducing a non-exported
   `SendToConversationOptions` alias. `manageRoomMember` (private)
   likewise.

2. Move trailing optional positional parameters into the options bag

   - `getRoom(roomId, withMembers, options?)`
     -> `getRoom(roomId, options?: { withMembers?, abortSignal? })`
     `withMembers` is now optional and defaults to `false` (saves the
     extra round-trip unless the caller asks).

   - `createRoom(title, members, roomId?, options?)`
     -> `createRoom(title, members, options?: { roomId?, abortSignal? })`
     `roomId` moves into the options bag alongside `abortSignal`.

   Rationale: a long tail of optional positionals at the end of a
   signature is hard to read at call sites and forces callers to pass
   `undefined` placeholders when they only want the last argument.
   Both fields now live next to `abortSignal`, which is the existing
   options-bag convention for the rest of the SDK.

3. `ListRoomMessagesOptions.pageSize` -> `maxPageSize`

   Matches the `@azure/core-paging` `PageSettings.maxPageSize`
   convention already used by `listRoomMessages(...).byPage({
   maxPageSize })`. The same name now applies at both the iterator
   level (default round-trip size) and the per-page override, so the
   two knobs stop looking like unrelated concepts.

Call-site updates

   - `tests/integration.test.ts`: every `createRoom(title, members,
     roomId)` rewritten to `createRoom(title, members, { roomId })`,
     the single `getRoom(roomId, true)` rewritten to `getRoom(roomId,
     { withMembers: true })`, and the one `listRoomMessages({ ...,
     pageSize: 100 })` rewritten to `{ ..., maxPageSize: 100 }`.
   - Internal `ensureRoomCached` no longer passes the now-removed
     `withMembers: false` positional - the default does it.
   - `examples/quickstart/client.js` only used the no-roomId form and
     does not call `getRoom` / pass `pageSize`, so no change needed.
   - `README.md`: methods table updated for the new signatures and
     the `pageSize` -> `maxPageSize` rename.

Verification

   - tsc --noEmit clean.
   - `tsx --test tests/lifecycle.test.ts`: 6/6 pass.
   - `api-extractor run --local`: zero warnings; regenerated
     `review/web-pubsub-chat-client.api.md` reflects exactly the
     renames + the two field additions
     (`GetRoomOptions.withMembers?`, `CreateRoomOptions.roomId?`) +
     `ListRoomMessagesOptions.pageSize?` -> `maxPageSize?`.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two orthogonal cleanups on the event-emitter surface, both
flagged by the api-extractor report.

1. `MessageEvent` -> `ChatMessageEvent`

   The local `MessageEvent` collided with the DOM `MessageEvent`
   interface from `lib.dom.d.ts` (the type of "message" events on
   `EventTarget` / `WebSocket` / `Window`, a different shape).
   api-extractor was suffixing ours to `MessageEvent_2` and
   re-exporting it under the unsuffixed name, which made the report
   noisy and the autocomplete misleading. Renaming makes the
   collision go away and makes the type self-describe (a chat
   message event, not a DOM message event).

2. Drop `Disposable`; all `on*()` listeners return `void`

   The local `Disposable` collided with the TypeScript 5.2+ built-in
   `Disposable` interface used by the explicit-resource-management
   proposal (`{ [Symbol.dispose](): void }`) - a different shape
   from our callable `() => void`, with all the same api-extractor
   noise as `MessageEvent`.

   Rather than rename to something like `Unsubscribe`, we dropped
   the unsubscribe-by-return-callable pattern entirely and aligned
   the chat-event API with the EventEmitter-style add/remove used by
   the underlying `WebPubSubClient`:

       // node_modules/@azure/web-pubsub-client/.../webPubSubClient.d.ts
       on(event: "connected", listener: ...): void;
       off(event: "connected", listener: ...): void;

   Three reasons:

   a) The chat `on*()` family already had a paired `off(event, cb)`.
      Returning an unsubscriber on top of that gave two unsubscribe
      paths and an implicit "which one is canonical?" choice.

   b) Users routinely write `client.on(...)` next to
      `client.connection.on(...)` (connection-lifecycle events live on
      the underlying client). Having the two surfaces use different
      idioms - returned callable vs `off(event, cb)` - is a real
      surprise. Single idiom across both makes the package
      consistent.

   c) No internal caller, no test, no example actually captured the
      returned callable. The pattern existed only as documentation;
      removing it costs us nothing observable.

   The chat event API now mirrors the connection event API one-to-one:

       client.on(event, cb)   client.connection.on(event, cb)
       client.off(event, cb)  client.connection.off(event, cb)

Surface changes

   - `src/events.ts`: `MessageEvent` -> `ChatMessageEvent`;
     `Disposable` type deleted.
   - `src/chatClient.ts`: `event: MessageEvent` -> `ChatMessageEvent`
     (two construction sites); `on()` no longer returns a callable
     (just `this._emitter.on(...)`); all five `on<Event>` convenience
     methods return `void`; JSDoc example updated to the
     `on()`/`off()` paired form.
   - `src/index.ts`: drop `Disposable` re-export and the (briefly
     introduced) `Unsubscribe` rename; export `ChatMessageEvent`.
   - `README.md`: events-listeners section rewritten around
     `on(event, cb)` / `off(event, cb)`; events table updated to
     `ChatMessageEvent`.
   - `review/web-pubsub-chat-client.api.md`: regenerated; no more
     `_2` aliases, no more `Unsubscribe` / `Disposable` type, all
     six `on*()` methods now show `: void`.

Tests do not reference any of the renamed / removed names.

Verification

   - tsc --noEmit clean.
   - `tsx --test tests/lifecycle.test.ts`: 6/6 pass.
   - `api-extractor run --local`: zero warnings; zero `_2` matches in
     the regenerated report.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@vicancy vicancy force-pushed the api-v1-p0 branch 2 times, most recently from b306a03 to 8099487 Compare May 28, 2026 04:17
The instance `ChatClient.start(options?: StartOptions)` accepts an
`abortSignal` and propagates it through `startCore` to
`connection.start` and the login/get-room invocations, so a stuck
start can be cancelled cleanly. The static `ChatClient.start(...)`
helper - which constructs a new `ChatClient` and starts it in one
step - was left calling `chatClient.start()` with no arguments, so
there was no way to cancel a one-shot construct-and-start.

This was an oversight from the original AbortSignal pass (commit
`515f5f2`), not a deliberate omission: every other public async
method already accepts its `<MethodName>Options` bag, so the static
helper diverging here is a small but real inconsistency.

Why two parameters instead of an intersection?

The first iteration of this fix exposed
`options?: WebPubSubClientOptions & StartOptions` on the URL /
credential overloads, so a single bag could carry both transport
config and a cancellation token. That works today but is fragile:

  - `WebPubSubClientOptions` ships from a separate package
    (`@azure/web-pubsub-client`) on a separate release cadence. If
    it ever gains a field that overlaps with `StartOptions`
    (`abortSignal` being the obvious one), the intersection
    silently changes shape - in the worst case, a field's type
    narrows to the upstream variant and breaks callers passing the
    polyfilled type.
  - Upstream itself draws this exact line: `WebPubSubClientOptions`
    holds transport config (protocol, autoReconnect, retry options,
    keep-alive intervals) while a separate `StartOptions` interface
    in the same package holds `abortSignal`. Our intersection
    deliberately blurred a boundary upstream chose to draw.

`WebPubSubClientOptions` has too many genuinely useful fields
(`autoReconnect`, `autoRejoinGroups`, `messageRetryOptions`,
`reconnectRetryOptions`, keep-alive intervals, `protocol`) to drop
silently from the static helper, so the fix is to give it its own
positional parameter on the URL / credential overloads, slotted in
between the connect target and the start options:

    static start(clientAccessUrl, webPubSubClientOptions?: WebPubSubClientOptions, options?: StartOptions): Promise<ChatClient>
    static start(credential,      webPubSubClientOptions?: WebPubSubClientOptions, options?: StartOptions): Promise<ChatClient>
    static start(wpsClient,                                                        options?: StartOptions): Promise<ChatClient>

The `wpsClient` overload only takes `StartOptions` since transport
construction has already happened; passing a `WebPubSubClient`
that's already configured replaces the `webPubSubClientOptions`
parameter.

Implementation forwards `abortSignal` explicitly to
`chatClient.start({ abortSignal })` rather than the whole options
bag, so future StartOptions-only fields are picked up cleanly
without leaking unrelated fields into the start path.

README: the static-methods table annotates each row with the new
parameter shape (`webPubSubClientOptions?: WebPubSubClientOptions`,
`options?: StartOptions`) so readers know exactly what each
positional means.

Verification

   - tsc --noEmit clean.
   - lifecycle.test.ts: 6/6 pass.
   - api-extractor: zero warnings; regenerated report shows the
     URL / credential overloads with `webPubSubClientOptions?` and
     `options?: StartOptions` as separate parameters.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR 1 has progressively pulled every public mention of the
"conversation" concept below the room abstraction:

  - `sendToConversation()` was made `private` in `515f5f2`.
  - The `_conversationIds` set on `ChatClient` was made `private`
    (was leaking via `protected`) in `a9e31a5`.
  - The `LOGIN` invocation's `conversationIds` array stays internal
    state; nothing in the public surface references it.

This commit finishes the job on the last remaining public reference
- `ChatMessageEvent` - by dropping `conversationId` outright and
tightening `roomId` from optional to required.

Drop `conversationId`

From the user's mental model the conversation isn't a thing they
manage: they create / join rooms, they send to rooms, they list
room messages. The `roomId` on the event already tells them
everything they can act on. The `conversationId` is a wire-protocol
detail that we resolve to a room internally before invoking server
events; leaking it on the event surface forces clients to either
ignore it (most of them) or build their own resolver against it
(none of them do today).

The drop was originally tagged in the earlier session plan as
"deferred to model PR" because it sat in the same bucket as the
rest of the conversation-concept hiding work. But that work
finished in `515f5f2` / `a9e31a5`; the only remaining
`ChatMessageEvent` quality fix that genuinely needs the model
rewrite is the sender-echo synthesis (the `bodyType: "text"`
hardcode and the `createdAt` fakery), which touches the
`ChatMessage` shape. Dropping the field is purely subtractive and
has zero coupling to that work.

Tighten `roomId` from `roomId?: string` to `roomId: string`

The previous version was `roomId?: string` with the doc string
"Undefined for non-room conversations (currently unused)". Two
problems with that:

  - "non-room conversations" reintroduces the very concept the drop
    above is trying to hide. The doc was leaking through the back
    door what the type change had just closed off the front.
  - "currently unused" is hand-wavy. If a case is unused, the field
    shouldn't be modelled as optional at all - that is just
    defensive `undefined`-everywhere coding masquerading as
    protocol nuance.

In practice `roomId` can only be undefined in two anomalous paths:

  a. Notification path. The wire `body.conversation.roomId` is
     `string | null` per the generated types, but in practice
     always populated today - the null is a future-protocol
     reservation, not a real case.

  b. Sender-echo path. `_rooms` is consulted via reverse lookup
     (`defaultConversationId -> roomId`) immediately after the
     forward lookup in `sendToRoom`. A miss is only possible if a
     `RoomLeft` notification arrives during the in-flight invoke
     call, leaving the room out of the cache. Narrow race window.

For both cases the fix is the same: log a warning and skip the
emit. Subscribers get a tight contract (every `ChatMessageEvent`
has a `roomId` they can act on) and the anomalous cases are
contained inside the SDK rather than pushed onto every caller as a
`roomId?` they have to null-check defensively.

In the sender-echo path the function still returns the `msgId` -
the send itself succeeded server-side; only the local echo is
suppressed. Suppressing is the right call: emitting a
"message in room you just left" event would be more confusing than
silence.

Changes

  - `src/events.ts`: drop `conversationId: string;` from
    `ChatMessageEvent`; change `roomId?: string;` to
    `roomId: string;`.

  - `src/chatClient.ts`:
    * Notification path (`MessageCreated`): guard
      `body.conversation.roomId`, warn-and-skip if missing.
    * Sender-echo path (`sendToConversation`): the existing warning
      for failed reverse lookup now returns early instead of
      proceeding with a `roomId: undefined` event. Send result is
      still returned.

  - `review/web-pubsub-chat-client.api.md`: regenerated; one field
    removed, one optional turned required.

Verification

  - tsc --noEmit clean.
  - lifecycle.test.ts: 6/6 pass.
  - api-extractor: zero warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
vicancy and others added 2 commits May 28, 2026 16:30
The public isStarted getter was a thin wrapper over the private
_isStarted backing field, intended as a "is it safe to call methods?"
check. In practice it adds no value and a little risk:

- TOCTOU race. `if (client.isStarted) { await client.getRoom(...) }`
  gives a false sense of safety: a stop() from another caller or a
  network drop between the check and the call still throws. The check
  cannot be made meaningful without a lock that the SDK does not own.

- Redundant with the exception path. Every operation method already
  calls ensureStarted() and throws ChatError(NotStarted). Callers must
  handle that exception anyway, so the proactive check pays no rent.

- No sibling precedent. The underlying WebPubSubClient exposes no
  equivalent state-check getter; it is exception-driven throughout.

- Zero internal callers. All in-package references already read
  this._isStarted directly.

userId is left as-is: it is a getter-only property, which TypeScript
renders to consumers as effectively `readonly userId: string` (no
setter exists; assignment is a compile error). The lazy throw-on-not-
started guard inside the getter is preferable to a `string | undefined`
field, since it forces callers to call start() first via a clear
runtime error rather than requiring null-check at every call site.

Changes:
- Drop the public `isStarted` getter from ChatClient (the private
  `_isStarted` field is unchanged).
- Update lifecycle tests to assert on the internal flag via a small
  typed helper `isStarted(client)`. These are own-package smoke tests
  exercising the state machine; reaching past `private` here keeps the
  test intent intact without re-exposing the API on the public surface.
- Drop the `isStarted` row from the README properties table; reword
  the `userId` row to note it is read-only.
- Regenerate review/web-pubsub-chat-client.api.md (one-line removal).

Verified: tsc clean, 6/6 lifecycle tests pass, api-extractor report
regenerated without warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…, On<X>Args, kebab-case)

Align the ChatClient event surface with the underlying
@azure/web-pubsub-client so the two sit cleanly side by side and the
two `client.on(...)` calls (chat-domain vs connection-lifecycle) feel
identical.

Before:

    on<K extends ChatEventName>(event: K, callback: ChatEventListener<K>): void;
    off<K extends ChatEventName>(event: K, callback: ChatEventListener<K>): void;
    onMessage(cb), onRoomJoined(cb), onRoomLeft(cb), onMemberJoined(cb), onMemberLeft(cb)

    // 3 supporting types + 5 *Event payload types exported

After (matches `WebPubSubClient.on(event: "server-message", listener: (e: OnServerDataMessageArgs) => void)`):

    on(event: "message",       listener: (e: OnMessageArgs)      => void): void;
    on(event: "room-joined",   listener: (e: OnRoomJoinedArgs)   => void): void;
    on(event: "room-left",     listener: (e: OnRoomLeftArgs)     => void): void;
    on(event: "member-joined", listener: (e: OnMemberJoinedArgs) => void): void;
    on(event: "member-left",   listener: (e: OnMemberLeftArgs)   => void): void;
    // + same 5 overloads of off(...)

Three coupled changes:

1. Explicit overloads replace the generic indexed-map shape. Drops
   `ChatEventMap`, `ChatEventName`, `ChatEventListener<K>` from the
   public surface — these only existed to back the generic.

2. Event payload types renamed to `On<Event>Args` to mirror upstream
   (`OnConnectedArgs`, `OnGroupDataMessageArgs`, ...):
     ChatMessageEvent   -> OnMessageArgs
     RoomJoinedEvent    -> OnRoomJoinedArgs
     RoomLeftEvent      -> OnRoomLeftArgs
     MemberJoinedEvent  -> OnMemberJoinedArgs
     MemberLeftEvent    -> OnMemberLeftArgs

3. Event-name string literals switch from camelCase to kebab-case so
   they read identically alongside `client.connection.on("server-
   message", ...)`. `"message"` is unchanged; the rest become
   `"room-joined"`, `"room-left"`, `"member-joined"`, `"member-left"`.

4. Convenience helpers (`onMessage(cb)`, `onRoomJoined(cb)`, etc.)
   removed. Upstream has no equivalent, and they had an asymmetric-
   removal smell — there was never a paired `offMessage(cb)`, so a
   caller still had to use `off("message", cb)` to unregister. Net
   result: one entry point per event in each direction, all listed
   explicitly in the api report.

Updated all internal emit() sites in chatClient.ts to use the new
kebab-case names and the renamed argument types. README event-listener
section reworked to drop the convenience-method column. Integration
tests migrated from `chat.onMessage(cb)` / `chat.onRoomJoined(cb)`
to `chat.on("message", cb)` / `chat.on("room-joined", cb)`.

Verified: tsc clean, 6/6 lifecycle tests pass, api-extractor report
regenerated without warnings; all 10 on/off overloads (5 each) and
the 5 OnXArgs interfaces appear cleanly in review/web-pubsub-chat-
client.api.md, with `ChatEventMap` / `ChatEventName` / `ChatEventListener`
gone.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread sdk/webpubsub-chat-client/review/web-pubsub-chat-client.api.md Outdated
…stopped chat events

Two coupled API-shape changes:

1. Drop the URL and credential constructor overloads. The static
   `ChatClient.start(url, ...)` / `start(credential, ...)` factories
   already construct the underlying `WebPubSubClient` and atomically
   start the chat client, so the URL/credential constructors were a
   footgun: they left callers with a half-constructed `ChatClient`
   that threw `ChatError(NotStarted)` on every method call until the
   instance `start()` was awaited. After this change:

       new ChatClient(wpsClient: WebPubSubClient)                         // only constructor
       ChatClient.start(url,        wpsOpts?,  startOpts?): Promise<...>  // builds & starts
       ChatClient.start(credential, wpsOpts?,  startOpts?): Promise<...>  // builds & starts
       ChatClient.start(wpsClient,             startOpts?): Promise<...>  // starts existing

   Internally the static URL/credential branches now `new
   WebPubSubClient(...)` first and pass the result to `new
   ChatClient(wpsClient)`, so there is exactly one transport
   construction path.

2. Add chat-domain "started" and "stopped" events. The pre-existing
   `client.connection.on("connected", ...)` / `connection.on("stopped",
   ...)` events fire at the transport boundary and miss the chat-
   domain transition: `connected` fires before LOGIN + room hydration
   complete, so a UI that flips to a "ready" state on `connected`
   will still hit `ChatError(NotStarted)` on operations. The new
   events fire on the chat-domain transition:

       on(event: "started", listener: (e: OnStartedArgs) => void): void;
       on(event: "stopped", listener: (e: OnStoppedArgs) => void): void;

   `OnStartedArgs` carries `{ userId }` — mirrors the upstream
   `OnConnectedArgs.userId` pattern and lets handlers consume the
   chat-domain identity without re-reading `client.userId`.
   `OnStoppedArgs` is empty, matching the upstream `OnStoppedArgs`
   shape exactly.

   Emission rules (single transition guarantee):
   - "started" fires exactly once at the end of a successful
     `startCore()`, after `_isStarted` has been set to true and
     `_userId` / `_rooms` are live. Re-calling `start()` on an already-
     started client is a no-op and does not re-emit.
   - "stopped" fires from inside `resetState()`, guarded on a local
     `wasStarted = this._isStarted` snapshot taken on entry. This
     guarantees one emission per started→not-started transition,
     covering both explicit `stop()` and transport-driven termination
     (the `connection.on("stopped")` callback we install in the
     constructor). The reset paths that run before `_isStarted` was
     ever set to true (the pre-start `resetState()` at the top of
     `startCore()` and the post-failure rollback) do not emit.

Tests:
- Two new lifecycle.test.ts cases exercise the emission rules:
  "started event fires once after start() completes with userId
  payload" (covers single fire + payload shape + no re-emit on
  no-op re-start), and "stopped event fires on started→not-started
  transitions only" (covers stop()-before-start() no-emit, explicit
  stop() emit, and transport-driven stop emit).
- All 8 lifecycle tests pass; tsc clean; api-extractor report
  regenerated cleanly. The public surface now shows a single
  `constructor(wpsClient: WebPubSubClient)` and 7 `on`/`off`
  overloads each (5 chat events + started + stopped).

README updated:
- Constructor section reduced to a single signature with a pointer
  to `ChatClient.start(...)` for URL/credential construction.
- Event listeners table gains `started` (`OnStartedArgs`) and
  `stopped` (`OnStoppedArgs`) rows at the top.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
// (undocumented)
readonly connection: WebPubSubClient;
createRoom(title: string, members: string[], options?: CreateRoomOptions): Promise<RoomInfoWithMembers>;
getRoom(roomId: string, options?: GetRoomOptions): Promise<RoomInfoWithMembers>;

@xingsy97 xingsy97 May 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

rename to getRoomDetail before merge

Comment thread sdk/webpubsub-chat-client/review/web-pubsub-chat-client.api.md Outdated
Comment thread sdk/webpubsub-chat-client/review/web-pubsub-chat-client.api.md Outdated
…ssages roomId, drop StopOptions)

- Rename getUserInfo -> getUserProfile and GetUserInfoOptions ->
  GetUserProfileOptions so the method name matches its UserProfile return
  type (resolves PR Azure#892 review comment). The generated UserProfile type
  is left untouched to stay aligned with the OpenAPI schema.
- listRoomMessages(options) -> listRoomMessages(roomId, options?); the
  required roomId is now a positional parameter (consistent with the
  other room-scoped methods) and is removed from ListRoomMessagesOptions.
- Remove StopOptions and the stop() options parameter. The underlying
  WebPubSubClient.stop() is not cancellable, so the advertised abortSignal
  could never be honored; dropping it avoids a misleading API surface.
- Regenerate the API report and update README, quickstart example, and
  integration tests accordingly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@vicancy vicancy merged commit 2ecfcb6 into Azure:main Jun 9, 2026
6 checks passed
@vicancy vicancy deleted the api-v1-p0 branch June 9, 2026 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants