Skip to content

feat(sdk): add new managerOptions optional param on MessageBoxClient - #490

Open
kjartan221 wants to merge 3 commits into
bsv-blockchain:mainfrom
kjartan221:feat/message-box-manager-options
Open

feat(sdk): add new managerOptions optional param on MessageBoxClient#490
kjartan221 wants to merge 3 commits into
bsv-blockchain:mainfrom
kjartan221:feat/message-box-manager-options

Conversation

@kjartan221

Copy link
Copy Markdown
Collaborator

feat(message-box-client): forward socket.io manager options to AuthSocketClient


Draft until the checks on the exact head are terminal. Boxes below are checked
only where the evidence is in hand; CI-dependent items are intentionally left
unchecked.

Motivation for this change

MessageBoxClient reaches a server two ways. sendMessage, listMessages, and
acknowledgeMessage go over BRC-31 authenticated HTTP via authFetch.fetch.
initializeConnection, listenForLiveMessages, and sendLiveMessage go over
socket.io via AuthSocketClient. Only the second path is affected here.

initializeConnection constructs its socket like this:

this.socket = AuthSocketClient(targetHost, {
  wallet: this.walletClient,
  originator: this.originator
})

AuthSocketClient accepts more than that. Its options include managerOptions
(socket.io's own Partial<ManagerOptions & SocketOptions>), which it passes
straight through to io():

const socket = realIo(url, opts.managerOptions)

MessageBoxClient does not forward it, and MessageBoxClientOptions has no
socket, manager, or transport field — so there is no route from a caller down to
it. Every client therefore gets socket.io's default
transports: ['polling', 'websocket']: establish over Engine.IO HTTP
long-polling, then upgrade to WebSocket.

That default is not universally viable. Whether the polling transport completes
depends on the deployment in front of the socket server — reverse proxies,
ingress rules, and CDN layers vary in how they treat Engine.IO's polling
requests, and socket.io's own documentation recommends
transports: ['websocket'] for environments where polling is not carried
cleanly. Today a MessageBoxClient caller has no way to express that, regardless
of what they know about the host they are talking to.

This change makes the client host-agnostic on that axis: transport selection
becomes a caller decision, so a MessageBoxClient works against any deployment
running the AuthSocket backend rather than only those whose fronting
infrastructure happens to suit socket.io's defaults. It is additive, and clients
that pass nothing behave exactly as they do now.

Change

Three source lines of behavior, plus documentation and release metadata.

src/types.ts — new option on MessageBoxClientOptions, typed by borrowing from
the dependency rather than adding one:

managerOptions?: AuthSocketClientOptions['managerOptions']

This keeps socket.io-client out of @bsv/message-box-client's dependency list
(the type arrives through @bsv/authsocket-client, already a dependency) and
makes the option incapable of drifting from what AuthSocketClient accepts.

src/MessageBoxClient.ts — stored on a private readonly field, destructured in
the constructor, and forwarded at the construction site so an unconfigured client
passes no managerOptions key at all:

this.socket = AuthSocketClient(targetHost, {
  wallet: this.walletClient,
  originator: this.originator,
  ...(this.managerOptions !== undefined && { managerOptions: this.managerOptions })
})

Usage:

const client = new MessageBoxClient({
  walletClient,
  host: 'https://messagebox.example',
  managerOptions: { transports: ['websocket'] }
})
await client.listenForLiveMessages({ messageBox: 'inbox', onMessage })

Backward compatible: the option is optional, omitted by default, and touches only
the socket path. The HTTP methods are untouched.

Open for maintainer input

Two decisions here are deliberate but genuinely open. If you would prefer
either changed, say so and I will add a commit to this PR rather than argue
it.

  1. The name managerOptions is up for debate. It mirrors
    AuthSocketClientOptions.managerOptions exactly, so the same name appears at
    both layers and greps across them — that was the reasoning. The fair
    criticism is that it leaks socket.io vocabulary into an API whose users may
    never have heard of an Engine.IO Manager. A narrower transports?: string[],
    or a domain-named socketOptions, would read better to a normal caller at the
    cost of needing revisiting for the next socket.io knob. Happy to rename.

  2. Placement on the constructor rather than as an initializeConnection
    parameter.
    initializeConnection already takes overrideHost, so there is
    precedent for the other choice. Reasons we went with the constructor:

    • initializeConnection is mostly reached implicitly — from joinRoom and
      listenForLiveMessages, and via sendLiveMessage. A live-message consumer
      calls listenForLiveMessages, not initializeConnection, so a parameter
      there would need threading through three more signatures to be reachable at
      all. overrideHost already pays that tax.
    • The socket is constructed once, under if (this.socket == null). A per-call
      option would be silently discarded on every later call. overrideHost has
      this behavior today: if a socket already exists — even an unauthenticated
      one — the construction block is skipped and overrideHost is ignored.
    • socket.io's Manager retains these options for the life of the connection,
      including automatic reconnects, so connection-lifetime scope matches the
      layer beneath; a call parameter would advertise per-call granularity it
      cannot deliver.

    The honest downside, for the record: managerOptions is the only option on
    the interface that can be entirely inert — pass it, use only the HTTP methods,
    and nothing reads it. The JSDoc says so explicitly.

Impact

  • No public package source or manifest changed
  • Public package source or manifest changed; affected packages are listed below
  • Infrastructure source, dependency, image, or deployment configuration changed
  • Public API, exports, types, runtime targets, or browser/mobile behavior changed
  • Security-sensitive boundary changed
  • Documentation or examples changed

Affected packages and intended versions (publication occurs only through the
release workflow after approval):

Package Published baseline This PR Classification
@bsv/message-box-client 2.4.0 2.5.0 minor

2.4.1 → 2.5.0 via npm version minor --no-git-tag-version. Minor per
docs/about/versioning.md"MINOR adds backward-compatible behavior or public
API"
— and per this package's own history, where every feat took a minor
(2.2.6 → 2.3.0, 2.3.0 → 2.4.0) and refactor/lint/fix work took patches.
governance/package-release-notes.json is updated to releaseType: "minor"
accordingly; its summary now covers both this addition and the BRC-29 work
already in the unreleased 2.4.1 window, since the note spans 2.4.0 → 2.5.0.

On the unchecked security box: transport selection does not weaken the BRC-103
mutual authentication AuthSocketClient performs, the option is opt-in, and no
auth, authorization, or validation path changed. Flagging that it was considered
rather than leaving a bare unchecked box.

Verification

Node v24.19.0, pnpm 10.33.2, run in packages/messaging/message-box-client:

Command Result
npm test 9 suites, 214 tests passed
npm run typecheck (tsc --project tsconfig.typecheck.json) clean, exit 0
npm run lint (oxlint --deny-warnings) clean, exit 0
prettier --check on all six changed files clean
pnpm install --filter '@bsv/message-box-client...' lockfile unchanged

Two tests were added, both against the @bsv/authsocket-client module boundary —
the same technique and the same { transports: ['websocket'] } fixture that
authsocket-client's own suite uses for this call
(expect(mockIo).toHaveBeenCalledWith('https://example.test', managerOptions)):

  • Forwards managerOptions to AuthSocketClient when configured — written first
    and watched fail against the old behavior, which received only
    {originator, wallet}.
  • Omits managerOptions from AuthSocketClient when not configured — guards the
    default transport-negotiation path.

Both were mutation-checked rather than taken on trust:

Mutation applied to the production code Caught by
Unconditional spread (managerOptions: this.managerOptions) the "Omits" test
Hardcoded managerOptions: { transports: ['polling'] } both tests

Live-socket evidence: AuthSocketClient with
managerOptions: { transports: ['websocket'] } connects and authenticates
against a MessageBox deployment running the AuthSocket backend — verified
independently, outside this repo, including with a plain @bsv/sdk ProtoWallet
(no funding, no certificates, no wallet-toolbox). This PR's own test suite does
not open a live socket
; it proves the option reaches AuthSocketClient, not
that a given host accepts it. Stated plainly so the coverage is not overread.

  • Conformance evidence: not applicable — no wire format, encoding,
    serialization, or persisted schema changed.

  • Coverage delta: two added tests over previously uncovered forwarding
    behavior; no lines removed from coverage.

  • Lint/typecheck delta: none — both clean before and after.

  • Browser/mobile/packed-consumer evidence: not run locally. No runtime target
    or export surface changed; the addition is a type-only widening of an existing
    options interface plus one object spread.

  • Performance or bundle-size delta: none expected — no new dependency and no
    new runtime import (AuthSocketClientOptions is import type and erases).

  • I self-reviewed the complete diff for correctness, security,
    compatibility, public API, artifacts, dependencies, docs, and operations

  • All applicable checks are terminal and successful on the exact head; any
    scope-based skip is expected and validated by the merge gate

Two open items, stated rather than left to be found

  1. docs/reference/package-api-migrations.md still needs regenerating for the
    version bump. It could not be regenerated on a Windows workstation:
    scripts/package-documentation.mjs builds doc links with relative() from
    node:path, so on win32 it emits backslash paths and rewrites all 31 rows.
    That output was reverted rather than committed, and the file was deliberately
    not hand-edited. This is pre-existing —
    node scripts/package-documentation.mjs --check fails identically against a
    pristine HEAD with none of these changes present. One pnpm docs:packages
    run on Linux fixes it; happy for a maintainer to push that onto this PR.
  2. pnpm health:check and the repo-wide lint / format:check / typecheck
    were not run locally — only the affected package's checks. (format:check
    reports 31 pre-existing failures in this package on files this PR does not
    touch; left alone to keep the diff scoped.)

