fix: various lifecycle issues#77
Conversation
🦋 Changeset detectedLatest commit: 152e41d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 91.40% 91.66% +0.26%
==========================================
Files 20 20
Lines 733 732 -1
Branches 140 140
==========================================
+ Hits 670 671 +1
+ Misses 55 53 -2
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where queries that were not fetched were still being cached. It updates the QueryCacheEntry type to exclude null and moves the cache insertion logic inside the shouldFetch conditional block. The reviewer suggests making the resource property optional in QueryCacheEntry to support retaining queries even when they are not fetched (e.g., for store-only policies).
| export type QueryCacheEntry = { | ||
| resource: Resource<unknown>; | ||
| retain: (environment: IEnvironment) => Disposable; | ||
| } | null; | ||
| }; |
There was a problem hiding this comment.
To support retaining queries even when they are not fetched (e.g., for store-only or store-or-network policies when data is already available), we should make the resource property optional in QueryCacheEntry. This allows us to cache and retain the query without needing to create a dummy resource.
| export type QueryCacheEntry = { | |
| resource: Resource<unknown>; | |
| retain: (environment: IEnvironment) => Disposable; | |
| } | null; | |
| }; | |
| export type QueryCacheEntry = { | |
| resource?: Resource<unknown>; | |
| retain: (environment: IEnvironment) => Disposable; | |
| }; |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
solid-relay | 152e41d | Commit Preview URL Branch Preview URL |
May 28 2026, 05:20 AM |
1 participant
Closes #74
Closes #75
Action Items From #74:
Items
createResource.storageis used as a place to wire subscriptions. The documented purpose ofstorageis signal substitution, not subscription wiring. — not an issue, but an intentionally hacky workaroundcreateFragmentfinallycleanup is gated onisServer, so the client error path leaksobserveFragmentsubscriptions. — 152e41dcreateLazyLoadQuery.storagelaunches avoid (async () => …)loop with no stale-check; the loop keeps writing toreplaySubjectafter the owner is gone. — not an issue since the purpose of that callback is to stream all the data fetched from the server into the client Relay environment, which should happen even when the owner is gonequeryCacheinnerMaphas no eviction policy, andentry === nullentries are stored permanently. — c3026c5runWithOwneris not used anywhere in the codebase, even at the call sites that subscribe outside the synchronous owner phase. The canonical Solid pattern (used bycreateResourceitself for refetch) is missing. — unowned code paths don't seem to create anything that should be ownedsetSubscription(...)overwrites the previousSubscriptionsignal without unsubscribing it first; the previous subscription becomes unreachable but alive. — 152e41dcreatePaginationFragmentcan have concurrentcreateFetchTrackerinstances during rapidloadNextcalls before the previous one is disposed. — separate issue (needs reproduction first)