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

Update with-filter.ts #103

Merged
merged 9 commits into from
Sep 15, 2017
Merged

Update with-filter.ts #103

merged 9 commits into from
Sep 15, 2017

Conversation

srtucker22
Copy link
Contributor

I think this is might be what I’m looking for.

Let’s say my GQL sub is something like:

type Subscription {
    # Subscription fires on every message added
    # for any of the groups with one of these groupIds
    messageAdded(groupIds: [Int]): Message
  }

Right now, my withFilter resolver might look like this:

  messageAdded: {
      subscribe: withFilter(
        () => pubsub.asyncIterator(‘messageAdded’),
        (payload, args, ctx) => {
          return myFilter(payload, args, ctx);
        },
      ),
  }

That’s pretty cool, but this means that every subscriber to messageAdded will get a message and then filter for whether to publish to client based on the myFilter function. Not very scalable :/

The most obvious way to fix this to me is to publish more distinct topics:

      subscribe: withFilter(
        (payload, args, ctx) => {
          const topics = getTopicsFromArgs(args);
          return pubsub.asyncIterator(topics);
        },
        (payload, args, ctx) => {
          return myFilter(payload, args, ctx);
        },
      ),

Now pubsub.publish will publish something like messageAdded.1 for a message added to Group 1, and only users subscribed to messageAdded.1 will receive the message and run it through myFilter. But currently asyncIteratorFn doesn't have access to the args, context, etc to make distinct topic(s) like messageAdded.1

So while this fix is a small bit of code, I think it could make withFilter a much more scalable helper function :)

@srtucker22
Copy link
Contributor Author

doesn't seem like this code is affecting the build failing :/

@NeoPhi
Copy link
Contributor

NeoPhi commented Sep 12, 2017

Can you rebase this PR against the most recent master to see if it resolves the issue.

@srtucker22
Copy link
Contributor Author

We cool now?

@NeoPhi NeoPhi merged commit 2e7f872 into apollographql:master Sep 15, 2017
@srtucker22 srtucker22 deleted the patch-1 branch September 15, 2017 15:20
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.

2 participants