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

Discussion: subscriptions API design improvements #2892

Closed
jedwards1211 opened this issue Jan 19, 2018 · 10 comments
Closed

Discussion: subscriptions API design improvements #2892

jedwards1211 opened this issue Jan 19, 2018 · 10 comments

Comments

@jedwards1211
Copy link
Contributor

jedwards1211 commented Jan 19, 2018

I've been really happy with Apollo so far, and I'm delighted that it supports subscriptions, but the way subscriptions work in Apollo still seems half-baked to me, especially compared to what I was used to in Meteor. Here are some random thoughts that have been brewing in my mind. Everyone feel free to comment or suggest solutions!

Subscription updates can be missed while you're waiting for query results

If your subscription variables depend on query results, you can't initiate the subscription until the initial query is complete. This poses a problem: if any updates were published between the time the initial query was executed on the server and the client gets the result and initiates the subscription, the client will never get those updates.

This is why I think it would be better to use Meteor-style subscriptions that fetch and send initial values after the server starts listening to PubSub. This is definitely already possible, but it's not recommended in the Apollo docs, and the danger of missed updates is not mentioned anywhere in the docs AFAIK. It would also take manual effort to send the initial update when the subscription starts; something systematic is desirable if possible.

For example, at the very least, PubSub could hang onto the last value published on each topic and (optionally) republish the last value when pubsub.asyncIterator is called.

Or, pubsub.asyncIterator could accept an initialValue option, so that in our subscription resolvers, we could fetch the initial value and then pass it to pubsub.asyncIterator. This would be less involved than setting up our own async iterator to yield the initial value and then yield the
rest from pubsub.asyncIterator. (I assume at least that any time we create an async iterator of our own, we should take care to implement the return and throws methods to ensure that it gets shut down properly?)

What about standalone subscriptions?

The current design seems to assume we'll always use a subscription with a corresponding query. But coming from Meteor, where you use subscribe by itself, and the subscription sends the initial data on startup, using a query and subscription together seems a little overcomplicated for some use cases.

If subscription updates contain objects with __typename and id, couldn't Apollo update the cache automatically?

Right now developers have to write their own updateQuery logic for each subscription, which is a bit cantankerous. But if the objects in a subscription update contain __typename and id, I'd think Apollo could apply the same normalization and cache update logic as it does for queries automatically, and not require developers to write any update logic of their own. Is there something I'm not thinking about that makes this difficult?

Updating deeply nested objects is awkward

This kind of goes along with the above. Let's say my query is

query {
  User {
    id
    name
    Posts {
      id
      text
    }
  }
}

