Skip to content

Conversation

@ilearnio
Copy link

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

@nordsimon
Copy link
Owner

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

@ilearnio
Copy link
Author

ilearnio commented Jul 24, 2016

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

@nordsimon
Copy link
Owner

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

@ilearnio
Copy link
Author

Looks great! Definitely much better than my approach.

@ilearnio
Copy link
Author

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

@nordsimon
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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

@ilearnio
Copy link
Author

ilearnio commented Jul 24, 2016

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

@nordsimon
Copy link
Owner

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 {}
  }
})

@ilearnio
Copy link
Author

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

@nordsimon
Copy link
Owner

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

@ilearnio
Copy link
Author

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
Copy link
Author

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

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.

2 participants