Elasticsearch-js: API design: which value should return an API method?

[delvedor]

Option 1

Pros:

• easy to use
• destructuring assignment for the win

Cons:

• Quite a significant breaking change (but is not that hard to migrate)
• Forces the use of HTTP names (less future proof)

client.search({ params }, (err, result) => {
  console.log(result)
  // {
  //   body: Object,
  //   headers: Object,
  //   warnings: Array,
  //   statusCode: Number
  // }
})

Option 2

Pros:

• no breaking change
• future proof

Cons:

• higher cognitive overhead

const { kMeta } = require('@elastic/elasticsearch') // kMeta is a Symbol and you must import it from the client
client.search({ params }, (err, body) => {
  console.log(body) // elasticsearch response
  console.log(body[kMeta])
  // {
  //   headers: Object,
  //   warnings: Array,
  //   statusCode: Number
  // }
})

Option 3

(note the asHttpResponse parameter, the default is false)
Pros:

• no breaking change

Cons:

• change the returned value based on a parameter can cause unexpected side effects
• typescript will complain
• not future proof, if in the future we will add a new transport method we will need to add another parameter to expose its metadata

client.search({ params, asHttpResponse: false }, (err, body) => {
  console.log(body) // elasticsearch response
})

client.search({ params, asHttpResponse: true }, (err, result) => {
  console.log(result)
  // {
  //   body: Object,
  //   headers: Object,
  //   warnings: Array,
  //   statusCode: Number
  // }
})

Option 4

Pros:

• no breaking change
• the symbol is hidden, so less cognitive overhead than option 2
• future proof

Cons:

• higher cognitive overhead

const { getMeta } = require('@elastic/elasticsearch') // getMeta is a function that reads the kMeta symbol inside the body
client.search({ params }, (err, body) => {
  console.log(body) // elasticsearch response
  console.log(getMeta(body))
  // {
  //   headers: Object,
  //   warnings: Array,
  //   statusCode: Number
  // }
})

---

If you want you can "vote" in this way:

• 👍 for option 1
• 😄 for option 2
• 🎉 for option 3
• ❤️ for option 4

[spalger]

The primary reason I'm not a fan of option 1 is that the vast majority of users never need to think about the headers or statusCode returned from Elasticsearch, and I think putting it into the default result undermines that and makes the API a little less friendly.

Something like the other options makes the api easier for projects like Kibana, which do a decent amount of proxying and want to consume the statusCode and headers as they are, but I don't think it's a very common use case. Just something to keep in mind.

[delvedor]

the vast majority of users never need to think about the headers or statusCode returned from Elasticsearch, and I think putting it into the default result undermines that and makes the API a little less friendly.

I can agree with this, but the main issue with option 2 and especially option 4, is that it feels more "magical", while option 1 has a well-defined contract.
Furthermore, from Node v6, you can use the destructuring assignment so that the code will become:

client.search({ ... }, (err, { body }) => {
  console.log(body) // es response
})

Which is not that bad, we can even use the above example as a default in the documentation.

[sqren]

Has something like this been considered?

client.search({ params }, (err, body, result) => {
  assert.equal(body, result.body)
})

That would address @spalger concerns, while still making result available. And then it's backwards compatible afaict.

[epixa]

The benefit of option 1 is that it is a pattern for http clients that is widespread throughout all languages I’ve worked with. Headers and responses codes are a first-class feature of the elasticsearch api (and a critical part of REST im general), so building a client that treats them as second class things is a strange choice to me.

I disagree about the easy of use comment because it’s less consistent with every http client I’m familiar with, which makes it harder for me to use. That’s purely annecdotal though, so take it with a grain of salt.

If not option 1, then option 4 is the only other option I like.

[delvedor]

Thank you for answering!

@sqren promises and callback should return the same object, since you can’t return multiple values from a promise, the unique big object is the only way to support both nicely at the same time.

@epixa, agree with everything! I’ll wait until Monday but very likely option 1 will be the final API.

[spalger]

The benefit of option 1 is that it is a pattern for http clients that is widespread throughout all languages

I don't think this is an http client, that's the difference to me. If this was a generic HTTP client then status code would be important, but it's not...

[delvedor]

If this was a generic HTTP client then status code would be important, but it's not...

Maybe the status code is not that important, but the headers or the warnings are used quite often, and the user should be able to access them easily.
Furthermore, since the headers are an HTTP specific name and we should expose them, I don't see why we shouldn't expose the status code as well.

[spalger]

headers or the warnings are used quite often

That isn't something I hadn't considered, since there isn't any other way to get access to that information. I'd prefer that we resolved to { body, warnings } if we wanted to expose them as an important part of the response.

[stale[bot]]

We understand that this might be important for you, but this issue has been automatically marked as stale because it has not had recent activity either from our end or yours.
It will be closed if no further activity occurs, please write a comment if you would like to keep this going.

Note: in the past months we have built a new client, that has just landed in master. If you want to open an issue or a pr for the legacy client, you should do that in https://github.com/elastic/elasticsearch-js-legacy