Security and dependencies

  • No dependency or lockfile change
  • Changelog, runtime relevance, peer compatibility, transitive graph, and
    audit results were reviewed
  • CodeQL/negative tests cover any changed trust boundary
  • The exact-head CodeQL analysis has no new alert
  • The exact-head repository quality gate reports zero new Sonar findings
  • No new override, advisory dismissal, quality suppression, or skipped test
  • Any temporary exception is registered with owner, evidence, review date,
    and removal condition — none added
  • Workflow permissions and lifecycle-script behavior remain least privilege —
    no workflow or lifecycle script touched

No dependency was added: managerOptions is typed via
AuthSocketClientOptions['managerOptions'] from @bsv/authsocket-client, which
is already a workspace:^ dependency, specifically so socket.io-client did not
have to become a direct dependency of this package. pnpm install reported the
lockfile already up to date. The CodeQL and Sonar boxes are unchecked because
they can only be evidenced on a pushed head.

Dependency evidence

No dependency was added, removed, or re-ranged. This section is completed because
the version bump touches a package.json, which the dependency-evidence check
treats as a dependency file.

  • Release notes and necessity: not a dependency update. The only package.json
    change is the 2.4.1 → 2.5.0 version bump required by the additive public API
    in this PR; no dependencies, devDependencies, peerDependencies, or
    engines field was modified.
  • Runtime, build, and peer compatibility: unchanged. No dependency range moved,
    so the resolved runtime, build, and peer graphs are identical to base.
  • Deduplicated lockfile: pnpm-lock.yaml is unmodified.
    pnpm install --filter '@bsv/message-box-client...' reported the lockfile
    already up to date, so no duplication was introduced.
  • Audit and CodeQL: no new package enters the graph, so the advisory surface is
    unchanged from base; CodeQL analysis runs against the PR head.
  • Package and consumer tests: npm test in the affected package passes 9 suites
    and 214 tests; npm run typecheck and npm run lint are both clean.
  • Bundle and performance impact: unchanged. The new type is imported with
    import type and erases at compile time, adding no runtime import and no
    transitive dependency.
  • Affected public package versions: @bsv/message-box-client 2.4.1 → 2.5.0
    (minor), classified minor in governance/package-release-notes.json.

