Replace lodash methods

[roggervalf]

No description provided.

[roggervalf]

I would expect that err gets into this line so converting it as a string should be sufficient

[manast]

Hmm, it is not really the same actually. As the issue here is that we do not know what err is. We are doing a lousy check to see if err contains a message, if not we just try to convert whatever err is to a string.

[roggervalf]

looks like this event is only receiving Error objects https://nodejs.org/api/process.html#event-uncaughtexception, do we have another example when this is not true? If not looks like ${err} will be sufficient 🤔

[manast]

Ok, but then why are we doing this:

if (!err.message) 

If err is guaranteed an Error, then it should not be needed to do that check I think.

[roggervalf]

yeah agree, let me remove that check

[manast]

Good initiative. I think we need to think more about toString, or worst case implement a similar function ourselves.

[manast]

Hmm, it is not really the same actually. As the issue here is that we do not know what err is. We are doing a lousy check to see if err contains a message, if not we just try to convert whatever err is to a string.

[manast]

I think it would be beneficial to refactor this code into a separate function (invertObject for example) so that we can more easily understand what this code is actually doing.

[manast]

I wrote this simple experiment:

process.on("uncaughtException", function (err) {
  console.log("Caught exception: " + err);
  console.log(err);
  console.log(err.stack);
});

throw new Error("This will crash");

// throw ["This will crash too!"]

It is actually not guaranteed that err will be an Error, it can be anything that has been thrown... so the correct signature should be:

process.on("uncaughtException", function (err: any)

Now, I do not know if there is a lot of value in using the lodash toString method anyway, as at least from BullMQ we should always make sure that we throw errors and not other things, however it is important to make this handler crash proof, but I think it is too with the new code.

[manast]

This makes me a bit nervous, not sure if this is the correct thing to do. So if the error does not include .message we assume it is not an error object, but more likely just a string, and thus we create a new error here, which will create also an error stack. If we do not do this, then we call errorToJSON with a string , will this yield this a correct "value"? This uncaughException is emitted in userland code, out of our control... can we track who did add this code to see if we can understand the original intend?

[roggervalf]

I can see that this change was added last year with worker threads support 0820985#diff-dba50fb04784a53f9826c6cb96929de2d71329d82f1d97418a13fc54a955f8d5R37-R39

[manast]

I think that in order not to create a regression (even though no test is testing this feature), we should implement our own toString then if we want to get rid of lodash.

[manast]

ah ok, you did already 👍