Skip to content
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

Forward the subscribeToMore option to enable usage from useQuery #35

Closed
wants to merge 6 commits into from
Closed

Conversation

dimitar-nikovski
Copy link
Contributor

No description provided.

@seeden
Copy link

seeden commented Dec 18, 2018

I did not see subscribeToMore in the observer. Did you test it ? Is it working ?

@dimitar-nikovski
Copy link
Contributor Author

dimitar-nikovski commented Dec 18, 2018

Hi @seeden , indeed it is part of the prototype of ObservableQuery from apollo-client/core link and is available in the returned observableQuery.

The relevant excerpt of calls is useQuery -> getCachedObservableQuery -> watchQuery -> instance of ObservableQuery -> public method subscribeToMore

It's not visible directly, because it's inside the __proto__ of the returned instance.

@seeden
Copy link

seeden commented Dec 18, 2018

Great sounds good. I can not wait :)

@trojanowski
Copy link
Owner

@dimitar-nikovski Thanks for your PR, but I'm not sure if it's a good idea to use subscribeToMore with hooks. You'll have to use it with the useEffect or useLayoutEffect hook and ensure that the array with correct variables is passed as the second argument of that hook and a proper unsubscribe function returned. Otherwise, you'll risk that the subscription will be subscribed/unsubscribed after each update (even if arguments won't be changed) or not resubscribed at all if they changed. In my opinion, a much better solution is the useSubscription hook which is WIP in #37. Are you aware of any use cases of subscribeToMore which wouldn't be possible to solve with useSubscription (or ApolloClient.subscribe which it will use under the hood)?

@seeden
Copy link

seeden commented Dec 18, 2018

I want to use it like this:

export default (query, options: Options = {}): Object => {
  const { onReady, ...rest } = options;
  const [subscribed, setSubscribed] = useState(false);
  const response = useQuery(query, {
    suspend: false,
    ...rest,
  });

  const { loading, error } = response;
  if (error) {
    throw error;
  }

  if (!loading && !subscribed && onReady) {
    setSubscribed(true);
    onReady(response);
  }

  return response;
};
useQuery(query, {
  onReady({ subscribeToMore } => {
      ...
  }),
})

I have tried it few weeks ago but there was no subscribeToMore function :) In this moment I am using <Subscribe> component from react-apollo

@trojanowski
Copy link
Owner

@seeden What do you think about that API:

const response = useQuery(query, {
  // query options
});

const subscriptionResult = useSubscription(subscription, {
  // subscription options
});

We could add a skip option for both useQuery and useSubscription to start them conditionally (#21).

@seeden
Copy link

seeden commented Dec 18, 2018

@trojanowski I am fine with useSubscription. I think that many users will not use subscriptionResult. They will use just updateQuery from subscription options

@seeden
Copy link

seeden commented Dec 18, 2018

Actually updateQuery is related to useQuery. I need to combine useQuery and useSubscription.

@seeden
Copy link

seeden commented Dec 25, 2018

@trojanowski I was thinking about it and I recommended to use both

  1. forward subscribeToMore for queries because it is related to current query (it is related to this pull request).
  2. add useSubscription as well. If user want to use just response from subscribe without updating query

If it is possible please merge this commit. It will be really helpful for me in this moment

SubscribeToMoreOptions<TData, TVariables, TSubscriptionData>,
'updateQuery'
> & {
updateQuery: UpdateQueryFn<TData, TVariables, TSubscriptionData>;
Copy link
Contributor Author

@dimitar-nikovski dimitar-nikovski Dec 25, 2018

Choose a reason for hiding this comment

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

updateQuery is optional in the apollo client typings, however, without it, subscribeToMore has no side effects, see code

document !== previousSubscription.current ||
isEqual(variables, previousSubscribtionOptions.current)
) {
unsubscribeToMore.current = observableQuery.current.subscribeToMore(
Copy link
Contributor Author

@dimitar-nikovski dimitar-nikovski Dec 25, 2018

Choose a reason for hiding this comment

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

subscribeToMore returns a function to unsubscribe from the subscription see here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

conditional mistake corrected in the commit below

@dimitar-nikovski
Copy link
Contributor Author

dimitar-nikovski commented Dec 26, 2018

Hi @seeden, now it should be much better, as @trojanowski pointed out, there are necessary conditions to set it up properly. I don't expect it to be merged, unless there is genuine desire from more people for it to be part of the project, for me, it mainly helps with the convenience of merging the subscription updates back into the store with updateQuery.

  • - unsubscribe from the subscription in useEffect if it has been set up
  • - only resubscribe if the variables or document (gql) have changed, also the recommended way, source

Conveniently, when merging the results in updateQuery, Apollo's ObservableQuery will schedule an update which will broadcast right into the subscription made in useQuery, therefore then setting a new result as if it was a change in query result, so the useQuery result would then become query + subscription merged.

@seeden
Copy link

seeden commented Dec 26, 2018

Hi @dimitar-nikovski thank you for your update 👍. Actually I am using subscribeToMore only with updateQuery. But it is related to useQuery because I am using old query result in the updateQuery (it is standard in the react-apollo and Query component I guess). I am merging edges most of the time.
I will be very happy to see this merged because I am refactoring my project to hooks and subscribe is not working in this moment in my project :) I do not want to use Subscribe component from react-apollo

@dimitar-nikovski
Copy link
Contributor Author

I ought to add some tests, just in case it will be going further.

@trojanowski
Copy link
Owner

What do you think about the API suggested in #37 (comment)? It would allow to update any previous query.

@trojanowski
Copy link
Owner

Do you think it would solve all your use cases for subscribeToMore or you still need it?

@dimitar-nikovski
Copy link
Contributor Author

@trojanowski it's cleaner indeed, we can use the subscription result from useSusbscription and the updateQuery prop form ObservableQuery returned by useQuery and achieve the exact same result. Feel free to close this down.

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