Release and operations

  • No npm publication was performed from a workstation or from this PR
  • Required version bumps are included (2.4.1 → 2.5.0, minor, with the
    matching governance/package-release-notes.json classification)
  • Image/SBOM/provenance/deployment/rollback impact is documented — none; a
    library-only change with no infrastructure or image surface
  • Documentation, changelog, migration, and operational guidance are current

The last box is intentionally unchecked: the package CHANGELOG.md, the JSDoc,
and governance/package-release-notes.json are all updated here, but the
generated docs/reference/package-api-migrations.md is not, for the
Windows-generator reason given under the two open items above. It becomes true
once pnpm docs:packages runs on Linux.

Migration guidance: none required. managerOptions is optional and omitted
when unset, so socket transport negotiation and every HTTP code path behave
exactly as before for existing callers.

Completion evidence

  • The linked tracker is updated only for work fully proved by merged code
  • Review conversations are resolved
  • Documentation, changelog, migration notes, release notes, and operator
    guidance are current or concretely not applicable
  • No pending, failed, stale, cancelled, or unexpectedly skipped check is
    being handed to another contributor as "complete" — the two open items
    (generated doc regeneration, repo-wide gate) are called out explicitly
    above rather than left to be discovered
  • One qualified maintainer approval is sufficient; no last-pusher
    restriction is assumed

@kjartan221 kjartan221 changed the title Add new managerOptions optional param on MessageBoxClient feat(sdk): add new managerOptions optional param on MessageBoxClient Aug 24, 2026
@kjartan221

Copy link
Copy Markdown
Collaborator Author

Following up on my own "Open for maintainer input" point 1 above, before this merges — there may be a better
shape than a top-level managerOptions, and it is much cheaper to change now
than after 2.5.0 publishes.

Rather than exposing one socket.io field, expose the whole AuthSocketClient
surface under one self-labelling parent:

socketOptions?: Omit<AuthSocketClientOptions, 'wallet' | 'originator'>
this.socket = AuthSocketClient(targetHost, {
  ...this.socketOptions,
  wallet: this.walletClient,
  originator: this.originator
})

wallet and originator are excluded because MessageBoxClient already owns
both — originator in particular feeds the WalletClient, AuthFetch, and
every wallet call, so a socket-specific value would be two sources of truth.
Spreading client-owned values last means they always win, and {...undefined}
being a no-op removes the conditional spread this PR currently needs.

That trades one option for full configurability and also makes onError and
requestedCertificates reachable, both of which are otherwise unreachable from
MessageBoxClient today. It answers the naming concern too: socketOptions is
domain-named, and managerOptions keeps socket.io's own name nested under a
parent that explains it.

Happy to push the new shape, or leave this PR as-is if you would rather keep the
change minimal. Which do you prefer?

@sonarqubecloud

Copy link
Copy Markdown

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.

1 participant