Open Gozala opened 2 years ago
The problem with being flexible with undefined
is that the codecs are attempting to be as symmetrical as possible, and that goes for silently ignoring fields. What goes in should be what comes out. The more scope for mismatching input:output the more scope for bugs appearing when round-trips get involved; we keep on seeing these problems in practice.
But, that's obviously not as clean when we hit the real world, and we have a couple of instances of sloppy behaviour:
prepare()
on js-dag-pb was introduced for this reason - the exact form you get in a round-trip is very specific and not usually exactly what you want to provide up-front, so prepare()
is there to take sloppy input and produce the exact form that you'd get during a round-trip.toGeneral()
to get the input:output symmetry.So there's two approaches we could take to alleviate the pain:
prepare()
an object, to get the symmetry, that would be a nice ideal solution; but it's going to have perf costs of course.BlockCodec
, or some kind of configuration option that would let you change the way the encoder works. Then we could push the object walking right down into the encoder and switch according to options. We already have a special option for undefined
and could extend that to do something else with it. The own properties thing will be a bit more difficult but I can imagine it also being achieved with a config option.How important is this? Is this simply a quality of life feature, or is it getting in the way and being a problem for downstream users? I could do some work on the CBOR encoder to expose options to do this but I don't know how to prioritise this and I have other things on my plate vying for attention.
How important is this? Is this simply a quality of life feature, or is it getting in the way and being a problem for downstream users?
It is pretty problematic at least in the context of UCAN based RPC system because service implementation:
undefined
somewhere and/or Error
instance. It would be really difficult to enforce that handlers never include such values and I fear it is something that would show up in the production as opposed to during development.
undefined
somewhere encoding fails and general error occurs as opposed to 99 successes and one error. That is really bad, if only one invocation failed and rest succeeded that would have been not as bad.At the moment I'm working around this by going over the returned values and:
undefined
s.Error
instance into a plain object.This is pretty unfortunate, because same data structure gets traversed once before handing it off to CBOR encoder which then traverses it again to actually encode.
In terms of ideal solution it would something along the lines of:
CBOR.encode
would take optional replacer
argument equivalent to one you can pass to JSON.stringify
replacer
would affect performance, but if not present current code path could be used.CBOR.loose.encode
or possibly other module all together that behaves similar to JSON.stringify
, that is has optimized code path (without replacer
) and is able to leave out undefined
s and avoid throwing on Error
instances.The problem with being flexible with undefined is that the codecs are attempting to be as symmetrical as possible, and that goes for silently ignoring fields. What goes in should be what comes out. The more scope for mismatching input:output the more scope for bugs appearing when round-trips get involved; we keep on seeing these problems in practice.
One could argue that leaving out undefined
meets that criteria CBOR.decode(CBOR.encode({ omit: undefined })).omit === undefined
is true even if Object.keys(input)
would be different.
While I generally prefer early errors, I'm still finding that JSON.stringify
strikes a better balance here. It is hard to force no undefined
s if you're interfacing with user space code.
This input:output
argument makes no sense to me in regards to Error
instances however because it is not enforced on any other class instances. So new class Point { constructor(x, y) { this.x = x; this.y = y }(0, 0)
would produce {x, y}
but new Error('boom')
would fail to encode.
You could probably start rejecting any class instances too, but I'm not sure if that would be an improvement.
+1 to a "cbor.prepare" or similar, or maybe a flag to do the sanitization.
I've been working on UCAN based RPC system that encodes request / responses into set of CBOR blocks in a CAR. Unfortunately it proved challenging to not encounter errors from CBOR codec due to some field set to
undefined
or due to some built-in data type like Error been referenced.Could we add an option to make CBOR codec behave more or less like JSON.stringify does, specifically
undefined
values