borc icon indicating copy to clipboard operation
borc copied to clipboard

This shouldn't encode JS `undefined` as CBOR "undefined".

Open Stebalien opened this issue 7 years ago • 8 comments

See: https://tools.ietf.org/html/rfc7049#section-3.8

CBOR undefined means the encoding is undefined. Now, technically, the encoding for undefined is undefined (unless you want to define it to be null) but undefined really shouldn't exist in "good" CBOR objects.

Stebalien avatar Nov 14 '18 21:11 Stebalien

So the options to deal with this would be either to not allow it (which would be OK but inconvenient), or do what JSON.stringify does:

An undefined value in a map is serialised as if the corresponding key is not present at all. An undefined value in an array is converted to null.

> JSON.stringify({ a: undefined })
'{}'
> JSON.stringify([undefined, undefined])
'[null,null]'

rklaehn avatar Nov 14 '18 21:11 rklaehn

Sounds reasonable to me.

Stebalien avatar Nov 15 '18 00:11 Stebalien

I'm in favour of not allowing undefined. I'd prefer having things to fail early, rather than doing implicit things which are then uncovered after long debugging sessions. I'd expect that If you have something undefined it was accidentally and not intentionally.

vmx avatar Nov 15 '18 09:11 vmx

I think failing on discovering undefined sounds like a good solution. For all non things there is null.

The above example from JSON.stringify seems more confusing than predictable tbh.

dignifiedquire avatar Nov 15 '18 12:11 dignifiedquire

As this is a library used in the context of JavaScript where undefined does occur and is legal, I think failing if undefined is encountered is going to lead to confusion, bugs, and inconvenience.

The way the JSON serializer handles this for objects will result in a similar object after parsing. Consider this:

> const orig = {a: undefined}
undefined
> orig.a === undefined
true
> const test = JSON.parse(JSON.stringify(orig));
undefined
> test.a === undefined
true

For the array case, I also agree with the JSON decision, as it gets as close as possible to the original and probably causes very few side effects.

I think that except where there are very compelling reasons, borc should align with the conventions used by other serializers, such as JSON.

Additionally, I think there are many use cases where borc is going to be processing arbitrary data that may only contain undefined in corner cases. This will undoubtedly lead to bugs in production systems, where the fix is to essentially pre-process every object going through borc anyway.

For those reasons, I believe that the undefined behavior from JSON should be applied to borc.

bradisbell avatar Nov 15 '18 14:11 bradisbell

I really don't know what is best here. The principled thing to do would be to reject undefined since it is not representable in cbor, so a js -> cbor -> js transformation will not yield an identical object. Same for things like functions.

But on the other hand in the javascript ecosystem people tend to assume that libraries will just work best effort with whatever they are given. E.g. JSON.stringify just ignores stuff that it can not serialise, such as functions, instead of failing.

What is the current behaviour when trying to serialise a function? Whatever the policy is, it should be consistent.

rklaehn avatar Nov 15 '18 17:11 rklaehn

Currently if you try to serialise a function an error is thrown. Here's an example:

const cbor = require('borc')

const myfun = () => {}
toEncode = {myfun: myfun}

console.log(JSON.stringify(toEncode))
console.log(cbor.encode(toEncode))

The output is:

$ node testfun.js
{}
/home/vmx/src/pl/misc/cborencoding/node_modules/borc/src/encoder.js:430
        throw new Error('Unknown type: ' + typeof obj + ', ' + (obj ? obj.toString() : ''))
        ^

Error: Unknown type: function, () => {}
    at Encoder.pushAny (/home/vmx/src/pl/misc/cborencoding/node_modules/borc/src/encoder.js:430:15)
    at Encoder._pushRawMap (/home/vmx/src/pl/misc/cborencoding/node_modules/borc/src/encoder.js:367:17)
    at Encoder._pushObject (/home/vmx/src/pl/misc/cborencoding/node_modules/borc/src/encoder.js:344:17)
    at Encoder.pushAny (/home/vmx/src/pl/misc/cborencoding/node_modules/borc/src/encoder.js:402:21)
    at Object.encode (/home/vmx/src/pl/misc/cborencoding/node_modules/borc/src/encoder.js:507:21)
    at Object.<anonymous> (/home/vmx/src/pl/misc/cborencoding/testfun.js:7:18)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)

vmx avatar Nov 19 '18 15:11 vmx

Then it seems only consistent to also throw an error on undefined. If this causes too much trouble for some users, introduce a "lenient" mode that just omits stuff that can not be serialized or uses CBOR undefined.

rklaehn avatar Nov 20 '18 20:11 rklaehn