fix(subscriptions): rebuild subscriber when getConfig() subscribe changes - #20
Closed
jimmy-phantom wants to merge 6 commits into
Closed
fix(subscriptions): rebuild subscriber when getConfig() subscribe changes#20jimmy-phantom wants to merge 6 commits into
jimmy-phantom wants to merge 6 commits into
Conversation
getConfig() re-evaluates on every fetch and produces a fresh subscribe value, but setupSubscription in QueryResult only consults config.subscribe at activation, params change, or when no subscriber exists. Once a subscriber is running, later subscribe values from getConfig() are ignored. Adds two failing tests: - A state-dependent interval (poll interval depending on this.response.ok) is captured at activation with response undefined, and never rebuilds once response.ok becomes true. - subscribe: undefined returned from getConfig() after a 404 does not stop the running subscriber. The existing "getConfig subscribe" tests pass because they return a constant poll() value, never exercising the cross-fetch change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
setupSubscription previously only consulted config.subscribe at
activation, on params change, or when no subscriber was running. Once a
subscriber was active, later subscribe values from getConfig() on each
refetch were ignored, so patterns like
subscribe: poll({ interval: this.response?.ok ? 100 : 5000 })
subscribe: this.response?.status === 404 ? undefined : poll({ ... })
had no effect after the first activation.
Changes:
- setupSubscription tracks the last-installed subscribe ref and rebuilds
on change. The post-fetch path always invokes it (the previous
unsubscribe === undefined guard skipped the case where the new value
is undefined and a running subscriber should be torn down).
- runQuery re-resolves options after the fetch resolves so getConfig()
observes the freshly assigned this.response before setupSubscription
runs.
- poll() is memoized by interval, so re-evaluating getConfig() with a
stable interval returns the same subscribe reference and incurs no
rebuild.
- Param-change rebuilds clear lastSubscribeFn before calling
setupSubscription, since the running subscriber captured the old
params.
Three lifecycle tests in query-stream.test.ts that used getConfig() to
scope an inline closure for test counters are converted to static
config = { subscribe(...) {...} }. Their config values never depended
on per-fetch state; the getConfig() form was incidental and would now
cause per-fetch rebuilds (the documented trade-off of choosing the
dynamic form).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Caching `poll()` output by interval was an asymmetric optimization: re-evaluating `poll()` inside `getConfig()` on every fetch returned a stable ref so steady-state polling didn't churn, but inline subscribe closures inside `getConfig()` always returned fresh refs and rebuilt on every fetch. The cache made poll-using queries cheaper than arbitrary user closures for the same shape of code, which is a distinction users would have to learn. Removed. Now both produce a fresh ref each `getConfig()` call and both incur the same cheap per-fetch rebuild (clearTimeout + setTimeout + one closure alloc), with no timing impact since the next tick is scheduled `interval` ms after fetch resolution either way. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test still uses a 404 to trigger the transition, but the behavior under test is `subscribe: undefined` after any error response, not specifically 404. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spell out the chain — adapter populates this.response, getConfig() may branch on it, the pre-fetch resolve saw it as undefined — so a reader following runQuery doesn't have to reverse-engineer why the resolve is called twice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test existed to verify that two queries sharing a memoized poll() reference each got independent per-invocation state. With the cache removed, two queries calling poll(100) get distinct refs, and the existing different-intervals test already covers per-query independence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jimmy-phantom
marked this pull request as draft
May 13, 2026 21:45
2 tasks
Collaborator
Author
|
Closing in favor of #25 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a bug where
getConfig()returning a differentsubscribevalue mid-session had no effect:setupSubscriptiononly consultedconfig.subscribeat activation, on params change, or when no subscriber was running, so once a subscriber was active any later value produced bygetConfig()was ignored.The two patterns that motivated the fix:
These are the canonical legitimate
getConfig()use cases: config field values that genuinely depend on per-fetch runtime state.Approach
One rule: subscribe rebuilds when its reference changes from what's currently installed. No marker, no opt-in mechanism, no asymmetry between user code and built-in factories.
setupSubscriptiontrackslastSubscribeFnand short-circuits on ref equality. The post-fetch path always invokes it. The previousunsubscribe === undefinedguard skipped the case where the new value isundefinedand a running subscriber should be torn down.runQueryre-resolves options after the fetch resolves sogetConfig()observes the freshly assignedthis.responsebeforesetupSubscriptionruns.lastSubscribeFnbefore callingsetupSubscription, since the running subscriber captured the old params.Trade-off worth flagging
Any
subscribevalue placed insidegetConfig()produces a fresh function reference each call (whether it'spoll(...)or an inline closure). The framework treats this honestly: re-evaluatinggetConfig()on each fetch produces a new ref, so the running subscriber is torn down and rebuilt. Per-fetch cost is one closure allocation, one cleanup call, and oneclearTimeout/setTimeoutswap for poll-like subscribers. Timing is unaffected because the next tick is scheduledintervalms after fetch resolution either way.The escape hatch for stable subscribers is static
config = { subscribe(...) { ... } }, which evaluates once at class-field-init time and gives a stable ref. The two forms have clear, distinct, predictable semantics:config = ...for stable subscribers (the common case).getConfig()when the value of a config field genuinely depends on runtime state.Earlier iterations of this PR experimented with a marker-based opt-in mechanism (
SUBSCRIBE_KEY) and apollCacheto give built-in factories stable identity. Both were removed in favor of the simpler rule above. The marker created a hidden distinction between built-in and user code that users would have to learn; the cache was an asymmetric optimization that only benefitedpoll()while leaving inline closures churning.Test plan
honors a state-dependent interval after first fetch resolves,stops polling when getConfig() switches subscribe to undefined after an error) now passquery-stream.test.tsconverted to staticconfig = ...(their config values never depended on runtime state)tsc --noEmitclean🤖 Generated with Claude Code