fuwaneko / node-protobuf

Google Protocol Buffers wrapper for Node.js [UNMAINTAINED]
181 stars 42 forks source link

Repeated Field gets casted as Local<ArrayBuffer> #78

Open webmakersteve opened 8 years ago

webmakersteve commented 8 years ago

I was taking a look at the underlying c++ to try to debug what was happening to my serialized proto when I had a repeated field. Essentially, I expected a repeated field to be parsed as an array of values. Instead, I was finding that my repeated fields got casted as an object-like value, like below:

{
   "r": {
      0: "foo",
      1: "bar",
      2: "baz"
   }
}

Is this the expected functionality? Doing it this way requires a manual iteration over the object so I can convert it to an Array to be handled.

I see this functionality was created with this PR: https://github.com/fuwaneko/node-protobuf/pull/64

I think TypedArray support is a good thing to have, but it seems to disrupt how this should be parsed in JavaScript. I'd like to know your thoughts, though.

Thank you,

fuwaneko commented 8 years ago

This is definitely a bug, TypedArrays should be created only for numeric values.

InfinitiesLoop commented 8 years ago

The problem is that a typed array does not JSON serialize the way you'd expect:

> JSON.stringify(new Uint8Array(3)) '{"0":0,"1":0,"2":0}'

I would have wanted:

[0,0,0]

I get that there are efficiencies with typed arrays, but if you are going to be sending your deserialized protobufs on to other systems that are expecting more traditional/plain javascript objects, you're going to have a hard time. It's reasonable to expect a repeated field to be represented as a plain old array. To fix, I could enumerate every object in a deep way to convert these typed arrays into plain arrays, but that will be costly and hurt performance too much for my needs.

What would you think about adding an option as a bool property to the proto wrapper that enables/disables typed arrays? That or an optional parameter to the parse call?

webmakersteve commented 8 years ago

I think I may have used an example that wouldn't happen. @InfinitiesLoop's example is the one I'm running into.

Looking at the C++, you're right. It will only make a typed array if the field type in the descriptor is numeric.

fuwaneko commented 8 years ago

@InfinitiesLoop I think bool property to the parse call would be the most suitable solution.

webmakersteve commented 8 years ago

@fuwaneko I just made a branch in my fork with that. I'll submit a PR for your review now.

webmakersteve commented 8 years ago

PR #79