Hide queryRef in a Symbol in useBackgroundQuery return value
#11010
Conversation
🦋 Changeset detectedLatest commit: d2c2379 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 |
size-limit report 📦
|
| @@ -617,7 +618,7 @@ describe('useBackgroundQuery', () => { | |||
|
|
|||
| const [queryRef] = result.current; | |||
|
|
|||
| const _result = await queryRef.promise; | |||
| const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise; | |||
There was a problem hiding this comment.
A note to anyone who'd argue these tests are testing implementation details: I'd agree! The vast majority are integration tests, though we also have a few unit tests that violate this rule in a way that feels like an acceptable tradeoff to keep some tests more compact :)
| interface QueryReferenceOptions { | ||
| export const QUERY_REFERENCE_SYMBOL: unique symbol = Symbol(); | ||
| /** | ||
| * A `QueryReference` is an opaque object returned by {@link useBackgroundQuery}. |
There was a problem hiding this comment.
Open to suggestions/wordsmithing here! I'm not wedded to this language, but wanted something short and that gets at the notion that QueryReference should only be passed into useReadQuery/its properties should not be accessed directly.
| @@ -197,9 +200,9 @@ export function useBackgroundQuery< | |||
| }, [queryRef, fetchMore, refetch]); | |||
| } | |||
|
|
|||
| export function useReadQuery<TData>(queryRef: QueryReference<TData>) { | |||
| export function useReadQuery<TData>(_queryRef: QueryReference<TData>) { | |||
There was a problem hiding this comment.
Since this argument is visible publicly, I'd prefer to leave this named queryRef and just call the variable inside this hook internalQueryRef (or whatever you see fit) to avoid confusion for anyone that looks at the arguments through their editor.
There was a problem hiding this comment.
Ah yep, good call! Resolved in 35b7667.
|
Bumped the size limit in d2c2379 but the |
Closes #10875.
Renames
QueryReferencetoInternalQueryReferenceso that theQueryReferencetype name can remain unchanged, which will be referenced in userland code for apps written in TS as is probably much less confusing than any alternative :)Also adds some basic type docs to
QueryReferenceto display in IDEs that support it:At first I thought maybe
SuspenseCacheshould return aQueryReferencewhen itsgetQueryRefmethod is called, but very quickly realized that any hook that returns aQueryReferenceshould be responsible for stashing it behind the Symbol instead of within Apollo's Suspense-y internals. I hesitated to renamequeryRefvariable names >internalQueryRefeverywhere for brevity, but if this is confusing I can make those changes too.Checklist: