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

Basic subscriptions support added to mergeSchemas and TypeRegistry. #463

Merged
merged 11 commits into from
Nov 22, 2017

Conversation

shipiak
Copy link
Contributor

@shipiak shipiak commented Nov 2, 2017

#420 - FourTwenty :)
Added support for subscription type (subscriptions will work with local schema only).

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

@apollo-cla
Copy link

@shipiak: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@freiksenet
Copy link
Contributor

Looks good to me! Could you add the changelog entry, then I can merge.

@freiksenet
Copy link
Contributor

I'm also not 100% sure about the test at this moment, it would be nice if you could add test that simulates actual subscription event and checks that both unmerged and merged schemas work.

@shipiak
Copy link
Contributor Author

shipiak commented Nov 2, 2017

Yeah, you are right I was bit too quick with PR :)

Subscription resolvers are not working correctly on merged schema and cannot figure why.
delegatingResolver func is never called on subscription.

Still studying how delegating of resolvers work, but maybe it could be connected to subscription resolver having different structure?

It is not a function as in query or mutation but object with method subscribe:

Subscription: {
        commentAdded: {
          subscribe: () => pubsub.asyncIterator('commentAdded')
        }
    },

Would be happy for any pointers :)
Many thanks

@freiksenet
Copy link
Contributor

Try adding delegating resolver to "subscribe" http://dev.apollodata.com/tools/graphql-subscriptions/subscriptions-to-schema.html

fullResolvers.Subscription = {};
}
Object.keys(subscriptionType.getFields()).forEach(name => {
fullResolvers.Subscription[name] = createDelegatingResolver(
Copy link
Contributor

Choose a reason for hiding this comment

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

so here it would be

 fullResolvers.Subscription[name] = { subscribe: createDelegatingResolver(

@freiksenet
Copy link
Contributor

I've added the comment where you should modify it. It might just work. :)

@shipiak
Copy link
Contributor Author

shipiak commented Nov 2, 2017

Hi @freiksenet ,

Subscriptions in local schemas are now working correctly, however I would like to ask you to review change in defaultMergedResolver.ts. I had to add custom condition for mapping of subscription payload from asyncIterator. Result is in parent.data[responseKey] not parent[responseKey]

Many thanks.

@shipiak
Copy link
Contributor Author

shipiak commented Nov 18, 2017

Hi guys,
@freiksenet @stubailo any feedback for this PR?
Could it be merged?

Many thanks!

@freiksenet
Copy link
Contributor

Hi!

Thanks! I was on holiday for two weeks and then had flu, so I didn't have chance to review it. I'll do it today.

@freiksenet freiksenet merged commit ebf0352 into ardatan:master Nov 22, 2017
@freiksenet
Copy link
Contributor

This looks awesome, thank you @shipiak.

@wawhal
Copy link

wawhal commented Jul 24, 2018

Any documentation related to this PR?

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.

4 participants