non standard error handling in callbacks about intercom-node

[stringbeans]

as a side note, it would be GREAT if the data coming back (either from a promise or callback) could be JUST the response body and not the entire response object.

[hypeofpipe]

This issue is addressed in the beta version. Now, the SDK supports only a promise based approach. https://github.com/intercom/intercom-node/wiki/Migration-guide#no-more-callbacks

[stringbeans]

this is actually very dangerous behaviour. consider the case where a team has linting setup and marks that one of the arguments in the callback is not being utilized (for whatever reason). this will cause very unexpected behaviour if that team member removes the unused arg

[choran]

Hi @mderazon
Just catching up with some older issues and wanted to follow up here.
Are you suggesting that it would be better to just have one version where accept two arguments and not allow the case for just the response?
I would be afraid now that people might be using this one argument method and would break if this was changed.
But I do see what you are saying. I am just not sure at present if it is something we would change. One option might be to remove one of the examples from the docs and leave the one with both arguments.
Will try and do some more digging here but let me know if you have any other thoughts in the mantime
Thanks
Cathal

[rhodgkins]

@choran I really think the error handling should be fixed and released as a new major (breaking) version. This could also include the great suggestion by @stringbeans!

The current way the callback handler is called (counting the parameters provided to the callback) makes for easy errors to occur...
If you just want to send a request are not interested in the response body you'd have code such as this (pretty standard node code):

const event = {
   event_name: 'some_event',
   created_at: Math.round(Date.now() / 1000),
   user_id: 'some user ID'
}

client.events.create(event, err => {
   if (err) {
      // Do something to handle the error
   } else {
      // Success - event created
   }
})

Now with the current error handling this doesn't work:

• If the request errors the code actually crashes (I've created PR #191 to fix this).
• If the request succeeds the whole response data is returned to the callback which, with nodes error first callback style means the // Do something to handle the error will be run - which is not what the consumer of your SDK wanted at all!
• If the request fails (due to error.list in response) it works fine.

[webmobiles]

there is no standard error management ?

try {
  await client....
} cath (err) {
}