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

Improved GraphQL client #672

Merged
merged 3 commits into from
Jan 24, 2020
Merged

Improved GraphQL client #672

merged 3 commits into from
Jan 24, 2020

Conversation

swalkinshaw
Copy link
Contributor

@swalkinshaw swalkinshaw commented Jan 20, 2020

This is a complete rewrite of the GraphQL client functionality.

The previous implementation suffered from a few problems making it virtually unusable as detailed in #511. Here's a summary:

  1. you couldn't specify a local schema file
  2. due to the above, the client made a dynamic introspection request on initialization which was very slow
  3. unbounded memory growth due to building new clients at runtime

This rewrite was focused on solving those problems first and foremost but we also had a few other goals:

  • support API versioning
  • provide better defaults and an improved developer experience
  • ensure it's hard to do the wrong thing

The new GraphQL client only supports loading local schema files to ensure no introspection requests are made during app runtime. The goal is that clients are fully initialized at application boot time (and if you're using Rails this is handled automatically).

Workflow:

  1. Set ShopifyAPI::GraphQL.schema_location to a directory path (or use the default in Rails of db/shopify_graphql_schemas).
  2. Save a JSON version of Shopify's Admin schema locally (or use the shopify_api:graphql:dump Rake task) to the schema_location and name it after the API version: 2020-01.json.
  3. Access the client at ShopifyAPI::GraphQL.client
  4. Execute queries via client.query

Rails integration

All the new features are designed to work independently of Rails but there are a few additional integration points:

  • schema_location is set to be Rails specific within db/
  • the Rake task is loaded automatically with the environment task prerequisite
  • clients are initialized during Rails initialization

Migration

If anyone is using the current GraphQL client it's very easy to migrate to this new one.

Previously a client was initialized with ShopifyAPI::GraphQL.new:

client = ShopifyAPI::GraphQL.new

SHOP_NAME_QUERY = client.parse <<-'GRAPHQL'
  {
    shop {
      name
    }
  }
GRAPHQL

result = client.query(SHOP_NAME_QUERY)
result.data.shop.name

Now you use ShopifyAPI::GraphQL.client which has already been initialized:

SHOP_NAME_QUERY = ShopifyAPI::GraphQL.client.parse <<-'GRAPHQL'
  {
    shop {
      name
    }
  }
GRAPHQL

result = ShopifyAPI::GraphQL.client.query(SHOP_NAME_QUERY)
result.data.shop.name

Todo

  • Update README

@swalkinshaw swalkinshaw requested a review from a team as a code owner January 20, 2020 22:09
@swalkinshaw swalkinshaw requested a review from a team January 20, 2020 22:10
Copy link

@RobertWSaunders RobertWSaunders left a comment

Choose a reason for hiding this comment

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

This is awesome, I think you can update the GraphQL docs to use your ShopifyAPI::GraphQL.client instead of ShopifyAPI::GraphQL.new. Just saw your PR description TODO. 👏

lib/shopify_api/graphql.rb Show resolved Hide resolved
Copy link
Contributor

@tanema tanema left a comment

Choose a reason for hiding this comment

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

Looks good to me, I would prefer to have the migration guide and docs on setting up the schema in before we release the version with this functionality in it.

lib/shopify_api/graphql/task.rake Show resolved Hide resolved
@swalkinshaw
Copy link
Contributor Author

Updated with:

  • README changes
  • Full usage output for the rake task
  • Documentation guide in docs/graphql.md

👀 appreciated

docs/graphql.md Outdated Show resolved Hide resolved
@swalkinshaw swalkinshaw force-pushed the graphql-client-rewrite branch from 7bc2509 to ab80075 Compare January 21, 2020 22:50
@swalkinshaw swalkinshaw force-pushed the graphql-client-rewrite branch from ab80075 to d6295ff Compare January 22, 2020 20:27
@swalkinshaw
Copy link
Contributor Author

@alanhill helped 🎩 this and got it all working upgrading a real app 🎉

With his help we found some issues with the rake task so they've all been fixed now.

Copy link
Contributor

@chrisbutcher chrisbutcher left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

This is a complete rewrite of the GraphQL client functionality.

The previous implementation suffered from a few problems making it
virtually unusable as detailed in
#511. Here's a summary:

1. you couldn't specify a local schema file
2. due to the above, the client made a dynamic introspection request on
initialization which was very slow
3. unbounded memory growth due to building new clients at runtime

This rewrite was focused on solving those problems first and foremost
but we also had a few other goals:

* support API versioning
* provide better defaults and an improved developer experience
* ensure it's impossible to do the wrong thing

The new GraphQL client *only* supports loading local schema files to
ensure no introspection requests are made during app runtime. The goal
is that clients are fully initialized at application boot time (and if
you're using Rails this is handled automatically).

Workflow:
1. Set `ShopifyAPI::GraphQL.schema_location` to a directory path (or
use the default in Rails of `db/shopify_graphql_schemas`).
2. Save a JSON version of Shopify's Admin schema locally (or use the
`shopify_api:graphql:dump` Rake task) to the `schema_location` and name
it after the API version: `2020-01.json`.
3. Access the client at `ShopifyAPI::GraphQL.client`
4. Execute queries via `client.query`
* Fixes Rails `environment` task prereq
* Does a preflight request to ensure authentication works
* Improved output
@swalkinshaw swalkinshaw force-pushed the graphql-client-rewrite branch from b41d875 to de79d8c Compare January 24, 2020 15:27
@swalkinshaw swalkinshaw merged commit b0b4806 into master Jan 24, 2020
@swalkinshaw swalkinshaw deleted the graphql-client-rewrite branch January 24, 2020 17:05
@linzhao125 linzhao125 temporarily deployed to rubygems January 27, 2020 20:49 Inactive
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.

7 participants