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

Support remote graphql schemas in plugins #4870

Merged
merged 6 commits into from Dec 18, 2018

Conversation

2 participants
@focusaurus
Copy link
Contributor

commented Dec 12, 2018

Resolves reaction-feature-requests/issues/53

Impact: minor
Type: feature

Issue

Allow a plugin to link a remote graphql API into the main reaction graphql API via remote schema delegation/stitching. This capability will support our long-term architecture vision of independent-but-integrated services.

Solution

The main implementation comes from graphql-tools which provides mergeSchemas() and other functions that support the base graphql schema delegation capability.

My solution here is to widen the feature set of our plugin options.graphQL.schemas. Currently, this option supports javascript strings which are GraphQL Schema Definition Language (SDL) format. This is widely used in the existing codebase. I've allowed this same plugin option to also support instances of graphql schema. This is implemented with 2 lines of duck typing. Strings are handled as before and become part of the local graphql schema. Non-strings are merged together via mergeSchemas(). The end result is our existing functionality still works but the new support for remote schema stitching is also available.

Breaking changes

I believe there are no breaking changes here. Quick local testing looks like the reaction meteor app still functions fine.

If an existing plugin in the ecosystem is passing something other than a javascript string to options.graphQL.schemas, then this implementation would most likely be a breaking change and we would need more thorough duck-typing to handle that correctly, or to introduce a new plugin option property dedicated to this like options.graphQL.remoteSchemas, for example.

Caveats

My test case for this is using a pre-generated schema for the remote service, and thus I can load the plugin synchronously into reaction. It is likely that enabling plugins to perform asynchronous initialization tasks would be convenient, but it's not strictly necessary for the work at hand, so I'm leaving it as a separate feature to implement, but it's worth thinking about a bit now. For example, a plugin might want to introspect the remote graphql service's schema over the network at runtime instead of generating an SDL file and bundling that with the plugin. At the moment, that configuration would not be supported.

Testing

  • Existing unit tests and lint pass
  • I have a sample plugin I'm using to build this out and the remote graphql delegation is working for that
  • I have manually smoke tested the reaction meteor UI is not obviously broken when a remote plugin is loaded.
  • I have manually smoke tested the starter kit storefront is not obviously broken when a remote plugin is loaded.
  • There is a unit test that stands up a mock remote/loopback graphql service with a single query, integrates it with TestApp, and sends a round trip graphql query.

Docs

Documentation pull request

feat: Support remote graphql schemas in plugins
Resolves reaction-feature-requests/issues/53
@aldeed
Copy link
Member

left a comment

It says that this is WIP, but I reviewed anyway. The approach looks good. Just a couple suggestions for performance.

Show resolved Hide resolved imports/node-app/core/createApolloServer.js Outdated
refactor(graphql): only merge schemas if non-empty
Move setup code outside of express middleware for efficiency

@aldeed aldeed changed the base branch from release-2.0.0-rc.8 to develop Dec 14, 2018

@focusaurus focusaurus force-pushed the feat-pete-53-remote-graphql-plugins branch Dec 17, 2018

@focusaurus focusaurus changed the title WIP: Support remote graphql schemas in plugins Support remote graphql schemas in plugins Dec 17, 2018

@focusaurus focusaurus force-pushed the feat-pete-53-remote-graphql-plugins branch to f3f71cd Dec 17, 2018

@focusaurus

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

OK I added unit tests, cleaned up WIP commits, made sure lint passed. I think this is ready for final code reviews.

@aldeed
Copy link
Member

left a comment

Nice to see the tests. I left a few comments about alternative ways.

Show resolved Hide resolved imports/node-app/devserver/schemas.js Outdated
Show resolved Hide resolved imports/node-app/core/createApolloServer.js Outdated
Show resolved Hide resolved imports/plugins/unit-test/remote-graphql/server/no-meteor/schemas.js Outdated
@focusaurus

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

OK @aldeed with those changes you suggested the code is much more isolated now. If it looks good now, merge away, or mention me if you find any other changes to request. Thanks!

@aldeed

aldeed approved these changes Dec 18, 2018

@aldeed aldeed merged commit 2506b23 into develop Dec 18, 2018

3 checks passed

License Compliance All checks passed.
Details
WIP ready for review
Details
security/snyk - package.json (Reaction Commerce) No new issues
Details

@aldeed aldeed deleted the feat-pete-53-remote-graphql-plugins branch Dec 18, 2018

@spencern spencern referenced this pull request Jan 18, 2019

Merged

Release 2.0.0 rc.9 #4922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.