Skip to content

FORGE-2185: react-ditto v5 migration follow-ups#99

Merged
Aaron Leopold (aaronleopold) merged 3 commits into
masterfrom
aaronleopold/forge-2185-react-ditto-follow-ups-from-the-v5-migration-review
Jul 6, 2026
Merged

FORGE-2185: react-ditto v5 migration follow-ups#99
Aaron Leopold (aaronleopold) merged 3 commits into
masterfrom
aaronleopold/forge-2185-react-ditto-follow-ups-from-the-v5-migration-review

Conversation

@aaronleopold

@aaronleopold Aaron Leopold (aaronleopold) commented Jul 6, 2026

Copy link
Copy Markdown
Member

Follow-ups from the FORGE-1921 v5 migration review. FORGE-2185

useQuery subscriptions under v5

v5 enables DQL_RESTRICT_SUBSCRIPTION, so registerSubscription rejects ORDER BY/LIMIT/OFFSET. Since useQuery subscribed with the same query it observed, such a query left the hook in an error state with no subscription while the observer kept returning local-only data.

  • Added an optional subscriptionQuery param so callers can subscribe with a sync-appropriate query (ignored when localOnly).
  • A failed subscription no longer overwrites error; it surfaces via onError/console.error instead, so a healthy observer isn't masked.

Removed the "fails to load wasm" provider tests

Both asserted the error element was empty, so they passed trivially. init() in v5 is a process-global singleton, so a bad wasm path is never exercised after the first init and can't be tested here. The provider's error-surfacing is already covered by the setup-failure tests, so these were removed rather than rewritten.

README

Documented the useRemotePeers/useConnectionStatus presence hooks and the v5 subscription constraint

- useQuery: add optional subscriptionQuery param, and stop a failed
  subscription registration from overwriting the hook error state so a
  healthy observer is not masked (v5 DQL_RESTRICT_SUBSCRIPTION rejects
  ORDER BY/LIMIT/OFFSET in subscriptions)
- remove the trivially-passing "fails to load wasm" provider tests: init()
  is a process-global singleton so a bad wasm path is never exercised after
  the first init, and provider error-surfacing is already covered by the
  setup-failure tests
- README: document the useRemotePeers/useConnectionStatus presence hooks and
  the v5 subscription constraint
Post-review cleanup: document that error reflects only observer setup (not
subscription registration), and extract a reportError helper so the observer
(setError + report) vs subscription (report only) failure semantics are legible.

@rcristea Ruben Cristea (rcristea) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🚀

@jreyes33 Jonathan Reyes (jreyes33) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Comment thread src/queries/useQuery.ts
Addresses PR review: after cancelling on reset, null both refs before
re-registering so a failed (or skipped) registration no longer exposes a
stale, already-cancelled observer/subscription on the return value. Covers the
subscription-rejected, observer-rejected, and localOnly-toggle cases. Adds a
test that resets into a rejected subscription query.
@aaronleopold Aaron Leopold (aaronleopold) merged commit 9342c68 into master Jul 6, 2026
1 check passed
@aaronleopold Aaron Leopold (aaronleopold) deleted the aaronleopold/forge-2185-react-ditto-follow-ups-from-the-v5-migration-review branch July 6, 2026 22:01
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