-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove setOptions
and tweak reobserve
#12463
Conversation
🦋 Changeset detectedLatest commit: f15b031 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: |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size-limit report 📦
|
src/core/ObservableQuery.ts
Outdated
/** | ||
* Reevaulate the query against the current set of options. | ||
*/ | ||
public rerun() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I called this rerun
, but am open to other names. Other names I considered:
reevaluate
/evaluate
reexecute
/execute
We use reobserve
without options in a decent amount of tests and call this in reobserveCacheFirst
so I figured there might be a use case for just trying to run the query again against the current set of options. I'm open to hearing otherwise and updating existing tests to observable['reobserve']
if need be, but I'm trying to avoid that. Calling setOptions({})
or refetch
doesn't have the same intent as this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the same time, introducing a public api that does kinda the same as before feels a bit moot.
Do we think this will be used in userland?
Maybe at that point, it might be better to rename reobserve
to rerun
and remove setOptions
?
src/core/ObservableQuery.ts
Outdated
@@ -669,12 +669,22 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`, | |||
}; | |||
} | |||
|
|||
/** | |||
* Reevaluate against a new set of options which might cause a fetch. | |||
*/ | |||
public setOptions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider if setOptions
works the way we'd like it to. The way its implemented is a big reason why I don't like the way setVariables
works where it just resolves with the current value if there are no observers.
setVariables({ id: 2 })
with no observers will not call reobserve
, but setOptions({ variables: { id: 2 } })
will which could result in a fetch.
I'd prefer some symmetry here if possible. In my mind, either we have all reobserve
-based methods execute against reobserve
, or we should require an active subscription to run all of them. Otherwise its difficult to know which ones might or might not fetch.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this feels like it should behave more like setVariables
.
Discussed in person. We are going to do a few changes:
|
30cb465
to
833f899
Compare
@phryneas this should be good to look at again as I've made the changes we discussed in our call. |
reobserve
privatesetOptions
and tweak reobserve
b2d8c43
to
aa91ec6
Compare
…bserve. Make it public
aa91ec6
to
f15b031
Compare
Closes #12447
Make
reobserve
a public, documented API inObservableQuery
. RemovessetOptions
as this was pretty much an alias forreobserve
without thenetworkStatus
option.This PR also makes setting the
networkStatus
in a private way. We didn't like thatreobserve
let you set whatever you wanted if this was made public, so that argument has been removed. There is now a private way to set this value insideObservableQuery
for the internal functions.