And let's say I subscribe to updates on posts for the given post ids in the query result.
When I get a post update, I have to merge it into the query shape, which is fairly tedious, something like the following code. It would be a lot simpler if I could just tell the Apollo cache to
replace its cached post with the given id with the updated version. (Which maybe is possible, but
I don't know if it can really be done via subscribeToMore.)

updateQuery(prev, update) {
  const {User: {Posts}} = prev
  const {subscriptionData: {data: {Post}}} = update
  const postIndex = newPosts.findIndex(p => p.id === Post.id)
  if (postIndex < 0) return prev
  return {
    ...prev,
    User: {
      ...prev.User,
      Posts: [
        ...Posts.slice(0, postIndex),
        Post,
        ...Posts.slice(postIndex + 1),
      ],
    }
  }
}

Unsubscribing and resubscribing when variables could be automated

Currently all the work of unsubscribing and resubscribing if variables change is pushed onto the developer. It wouldn't be too hard to make my own component to do this automatically in a similar way to how Apollo query HOCs automatically refetch when variables change. But it definitely seems to me like something that should live in Apollo itself.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jan 30, 2018

client.resetStore() doesn't automatically retry subscriptions

For example, if subscriptions failed because the user was unauthorized. I can use resetStore() to refetch queries after logging the user in, but subscriptions are stuck in a broken state.

I was wrong about this, I had a typo in one of my subscription names. As long as the view wrapped with graphql calls subscribeToMore whenever the data it gets from the initial query changes, it will successfully resubscribe.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jul 26, 2018

According to graphql/graphql-js#1433 (comment), Relay performs some automatic cache updates from subscription data:

Not sure about Apollo but in Relay caching is based on object ID so it should be updated automatically without requiring matching path in a query.

I haven't seen any evidence that Apollo is similarly performing automatic updates from subscription data using getDataIdFromObject, but wouldn't that be a useful feature?

@helfer would you mind commenting on this?

It also seems like even with mutations, Apollo doesn't automatically update from deeply nested objects in the result, only the top-level mutation fields. Someone correct me if I'm wrong about this. Seems to me Apollo ought to automatically update the cache with all objects at any level in query or subscription data.

@TheMickeyMike
Copy link

Hi, I'm not sure if i correctly understood your case, but i will try give you example config with automated cache updates when subscription received.

GraphQL Server schemas

const types = qpl`

type Post {
    id: Int!
    text: String!
  }

 type User {
    id: Int!
    name: String!
    posts: [Post]!
  }
`;

const query = gql`
   type Query {
    user(userId: Int!): User!
  }
`;


const postSubscription = gql`
  type Subscription {
    newPostSubscription: Post!
  }
`;

GraphQL Client

Setup ApolloClient with this cache

// Set up Cache
const cache = new InMemoryCache({
  dataIdFromObject: object => {
    switch (object.__typename) {
      case 'User': return `User:${object.id}`;
      case 'Post': return `Post:${object.id}`;

 // This is only nesesery when your server subscription returns custom type with same value in `id` as
// cached Post. Every field returned from MyCu... will add/update Post fields, because you map it to
// this cached object.
      case 'MyCustomTypeReturnedFromSubscription': return `Post:${object.id}`;


      default: return defaultDataIdFromObject(object); // fall back to default handling
    }
  }
});
const USER_QUERY = gql`
  query UserQuery($userId: Int!) {
    user (userId: $) {
      id
      name
      posts {
        id
        text
      }
    }
  }
`;

const NEW_POSTS_SUBSCRIPTION = gql`
  subscription onNewPost {
    newPostSubscription {
      id
      text
    }
  }
`;

<Fragment>
    <Banner headerText="Blog"/>
    <Query query={USER_QUERY}>
      {({ loading, error, data, subscribeToMore }) => {
        if (loading) return <p>Loading...</p>;
        if (error) return <p>Error :(</p>;
        return <PostsList
          posts={data.user.posts}
          subscribeToNewPost={() =>
            subscribeToMore({
              document: NEW_POSTS_SUBSCRIPTION,
            })
          }
        />;
      }}
    </Query>
  </Fragment>


class PostList extends React.Component {

  componentDidMount() {
    this.props.subscribeToNewPost();
  }
  
}

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Aug 7, 2018

Hmmm, it won't automatically add the new posts to user.posts though, will it? I think the new post will just wind up under an unreferenced key in the cache. GraphQL has no way of knowing whether a new post belongs in the user's list of posts or not.

Though if subscription updates for an existing post that's already referenced came in, I guess this would work?

@TheMickeyMike
Copy link

TheMickeyMike commented Aug 7, 2018

@jedwards1211 Your subscription can notify on:

  • Add new Post
  • Update Post text

Or just updating existing user post?

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Aug 7, 2018

Yes but AFAIK, even if the subscription notifies on add new post, Apollo can't automatically determine that it should add that post to user.posts. You have to provide your own custom update logic.

Just imagine if there are multiple different lists of posts in the query:

  query UserQuery($userId: Int!) {
    trendingPosts {
      id
      text
    }
    user (userId: $userId) {
      id
      name
      ownPosts {
        id
        text
      }
      likedPosts {
        id
        text
      }
    }
  }

When a new post comes in, which of these three lists should Apollo add it to? It can't make this decision automatically.

@TheMickeyMike
Copy link

In case of new user post I have no clue how this could be done automatically, but updating post text can be automated. In my Project i have Post object with many Tag objects

type Tag {
    id: Int!
    name: String!
    color: String!
  }

type Post {
    id: Int!
    title: String!
    content: String!
    tags: [Tag]!
  }

My subscription notifies clients when tag color is updated

type TagAction {
    name: String!
    color: String!
  }

  type Subscription {
    tagsActions: TagAction!
  }

In my client site i have custom cache keys mapping

// Set up Cache
const cache = new InMemoryCache({
  dataIdFromObject: object => {
    switch (object.__typename) {
      case 'Tag': return `Tag:${object.name}`;
      case 'TagAction': return `Tag:${object.name}`;
      default: return defaultDataIdFromObject(object); // fall back to default handling
    }
  }
});

Blog.js

const POSTS_QUERY = gql`
  query PostsQuery {
    posts {
      id
      title
      content
      tags {
        name
        color
      }
    }
  }
`;

const TAGS_SUBSCRIPTION = gql`
  subscription onNewTagName {
    tagsActions {
      name
      color
    }
  }
`;

const Blog = () => (
  <Fragment>
    <Banner headerText="Blog" subHeaderText="all_posts" />
    <Query query={POSTS_QUERY}>
      {({ loading, error, data, subscribeToMore }) => {
        if (loading) return <p>Loading...</p>;
        if (error) return <p>Error :(</p>;
        return <PostsList
          posts={data.posts}
          subscribeToNewTagColor={() =>
            subscribeToMore({
              document: TAGS_SUBSCRIPTION,
            })
          }
        />;
      }}
    </Query>
  </Fragment>
);

export default Blog;

PostList.js

class PostList extends React.Component {

  componentDidMount() {
    this.props.subscribeToNewTagColor();
  }
}

Before:
before

After subscription notifies:
after

As you can see when my ROOT_QUERY is done Post in cache are mapped as Post:ID and tags Tag:NAME. Apollo somehow (I'm not expert) links this objects together soPost object have something like foreign keys to tags. When subscription arrive, Tag color object is updated, React rerender all components with Post object where Post had FK to this updated Tag.

So you should map your Post before write it into cachecase 'Post': return Post:${object.id}; and your subscription should return object like this

{
"__type": "Post",
"id": "<POST_ID_YOU_WANT_TO_UPDATE>",
"text": "THIS IS NEW CONTENT OF POST"
}

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Aug 7, 2018

Yes, if you look at the structure of the raw cache data in the debugger (instead of the Apollo dev tools), you'll see that Post:1.tags has keys to the tags, rather than the tag data. This means the existing tags in the post will receive any changes written to the tags independently. To handle tags being added or removed from a post though, you would have to write a custom updater.
Out of curiosity, why are you using name instead of id to key the tags? Why not just have your subscription return type Tag with the correct id?

@TheMickeyMike
Copy link

Well for now I'm using name for better understanding how Apollo cache works. Moreover, I need to check case when my client subscribe many subscriptions (ex. likeSub, dislikeSub, newPostSub...) and
if there will be one open WebSocket pipe to my Apollo server, or each subscription opens new one, if so i will stick with one connection and probably doing some magic on server side with dynamic __typename or add enum field to recognize action type and parse subscription data based on this actionType field, idk for know, just overthinking some basic stuff 🤔
Btw. this Blog is still in development, because I started my React/Node.js/Typescript journey couple days ago, even JS is new for me 😄 Now I know, that learning GraphQL in totally new technologies is really challenging 🤦‍♂️

@hwillson
Copy link
Member

Thanks for the great discussion @jedwards1211 @TheMickeyMike. A new design for the subscription API is being worked on, as part of our Apollo Client 3.0 work. I'll close this for now (since we're trying to make sure issues here are for bugs only), but your input is greatly appreciated. Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants