allows users to provide a hook when graphql protected customer data e…

[infiton]

Gadget has just completed migrating all of our Shopify syncing engine to using the GraphQL api exclusively. One pain point in doing so was dealing with models that have protected customer data fields. Because there is no way to know if an app has filled out the form to request specific customer data fields the only way to know if Gadget can query these fields on apps behalf is to try.

Unfortunately with the way that shopify-api-node is currently set up, if an app does not have access the query will throw since shopify populates the errors field. However they also will return the data for the query, but with null in the place of the protected fields.

I think the default behaviour of throwing if the errors field is populated makes sense, however in this case we would like the option to not throw.

To resolve this I have added a configuration to the client onProtectedCustomerDataError that allows for custom handling of this situation.

Let me know what you think

cc @lpinca @airhorns @jcao49

[coveralls]

coverage: 100.0%. remained the same
when pulling a2179ac on infiton:handle-protected-customer-data
into 857bfe1 on MONEI:master.

[lpinca]

I'm not a fan of adding a new option for this edge case. What do you think about something like this

diff --git a/index.js b/index.js
index 14616cf..00bd31f 100644
--- a/index.js
+++ b/index.js
@@ -286,16 +286,18 @@ Shopify.prototype.graphql = function graphql(data, variables) {
     timeout: this.options.timeout
   };
 
-  const afterResponse = (res) => {
-    if (res.body) {
-      if (res.body.extensions && res.body.extensions.cost) {
-        this.updateGraphqlLimits(res.body.extensions.cost);
-      }
+  const updateLimits = (res) => {
+    if (res.body && res.body.extensions && res.body.extensions.cost) {
+      this.updateGraphqlLimits(res.body.extensions.cost);
+    }
 
-      if (Array.isArray(res.body.errors)) {
-        // Make Got consider this response errored and retry if needed.
-        throw new Error(res.body.errors[0].message);
-      }
+    return res;
+  };
+
+  const maybeError = (res) => {
+    if (res.body && Array.isArray(res.body.errors)) {
+      // Make Got consider this response errored and retry if needed.
+      throw new Error(res.body.errors[0].message);
     }
 
     return res;
@@ -303,19 +305,21 @@ Shopify.prototype.graphql = function graphql(data, variables) {
 
   if (this.options.hooks) {
     options.hooks = { ...this.options.hooks };
-    options.hooks.afterResponse = [afterResponse];
+    options.hooks.afterResponse = [updateLimits];
     options.hooks.beforeError = [decorateError];
 
     if (this.options.hooks.afterResponse) {
       options.hooks.afterResponse.push(...this.options.hooks.afterResponse);
     }
 
+    options.hooks.afterResponse.push(maybeError);
+
     if (this.options.hooks.beforeError) {
       options.hooks.beforeError.push(...this.options.hooks.beforeError);
     }
   } else {
     options.hooks = {
-      afterResponse: [afterResponse],
+      afterResponse: [updateLimits, maybeError],
       beforeError: [decorateError]
     };
   }

The idea is to allow the user to add a hook that will change the behavior of the last (error) hook. This is untested, I don't know if it works.

[infiton]

I'm down to try that; one sec I'll write some tests for it

[infiton]

closing in favour of #673

[lpinca]

@infiton #673 was merged, but on second thought, it is not clear to me why you need to suppress the throwing behavior. If the goal is to read the response body, this can also be done when an error is thrown / the promise is rejected via error.response.body.

Can you clarify? Thank you.

[infiton]

Can you clarify? Thank you.

more ergonomics than anything; I don't want to wrap every graphql request in our sync engine with an error handler and pull the data I need out of the error in the cases where it does throw