Node: Change JSON.stringify behaviour on certain builtin objects

[Ginden]

This is not a first time when it happens in my work.

We accidentaly called JSON.stringify on Error instance with certain non-standard properties. These objects contained _response field, instance of http.ServerResponse. Stringifyng such objects results in giant JSON that can't be used in future (or at least, I can't think of common use case for stringifying http.ServerResponse).

Therefore, I would suggest "blocking" stringifying instances of following classes:

• WritableStream
• net.Server
• ReadableStream

This blocking can be implemented as throwing in .toJSON method, making .toJSON return subset of object or making it return string "[Object net.Server]" instead of exposing whole internals of an object.

[jsmrcaga]

I don't think having a JSON.stringify() retuning '[Object <type>]' is a good idea, that's more a toString() kind of job. However overriding .toJSON methods can be a good practice for certain types 😃

Could you specify the use case in which you are transporting an http.ServerResponse on an Error object please ? My usual use-case involves transporting only the actual payload of any kind of communication on the Error so It can be logged properly.

[Ginden]

@jsmrcaga We used library that do something like that:

const err = new Error('Woo, HTTP request throwed');
err._response = instanceOfhttpServerResponse;
return Promise.reject(err);

Then we just blindly passed thrown error to logger.

[sagitsofan]

Should i override toJSON function in net.Server and return "[Object net.Server]"?

[devsnek]

we should have toJSON() { return {}; } on all things that can't be re-created from JSON.parse.

[refack]

Why not throw an exception? If the resault is unwanted behavior, I'd call that an error, rather then ignore it.

[devsnek]

@refack because the objects might be in unsavory places. for example from the OP: We accidentaly called JSON.stringify on Error instance with certain non-standard properties. These objects contained _response field, instance of http.ServerResponse. there's no reason to make the whole thing fail.

[sagitsofan]

I agree, we will not want to failed to whole function becouse of one (or more) unwanted instances, returning {} is a good solution.

If agreed i can send a PR with a fixed for this 3 classes.

[Ginden]

I would prefer to know type of stringified object - eg. {"$type": "net.Server"}.

[jsmrcaga]

@Ginden that type of behaviour should be overseen by some bigger instance (TC39?). It changes the whole behaviour for JSON.stringify, and should change it for every object, meaning that if you :

JSON.stringify({
    someProp: 5
});

you would get {$type: "[object Object]", someProp: 5}

[Ginden]

@jsmrcaga Why should it? Defining custom .toJson method to few objects doesn't require TC39 to participate.

Stringifing net.Server instances is almost always unintended. Making these instances return distinguishable output improve debugging and resolve issues with huge strings. Making them return empty object would resolve issues with huge strings, but it can be hard to debug.

[refack]

there's no reason to make the whole thing fail.

That is the Node.js way, fail on uncaught errors. Trying to serialise something that is unserializable is an error.

> var a = {}
undefined
> var b = {}
undefined
> a.b = b
{}
> b.a = a
{ b: { a: [Circular] } }
> JSON.stringify(a)
TypeError: Converting circular structure to JSON
    at JSON.stringify (<anonymous>)
>

A solution could be to make _response non enumerable.

[joyeecheung]

I am not a big fan of controlling behavior through environment variables or command line options but they seem to be good fits to resolve the error v.s. special output decision issue.

EDIT: making underscore properties non-enumerable (suggested by @refack) sounds like a good idea as well

[devsnek]

@refack i'm saying that this shouldn't be an error case at all, not that we should implicitly be catching some error somewhere.

@joyeecheung i̛'m̡ ͢af̴r̴aid

[refack]

@devsnek I get your POV, but _if_ the only three option are (1) keep this as this is (2) do something tricky (3) error. I prefer error, just because of the principal of least surprise.
That's why I hope we find another option.
AFICT any change is going to be semvar-major, that's why I'm in of non enumeration _ properties.