Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept string argument for emitting error events #134

Closed
micnic opened this issue Dec 10, 2014 · 7 comments
Closed

Accept string argument for emitting error events #134

micnic opened this issue Dec 10, 2014 · 7 comments

Comments

@micnic
Copy link
Contributor

micnic commented Dec 10, 2014

I do understand that the events.js's API is frozen and there is a small chance to change something, but I'd like to be able to emit error events using strings, not only Error constructor. Now if an error is emitted with a string we get Uncaught, unspecified "error" event., which is not informative, to receive another message you have specify a new error object with a string message. For me it seems a bit redundant, why not use both methods to specify the emitted errors? In terms of compatibility it should not break any functional, but at least helps to write less and it's quite intuitive.

What do you think about this?

@greelgorke
Copy link

this is so wrong. using strings as errors is a very bad practice. Error objects do have so many advantages. and to put a new Error('some error message') isn't that hard, really. You can distinguish the error from an actual string value.

-100000000

@micnic
Copy link
Contributor Author

micnic commented Dec 10, 2014

@greelgorke, I meant something like this:

EventEmitter.prototype.emit = function (type) {

  var er;

  // other logic

  if (type === 'error') {
    er = arguments[1];

    if (er instanceof Error) {
      throw er;
    } else if (typeof er === 'string') {
      throw new Error(er);
    } else {
      throw Error('Uncaught, unspecified "error" event.');
    }
  }

  // other logic
};

The error objects can be created based on the string message that is provided when the error event is emitted.

@timoxley
Copy link
Contributor

Note that the error's .stack trace points to the line of code where the new Error happens, so in your case, it will point inside the emit function first, rather than the actual line of code that generated the error.

You can hack the stack trace string, but that seems like a bad idea to enable a small convenience.

@micnic
Copy link
Contributor Author

micnic commented Dec 10, 2014

@timoxley, you are right, at least the line where the error event was emitted is in the stack trace on the second place, seems like my suggestion is not so ideal as I thought :)

@greelgorke
Copy link

@micnic it not even like that in constructs like:

function(){
  var ee = new EventEmitter()

  db.doSomething(query, function(err){
     if(err) return ee.emit('error',err);
  })

return ee;
}

@cjihrig
Copy link
Contributor

cjihrig commented Dec 10, 2014

Emitting errors based on strings is a bad idea.

@cjihrig cjihrig closed this as completed Dec 10, 2014
@petkaantonov
Copy link
Contributor

FWIW, you actually don't need to hack anything, the API supports setting a start point for the trace:

    } else if (typeof er === 'string') {
      er = new Error(er);
      Error.captureStackTrace(er, this.emit);
      throw er;
    }

minervapanda pushed a commit to minervapanda/node that referenced this issue Oct 9, 2016
boingoing pushed a commit to boingoing/node that referenced this issue Apr 6, 2017
 - Add a `napi_get_cb_info` function to get args length, args, this, and data in a single call
 - Move the static exception to a class member to enable inlining of the `lastException()` method.
 - Refactor the `CallbackWrapper` group of helper classes to avoid most (non-inlinable) virtual function calls
 - Remove use of `v8::TryCatch` (via `NAPI_PREAMBLE`) from several places where it shouldn't be needed.

Together, these optimizations reduce the overhead of every JS-to-C++ call through the NAPI layer by approximately 50%.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants