Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

This shouldn't encode JS undefined as CBOR "undefined". #36

Open
Stebalien opened this issue Nov 14, 2018 · 8 comments
Open

This shouldn't encode JS undefined as CBOR "undefined". #36

Stebalien opened this issue Nov 14, 2018 · 8 comments

Comments

@Stebalien
Copy link

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.

@rklaehn
Copy link

rklaehn commented Nov 14, 2018

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]'

@Stebalien
Copy link
Author

Sounds reasonable to me.

@vmx
Copy link
Collaborator

vmx commented Nov 15, 2018

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.

@dignifiedquire
Copy link
Owner

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.

@bradisbell
Copy link

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.

@rklaehn
Copy link

rklaehn commented Nov 15, 2018

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.

@vmx
Copy link
Collaborator

vmx commented Nov 19, 2018

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)

@rklaehn
Copy link

rklaehn commented Nov 20, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants