Skip to content
This repository has been archived by the owner on Oct 13, 2018. It is now read-only.

Repeated Field gets casted as Local<ArrayBuffer> #78

Open
webmakersteve opened this issue Aug 1, 2016 · 6 comments
Open

Repeated Field gets casted as Local<ArrayBuffer> #78

webmakersteve opened this issue Aug 1, 2016 · 6 comments

Comments

@webmakersteve
Copy link
Contributor

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: #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
Copy link
Owner

fuwaneko commented Aug 2, 2016

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

@InfinitiesLoop
Copy link
Contributor

InfinitiesLoop commented Aug 2, 2016

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
Copy link
Contributor Author

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
Copy link
Owner

fuwaneko commented Aug 3, 2016

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

@webmakersteve
Copy link
Contributor Author

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

@webmakersteve
Copy link
Contributor Author

PR #79

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

No branches or pull requests

3 participants