qlik-oss/enigma.js

rpc.js:165 throws inadequate error

carlhannes opened this issue · 7 comments

carlhannes commented

Description

In https://github.com/qlik-oss/enigma.js/blob/master/src/rpc.js#L165 "Not connected" is thrown without any status code or likewise, making it hard to identify.

Steps to Reproduce

Close an enigma.js connection unexpectedly and this will occur

Expected behavior

A more proper error such as an object or likewise with a status id or something easier to identify

Actual behavior

An unhandeled string error

Environment

Doesn't matter

Library
[ ] Node.js
[X] Browser
Operating system
[ ] Windows
[X] OSX
[ ] Linux
Qlik Sense
[X] Desktop
[X] Enterprise

Versions

  • Node.js:
  • Browser:
  • Qlik Sense:
  • Operating system:
  • [Other relevant versions]
peol commented

A more proper error such as an object or likewise with a status id or something easier to identify

You'll get an exception with the "Not connected" message, do you mean you'd like to have a more descriptive error message, e.g. ERR100: API xyz tried to send a message on a closed session? I don't think we'd like to remove the exception altogether.

carlhannes commented

It would be nice if you assigned a status code or something like that before throwing it, so it can be handled easier when we're catching it

peol commented

There is no status code property on an exception: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error so I'm unsure what the ask is if it's not a "better message" :)

peol commented

After discussions we've agreed to do the following:

Since it would be a breaking change changing the class returned (people might check err instanceof Error), and since message is the only reliable output we cannot change that, we'll:

  • Introduce a error.code property that'll be -1 in the not connected case which people can rely on instead.
  • 🎉1
carlhannes commented

Superb! Thank you :)

stoffeastrom commented

Makes sense, since we do this onClose already :)
https://github.com/qlik-oss/enigma.js/blob/master/src/rpc.js#L67

peol commented

enigma.js@2.3.2 includes this addition.