graphql-client icon indicating copy to clipboard operation
graphql-client copied to clipboard

Hooks

Open ilearnio opened this issue 8 years ago • 14 comments

Added onRequest and onResponse hooks that might be helpful when need to modify requests before sending, or log req/res objects etc. Updated the readme as well

ilearnio avatar Jul 19 '16 23:07 ilearnio

I'll take a look at this. A bit curious if I should support this with another api

nordsimon avatar Jul 24 '16 14:07 nordsimon

Do you mean whether it could break something in an existing API? If so then no, it shouldn't. I simply attached a request object to a variable and added two triggers of hooks. I have also committed some breaking changes to the master branch where I replaced throw with console.error for GraphQL's data.errors array because I needed to catch this array as is in my code. You can take a look and maybe consider to merge

ilearnio avatar Jul 24 '16 19:07 ilearnio

I thought it through and i would like to implement something like this instead

Initialize the client

var client = require('graphql-client')({
  url: 'http://your-host/graphql',
  headers: {
    Authentication: 'Bearer ' + token
  }
})

Use the promise API

var variables = {
  query: "Search Query",
  limit: 100,
  from: 200
}

client.query(`
query search ($query: String, $from: Int, $limit: Int) {
  search(query: $query, from: $from, limit: $limit) {
    took,
    totalHits,
    hits {
      name
    }
  }
}`, variables)
.then(function(body) {
  console.log(body)
})
.catch(function(err) {
  console.log(err.message)
})

or use the event emitter api

var variables = {
  query: "Search Query",
  limit: 100,
  from: 200
}

client.query(`
query search ($query: String, $from: Int, $limit: Int) {
  search(query: $query, from: $from, limit: $limit) {
    took,
    totalHits,
    hits {
      name
    }
  }
}`, variables)
.on('request', function(req) {})
.on('response', function(res) {})
.on('data', function(body) {
  console.log(body)
})
.on('error', function(err) {
  console.log(err)
})

What do you think about that ? Your changes wouldn't break anything but i didn't like the the api

nordsimon avatar Jul 24 '16 19:07 nordsimon

Looks great! Definitely much better than my approach.

ilearnio avatar Jul 24 '16 20:07 ilearnio

But I think that the event listener should be returned by the initializing function rather than returning it every time when calling client.query. I personally like the way client.query works now, it returns a promise which is great, especially when combining it with ES7 async/await

ilearnio avatar Jul 24 '16 20:07 ilearnio

Yeah, I do aswell. I tried to support both ways but it became a mess. Maybe adding it to the client would help. But then it won't get connected to a specific query which is also bad

nordsimon avatar Jul 24 '16 20:07 nordsimon

Okey, so this is where i am now. I added a third argument to the query method which will send the req and res object. I also changed the error to contain the errors array as a originalErrors property

var variables = {
  query: "Search Query",
  limit: 100,
  from: 200
}

client.query(`
query search ($query: String, $from: Int, $limit: Int) {
  search(query: $query, from: $from, limit: $limit) {
    took,
    totalHits,
    hits {
      name
    }
  }
}`, variables, function(req, res) {
  if(res.status === 401) {
    throw new Error('Not authorized')
  }
})
.then(function(body) {
  console.log(body)
})
.catch(function(err) {
  console.log(err.message)
})

nordsimon avatar Jul 24 '16 21:07 nordsimon

Since every query has their own req and res i don't want to add it to the client init

nordsimon avatar Jul 24 '16 21:07 nordsimon

The third parameter is a nice per-request solution. But the reason I need to modify req for every request to my graphql api is because I need to send different headers depending on whether a user is logged in or not, so I want to implement it once per application

ilearnio avatar Jul 24 '16 21:07 ilearnio

Hm, i see. Maybe i can make the headers argument optionally take a function, so in this way you could write something like

var client = require('graphql-client')({
  url: 'http://your-host/graphql',
  headers: function() {
    if(isLoggedIn) return {Authorization: 'Bearer ' + token}
    else return {}
  }
})

nordsimon avatar Jul 24 '16 22:07 nordsimon

Yes, please. Or maybe client.on('req', ...), which would be a more general solution? Unless it's too tedious to implement

ilearnio avatar Jul 24 '16 22:07 ilearnio

I ser your point. However i dont like the idea to mutate the request object by using a hook. It's more of a hack. I think a function for the headers that gets reevaluated on each request is a more straightforward and obvious solution

nordsimon avatar Jul 25 '16 07:07 nordsimon

I had some time to add client.on method, committed it to my master branch. But it is almost completely rewritten now.

I'm planning to add some more features to it, e.g. I'm going to add QL method which will automatically inject quotes for strings and do other useful stuff:

// Current approach
const result = await client.query(`mutation {
  signup (username: "${ username.replace(/"/g, '\\"') }" password: "${ password.replace(/"/g, '\\"') }") {
    token
  }
}`)

// VS

const result = await client.QL`mutation {
  signup (username: ${ username }, password: ${ password }) {
    token
  }
}`

// One more possible way

const result = await client.QL`mutation {
  signup (${ { username, password } }) {
    token
  }
}`

ilearnio avatar Jul 26 '16 22:07 ilearnio

It was a stupid idea. Shit, I completely forgot about variables! So it seems to be finished now after a few more fixes. You can check out my master branch and perhaps consider to merge

ilearnio avatar Jul 27 '16 16:07 ilearnio