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

Add support for GraphQL subscriptions #52

Closed
grxy opened this issue Dec 12, 2017 · 15 comments
Closed

Add support for GraphQL subscriptions #52

grxy opened this issue Dec 12, 2017 · 15 comments

Comments

@grxy
Copy link

grxy commented Dec 12, 2017

My team would love to use the GrAMPS architecture, but we're blocked by not having subscription support. It would be great if we could have a basic subscription implementation so data sources can add their own subscriptions.

@jlengstorf
Copy link
Member

@grxy This is on our roadmap, but not highly prioritized — however, if you wanted to add subscriptions to https://github.com/gramps-graphql/gramps (which is on its way to 1.0 right now), PRs are extremely welcome.

If your goal is to just add the basics (e.g. you can define a subscription mySub to the schema), it would be a relatively small PR.

@grxy
Copy link
Author

grxy commented Dec 12, 2017

@jlengstorf Is the https://github.com/gramps-graphql/gramps repo going to replace this one in the long term? Or is it mainly a separation of functionality so that things can be more maintainable?

@jlengstorf
Copy link
Member

That package will ultimately power this one. The Express part will be a really thin wrapper around the other module, so it'll probably make the most sense to fully replace it unless you REALLY need the middleware pattern.

@grxy
Copy link
Author

grxy commented Dec 15, 2017

@jlengstorf For now, we've decided to do all of our queries and mutations in GrAMPS while defining our subscriptions separately and calling mergeSchemas later in the process. We definitely want to use the new pattern in the long term, but in the short term, it would be nice to have subscription support added in this middleware package. That being said, would you be open to reviewing a PR for this package that provides basic support for subscriptions as a short-term solution and then porting that functionality to the new gramps package as it evolves towards 1.x?

@jlengstorf
Copy link
Member

@grxy For sure! I think we're almost ready with the 1.0 version of GrAMPS (see this comment for details).

One note: there's a breaking change from 0.x to 1.x in the shape of data sources. Depending on how many data sources you have right now, it may be worth making the refactor sooner rather than later to limit the amount of headache you're up against later.

If the 1.0 pattern works for you, I'd really appreciate PRs against @gramps/gramps instead. Data source changes notwithstanding, the GrAMPS core should be a drop-in replacement.

If that doesn't work for you, I'm happy to accept PRs against this repo in the short term.

Thanks!

@grxy
Copy link
Author

grxy commented Dec 15, 2017

@jlengstorf Awesome! I will take a look at 1.x first to see if that works for our use case.

@grxy
Copy link
Author

grxy commented Dec 15, 2017

@jlengstorf I don't see mutations added by default in the new package. Are you not planning to support them out of the box or is that functionality just pending?

@jlengstorf
Copy link
Member

@grxy Anything that's parseable by Apollo should be supported in the new package. Since we're using mergeSchemas instead of string concatenation, we don't need to declare anything up front. If you want to give this a shot and write a subscription definition in a data source, I'd love to hear if this works as expected. 😄

@grxy
Copy link
Author

grxy commented Dec 18, 2017

@jlengstorf I will do that today and let you know.

@grxy
Copy link
Author

grxy commented Dec 18, 2017

@jlengstorf Our data sources have custom scalar types such that I can't build one of our subscriptions without it. I've created an issue in the other project to address the problem: gramps-graphql/gramps#41

@jlengstorf
Copy link
Member

@grxy It looks like @ecwyne is operating at Usain Bolt-like speeds today. 😄 Let me know if you're still having trouble after that fix went in.

@grxy
Copy link
Author

grxy commented Dec 18, 2017

@jlengstorf Will check once that fix is published.

@grxy
Copy link
Author

grxy commented Dec 19, 2017

@jlengstorf So I was successfully able to get subscriptions to work within a Gramps data source. I was, however only able to set it up such that the resolvers live in the stitching config, not in the main resolvers (mapResolvers was throwing an error). Shall I create an issue in the new repo to address the problem I encountered?

@jlengstorf
Copy link
Member

@grxy Please do! I'll close this issue, since it sounds like subscriptions are working (ish). 😄

@grxy
Copy link
Author

grxy commented Dec 21, 2017

@jlengstorf Finally got around to opening an issue: gramps-graphql/gramps#58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants