feat: make circular input errors for EJSON expressive

[addaleax]

Give errors roughly in the style of those thrown by
JSON.stringify() on modern Node.js/V8 versions, where the path to
the property and its circularity are visualized instead of just
recursive indefinitely.

(See the test file for an example of how that would look like)

This is just one suggested solution – it would be nice to have some
kind of better error in these cases, and I think actually displaying
the path would be nice in terms of UX, but I can also see an argument
for avoiding the extra bits of complexity here.

NODE-3226

Description

What changed?

[nbbeeken]

As always thanks for helping us improve our BSON lib it is much appreciated, here's some thoughts I came across while testing this locally:

> var obj = {a:3}
undefined
> obj.b = obj
<ref *1> { a: 3, b: [Circular *1] }
> BSON.EJSON.stringify(obj)
Uncaught TypeError: Converting circular structure to EJSON:
     -> (input) ->  -> b
           \-----------/
    at serializeValue (/Users/neal/code/drivers/js/bson/lib/extended_json.js:148:19)

Here's the little example I was playing around with, I think the blank space between the two arrows before b is a bug as well as the leading arrow before (input). Is the expectation to get something like: (input) -> b?

I am thinking it is difficult enough to build this path correctly that it warrants a separate PR / project to ensure we can get it right. Meanwhile I think it would be great to merge in the circular detection logic to avoid the "Max Stack" error that currently occurs.

Is there potentially an alternative here where we use util.inspect on the object with the circular reference and let node handle printing out a useful representation? I see I can get the <ref *> and [Circular *] syntax if I use that, maybe there's something useful in that.

[addaleax]

Here's the little example I was playing around with, I think the blank space between the two arrows before b is a bug as well as the leading arrow before (input). Is the expectation to get something like: (input) -> b?

Yes, that’s true.

I am thinking it is difficult enough to build this path correctly that it warrants a separate PR / project to ensure we can get it right.

I don’t really think so, tbh – yes, there were off-by-one errors here, and it’s fair to say that not adding more extensive tests for those situations was not great, but this isn’t exactly rocket science either. I’ve fixed this and added the tests, but of course that doesn’t mean that this can’t still be split out into a separate PR, especially if this helps get the baseline check released a bit faster.

Meanwhile I think it would be great to merge in the circular detection logic to avoid the "Max Stack" error that currently occurs.

Yeah, that’s okay – if you want me to do that, just be explicit about it. :)

Is there potentially an alternative here where we use util.inspect on the object with the circular reference and let node handle printing out a useful representation? I see I can get the <ref *> and [Circular *] syntax if I use that, maybe there's something useful in that.

To be honest, I feel like that using util.inspect output as-is would be way too verbose sometimes. However, if you like the format, the error message string here could also of course be adjusted to say something like foo -> <ref*> bar -> baz -> [Circular*] or something like that.

[nbbeeken]

off-by-one errors here

Ah now that I see the fix I agree it was a small thing, thanks for adding those tests.

To be honest, I feel like that using util.inspect output as-is would be way too verbose

I think that is fair, and given my revision on how difficult I thought this was I don't think we need to change the format it is helpful as is (sans one small optional nit).