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

Add option to exclude function properties when encoding objects #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brentd
Copy link

@brentd brentd commented Sep 24, 2017

The behavior of JSON.stringify is to exclude properties on an object that store a function:

JSON.stringify({ f: function(){} }) // '{}'

Current behavior of msgpack-lite:

msgpack.decode(msgpack.encode({ f: function(){} })) // { f: null }

Behavior this PR introduces - excluding functions is the new default:

var obj = { f: function(){} }

msgpack.decode(msgpack.encode(obj)) // {}

// To get the old behavior
var codec = msgpack.createCodec({functions: true})
msgpack.decode(msgpack.encode(obj, {codec: codec})) // { f: null }

Justification

The behavior of JSON is convenient for me, because when prototyping ideas, I often serialize whole ES6 class instances without explicitly picking properties (I'm lazy). msgpack-lite already excludes the class prototype's functions since it uses Object.keys to encode objects, but functions set as properties on the object (like functions that are bound using the common idiom in the constructor) will remain.

class Foo {
  constructor() {
    this.someProp = 123
    this.boundFunc = this.boundFunc.bind(this)
  }

  someFunc() {
    return "hello"
  }

  boundFunc() {
    return this
  }
}

msgpack.decode(msgpack.encode(new Foo())) // { someProp: 123, boundFunc: null }

boundFunc would be sent over the wire as null and is of no benefit.

That said, I realize someone out there might have actually added a custom encoder for functions, so excluding them by default is maybe not the right solution. 100% open to discussing and making any adjustments necessary.

An alternative idea may be a codec option which allows you to exclude certain types from serialization.

This matches the behavior of JSON.stringify
@brentd
Copy link
Author

brentd commented Nov 30, 2017

Closing since there doesn't seem to be any interest.

@brentd brentd closed this Nov 30, 2017
@kawanet
Copy link
Owner

kawanet commented Nov 30, 2017

I'm sorry to the late response. I was pretty busy for those months.
I'm currently working for Issue #76. I don't think it will take a long time to wait until merging.
I'd like to merge the PR #78 at the same time with merging the similar issue #71 .

Issue #71 wants to map undefined to be stripped.

PR #78 wants to map function to null.

Those two seem similar requests. Your code looks pretty clean. LGTM. The option parameter name functions may give confused, however, at the contrast of yet another option made with issue #71 . Could I ask you a pair of option parameter names of those two?

  • ignore_function
  • function_null
  • function_to_null
  • strip_undef
  • ignore_undef
  • etc

I like the new default behavior given with your patch.

var obj = { f: function(){} }
msgpack.decode(msgpack.encode(obj)) // {}

Idea:

  • ignore_function is true per default which removes the property. If it is false, it runs as before.
  • ignore_undef is true/false per default which removes/keeps the property.

I'm more happy when you could also give a code and README for those two!

@kawanet kawanet reopened this Nov 30, 2017
@brentd
Copy link
Author

brentd commented Nov 30, 2017

Sorry @kawanet, I hope I didn't make you feel guilty - I know how it goes. I was just cleaning out my PR queue.

Thanks very much for the feedback. I'll do what I can to address your comments.

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

Successfully merging this pull request may close these issues.

None yet

2 participants