Skip to content

refactor(js/net)!: drop transport from ReloadProps - #2665

Merged
kixelated merged 1 commit into
devfrom
claude/reload-props-omit-transport
Aug 6, 2026
Merged

refactor(js/net)!: drop transport from ReloadProps#2665
kixelated merged 1 commit into
devfrom
claude/reload-props-omit-transport

Conversation

@kixelated

@kixelated kixelated commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Reload.#connect has never forwarded ConnectProps.transport to connect(), so a caller who supplied a pre-existing WebTransport silently got a fresh dial instead.
  • The option cannot work here by construction. A supplied session is good for exactly one connection, and the reconnect loop has nothing to reuse after the first drop. connect() remains the entry point for handing over a session.
  • Narrow ReloadProps to omit signal and transport, then explicitly forbid both with optional never members. This rejects both inline object literals and preassembled config objects instead of silently ignoring either field.
  • Bump @moq/net from 0.3.0 to 0.4.0 for the breaking published API change.

Public API changes

  • Breaking: ReloadProps no longer accepts transport.
  • Breaking type tightening: the existing signal exclusion now also rejects preassembled option objects. Current Reload owns the abort signal for each connection attempt.
  • ConnectProps.transport, ConnectProps.signal, and connect() are unchanged.
  • Targets dev per Branch Targeting.

Test plan

  • Type-check and package build passed.
  • Full @moq/net source suite: 354 pass, 0 fail.
  • Pinned Bun 1.3.13 run after building: 708 pass, 0 fail across source and generated output.
  • Required GitHub Check passed on 79cdceeaf in 3m42s.

Cross-package sync

  • Updated js/net/package.json and bun.lock to 0.4.0.
  • doc/lib/js/@moq/net.md never documented transport on Reload, so no docs change is needed.
  • No wire format change.

Interaction with #2655

#2655 targets main and deliberately makes Reload honor a caller-provided signal, while adding a runtime warning for transport as a non-breaking stopgap. When main and dev are reconciled, preserve #2655's signal support and this PR's transport prohibition. Remove #2655's transport warning and its warning-specific test.

(Written by GPT-5)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 152e11119c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread js/net/src/connection/reload.ts
@kixelated
kixelated force-pushed the claude/reload-props-omit-transport branch from 152e111 to 5af2ab5 Compare August 5, 2026 22:17
Reload.#connect has never forwarded ConnectProps.transport, so a caller supplying a session silently got a fresh dial instead. The option cannot work here by construction: a supplied session is good for exactly one connection, so the reconnect loop has nothing to reuse once it drops.

Narrow ReloadProps to exclude signal and transport, with optional never members so both literals and preassembled config objects fail to type-check. Bump @moq/net to 0.4.0 for the breaking API change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kixelated
kixelated force-pushed the claude/reload-props-omit-transport branch from 5af2ab5 to 79cdcee Compare August 6, 2026 01:16
@kixelated
kixelated merged commit 242339c into dev Aug 6, 2026
1 check passed
@kixelated
kixelated deleted the claude/reload-props-omit-transport branch August 6, 2026 01:21
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