Skip to content

feat(data-connect): Migrate streaming to use SubscribeObserver and further harden errors / disconnects#9815

Merged
stephenarosaj merged 11 commits intopasta/mainfrom
pasta/error-hardening
Apr 7, 2026
Merged

feat(data-connect): Migrate streaming to use SubscribeObserver and further harden errors / disconnects#9815
stephenarosaj merged 11 commits intopasta/mainfrom
pasta/error-hardening

Conversation

@stephenarosaj
Copy link
Copy Markdown
Contributor

@stephenarosaj stephenarosaj commented Apr 7, 2026

Description

✨ This PR migrates the internal streaming transport notification system to the SubscribeObserver pattern, simplifies error handling, and refactors REST routing fallback logic.

Changes

  • Observer Pattern: Migrated from functional hooks to SubscribeObserver interface in QueryManager and transport layers (no functionality change from user perspective - just changed communication scheme between query layer + transport layer).
  • REST Routing: Simplified fallback to REST by checking executeShouldUseStream() in catch blocks instead of checking isUnableToConnect directly.
  • Connection State: Replaced isUnableToConnect getter with a property, updating it on WebSocket open/error and adding checks in TransportManager.
  • Error Handling: Removed WebSocketDataConnectError and custom close codes, and refactored rejectAllActiveRequests to use standard codes.
  • Cleanup: Refactored stream teardown and disconnect logic, moving disconnect handling to onStreamClose.
  • Hardening: Guarded WebSocket initialization in Node and truncated close reasons to 123 bytes to comply with the spec.

Testing

  • Updated test/unit/streamTransport.test.ts and test/unit/streaming.test.ts for the observer pattern.
  • Updated test/unit/websocketTransport.test.ts and test/unit/transportManager.test.ts for error handling and routing.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 7, 2026

⚠️ No Changeset found

Latest commit: 7d32f25

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the subscription mechanism by replacing the SubscribeNotificationHook callback with a SubscribeObserver interface across the QueryManager and transport layers. This change improves the handling of data updates, disconnections, and errors. The PR also adds a WebSocket availability check for Node.js environments. Feedback includes suggestions to support and await asynchronous processing in the observer to prevent race conditions, refining error construction in the QueryManager, and addressing potential crashes due to WebSocket protocol limits for reason strings.

Comment thread packages/data-connect/src/network/transport.ts Outdated
Comment thread packages/data-connect/src/network/stream/streamTransport.ts Outdated
Comment thread packages/data-connect/src/core/query/QueryManager.ts Outdated
Comment thread packages/data-connect/src/network/stream/websocket.ts
@stephenarosaj stephenarosaj marked this pull request as ready for review April 7, 2026 16:23
@stephenarosaj stephenarosaj requested review from a team and dconeybe as code owners April 7, 2026 16:24
@stephenarosaj
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the subscription mechanism in the Data Connect SDK, replacing the SubscribeNotificationHook with a more robust SubscribeObserver interface that includes explicit handlers for data, disconnections, and errors. It also improves WebSocket handling by ensuring close reasons are truncated to the protocol-defined limit and adds environment checks for WebSocket availability in Node.js. Feedback focuses on improving error propagation by extracting specific error codes from stream notifications, providing better default error messages for WebSocket closures, and preserving original error details during message parsing.

Comment thread packages/data-connect/src/core/query/QueryManager.ts Outdated
Comment thread packages/data-connect/src/network/stream/websocket.ts Outdated
Comment thread packages/data-connect/src/network/stream/websocket.ts
Comment thread packages/data-connect/src/network/stream/websocket.ts
@stephenarosaj
Copy link
Copy Markdown
Contributor Author

Firestore tests keep failing... I'm merging anyways. I believe they are just flaky

@stephenarosaj stephenarosaj merged commit 13c9e4f into pasta/main Apr 7, 2026
33 of 37 checks passed
@stephenarosaj stephenarosaj deleted the pasta/error-hardening branch April 7, 2026 23:12
@stephenarosaj stephenarosaj changed the title feat(data-connect): Migrate streaming to use SubscribeObserver and harden errors / disconnects feat(data-connect): Migrate streaming to use SubscribeObserver and further harden errors / disconnects Apr 8, 2026
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.

3 participants