better error handling

[JohnDoePBabu]

I noticed that currently errors are neither captured, handled, or propagated.
This could be replicated by calling any method with an invalid token (there are many other ways, of course).

if (!error & response.statusCode === 200) { callback(JSON.parse(body)) }

Also, we aren't following the NodeJS error-first-callback practice.

We could modify this by introducing a breaking change and passing the error as the first argument of the callback.

callback(err, JSON.parse(body))
or make it backward compatible by passing error as the second argument. This would be a departure from the usual node JS callback conventions though.

thoughts?

[warengonzaga]

@JohnDoePBabu what is your best approach for handling error? I didn't add the error handling while making the project since I want to understand how BMC API works. Do you have any suggestion since we are going to use GOT might has better error handling than request.

[warengonzaga]

Hi @JohnDoePBabu I'm assigning this to you...

[JohnDoePBabu]

@warengonzaga We can do one of the below

1. Catch error from Axios and pass the error as the first callback

        try {
            const response = await requester.get(path, {
                headers: {
                    Authorization: 'Bearer ' + this.access_token,
                }
            });
            callback(null, response.data); 
        } catch (err) {
            callback(err);
        }

Pros:

• With callback and Node JS, error first is the real recognized approach.

Cons:

• For people currently using the library, this will be a very big breaking change.
  if they are currently using the module as

coffee.Subscriptions((data) => {
    console.log(JSON.stringify(data));
  });

They will have to change to

 if (err) // do something with error
 else console.log(JSON.stringify(data));
  });

2. Catch error from Axios and pass the error as a second callback

 if (err) // do something with error
 else console.log(JSON.stringify(data));
  });

Pros:

• Backward compatible with the previous version of BMC JS. No breaking change for current users.
  Cons:
• Against Node JS convention.
• counter-intuitive for future users

3. Move to Promise style of handling error by rejecting

        try {
            const subcriptions = await coffee.Subscriptions();
        } catch (err) {
            // do something with error
        }

Pros: cleaner than callback
Cons: Again breaking change

4. Support both callback and promise
   Check if the user has sent a function as a second argument if so return callback else use a promise.
   Again for the callback version, we will have to decide if we want option 1 or option 2.
   So the user can do both

coffee.Subscriptions((err, data) => {
 if (err) // do something with error
 else console.log(JSON.stringify(data));
  });

and ...

        try {
            const subcriptions = await coffee.Subscriptions();
        } catch (err) {
            // do something with error
        }

[warengonzaga]

What do you think is better @JohnDoePBabu? and more standard?
What I see your option number 3 is more okay to me. Breaking changes is natural since we are planning to release the v2.
We need clear documentation once these changes implemented so they can easily adapt to the latest version.

[JohnDoePBabu]

Hi @warengonzaga,

I noticed that when there is no data, eg: no supporters, BMC API sends a 200 response with an error in the response body.
such as
{ "error": "No subscriptions" }
Shall we implement it the same way or do you prefer to

• send an empty array
• throw an error

[warengonzaga]

Hi @JohnDoePBabu,

I would prefer to throw an error message instead of empty array.

[warengonzaga]

All good closing this issue with merge #18