Handle top-level null values

[chrisguttandin]

Hi @sindresorhus,

I hope it's fine to submit a pull request without a corresponding issue.

This pull request fixes the problem that serializeError() can't currently be called with a null value. In other words serializeError(null) will throw a "Cannot convert undefined or null to object"-error.

Of course it doesn't really make sense to have null as an error but unfortunately null gets sometimes thrown instead of an error object. A workaround which would work already is to use a ternary expression to guard the call to serializeError() like this: (error === null) ? null : serializeError(error). But since the library already contains code to handle other edge cases I thought it's fine to add another 14 characters to handle this one as well.

Please let me know if you want me to change this pull request in any way.

[sindresorhus]

This function should definitely never return null. People would expect an error to be returned, so either we should throw a more user-friendly error that null was thrown, so they can fix it, or we should gracefully handle it by wrapping null in an error, by for example, using https://github.com/sindresorhus/ensure-error

I think I would prefer to just fail loudly. No one should intentionally throw null. But I'm happy to discuss.

[chrisguttandin]

Hi, thanks for the fast reply.

I do agree that one should never throw anything else than an error. I do myself use a couple of linting rules (including some that have been written by you - many thanks for that) to make sure that this never happens. However even browser vendors throw null sometimes. If you try to decode some unsupported audio in Safari using the Web Audio API it may for example throw null. https://github.com/chrisguttandin/standardized-audio-context/blob/master/test/expectation/safari/any/audio-context-constructor.js#L1089

I guess it depends how you interpret the name of this package. Does serialize-error mean serialize-error-instances or does it mean serialize-anything-that-can-be-thrown-as-an-error?

In my opinion it is currently written in a way which looks more like it is trying to handle anything that can be thrown as an error.

console.log(serializeError(undefined)); // logs undefined
console.log(serializeError('text')); // logs 'text'
console.log(serializeError(17)); // logs 17
console.log(serializeError(true)); // logs true
console.log(serializeError(false)); // logs false
console.log(serializeError({ some: 'object' })); // logs { some: 'object' }

console.log(serializeError(function X () { })); // logs [Function: X]

console.log(serializeError(null)); // throws a TypeError

It will only throw an error if null is provided as an input. All other cases are handled gracefully.

I personally use this library as a toJSON() alternative. That's why I tried to align my "fix" with the behavior of JSON.stringify().

console.log(JSON.stringify(undefined)); // logs undefined
console.log(JSON.stringify('text')); // logs 'text'
console.log(JSON.stringify(17)); // logs 17
console.log(JSON.stringify(true)); // logs true
console.log(JSON.stringify(false)); // logs false
console.log(JSON.stringify({ some: 'object' })); // logs { some: 'object' }
console.log(JSON.stringify(function X () { })); // logs undefined
console.log(JSON.stringify(null)); // logs null

Please don't get me wrong. In the end it's your library and I'm thankful for the work you put into it. Whatever you decide will work for me as well. As mentioned in my initial comment, I could of course also make sure to never call serializeError with null as input.

[sindresorhus]

You have a good point. However, I think the existing behavior is not optimal either. I think the optimal default is to always return an object no matter what, as having a consistent return value is important. We can add an option to get the current behavior, including null. What do you think? Maybe called passthrough.

[sindresorhus]

Actually, there's 3 possible behaviors:

• Always return an object.
• Passthrough (what you want).
• Throw on non-object input.

Would be good to combine this and #25 into one option to decide the behavior.

[sindresorhus]

Let's continue the discussion in #25. I'll merge your PR for now as it's correct considering the current behavior.