cordova-plugin-camera icon indicating copy to clipboard operation
cordova-plugin-camera copied to clipboard

Throw structured errors instead of just messages

Open tobiaswaltl opened this issue 4 years ago • 4 comments

Feature Request

Motivation Behind Feature

If the plugin threw structured errors having an error code and an error message, developers could handle these errors without relying on magic strings representing the error message.

For instance, if the camera permission was not granted on Android devices, the error is currently only '20'. If it was { code: '20', message: 'Camera permission not granted' } instead, it would be more understandable and the error handler could differentiate based on the code.

Feature Description

I suggest passing a structured error object to the onError handle as described above. Furthermore, it would be great to export an enum with all the error codes like

export enum Error {
  NO_PERMISSION: '20',
  USER_CANCELED: '99',
  // ...
}

This would allow for handling errors like

navigator.camera.getPicture(
  () => { /* handle success */ },
  error => {
    switch(error.code) {
      case Error.NO_PERMISSION:
        // show a popup asking to grant the permission
      case Error.USER_CANCELED:
        // do nothing as the user chose to cancel the action
     ...
    }
  },
  cameraOptions,
);

This would break existing error handlers comparing with the message.

Alternatives or Workarounds

Currently, error handling is only possible via comparing the error string with magic strings (i.e. the actual messages).

tobiaswaltl avatar Dec 02 '20 17:12 tobiaswaltl

I assume, this could be a general point for discussion. My gut feeling says that a lot of plugins will use string messages over real error objects.

timbru31 avatar Dec 08 '20 13:12 timbru31

I assume, this could be a general point for discussion. My gut feeling says that a lot of plugins will use string messages over real error objects.

While that might be true, it doesn't mean Apache Cordova shouldn't set a standard. I think it's a good idea to actually create instances of Error objects (or an extension of) when propagating errors.

e.g:

class CameraError extends Error {
    constructor(code, message) {
        super("Code: " + code + ": " + message);
        this.code = code;
    }
}

var x = new CameraError(15, 'test error');
x.message; // Code: 15: test error
x.code; // 15

will report to the console:

Error: Code: 15: test error
    at <anonymous>:1:9

I'm not sure how safe it is to use class style syntax without the use of Babel or some other transpiler... we may have to rely on the prototype syntax which I think does affect the call stack by including the the error constructor, but I think that is minor.

Extending Error has a couple of benefits, namely:

  • Ability to generically test for errors via x instanceof Error
  • We can also test for specific errors via x instanceof CameraError
  • We have the ability to attach additional details onto the error object such as the code property.
  • Ability to enforce consistency

export enum Error {

Enums is also good, I'm surprised this plugin is actually missing them. Other plugins like the cordova-plugin-file has them (although, our projects are not in typescript so they are just simply objects, but serve the same purpose). I would advise against clobbering Error though since that is a browser object.

breautek avatar Dec 08 '20 13:12 breautek

While that might be true, it doesn't mean Apache Cordova shouldn't set a standard.

My comment was not clear enough - I meant our Apache Cordova core plugins. Therefore I was thinking to move this issue to https://github.com/apache/cordova or https://github.com/apache/cordova-discuss for a more general solution.

timbru31 avatar Dec 08 '20 13:12 timbru31

My comment was not clear enough - I meant our Apache Cordova core plugins. Therefore I was thinking to move this issue to https://github.com/apache/cordova or https://github.com/apache/cordova-discuss for a more general solution.

Ah yes, I definitely misunderstood - my apologies. This definitely sounds like a good idea.

breautek avatar Dec 08 '20 13:12 breautek