Seeing a bunch more weirdness in asJSON

[robin-anil]

This is converted with

{ fieldsOnly: true, longs: Number, enums: String, defaults: false, arrays: true }

Array entry 0 is missing color, secondary state and orderings.

Array entry 1 is missing all the fields.

I also have a case where enum is not converted to String.

[yoni-tock]

It also looks like the enum value 0 isn't being converted to a string, but anything greater than zero is being converted. For example, before toJSON index 0 has icon 0 and index 1 has icon 3 but after toJSON index 0 has icon 0 and index 1 has icon "TRANSFER".

[robin-anil]

Also including #616 There needs to be a better differentiation between 0 and unset.

[dcodeIO]

The initial thing is probably due to asJSON removing any default values when { defaults: true } isn't set. Hrm.

There, asJSON doesn't really know which property is on the message itself and which one is on the prototype - hence it handles both the same. Testing this through hasOwnProperty would have a significant performance impact.

I'll probably throw the current converter stuff away again and try something that iterates over the source's properties instead.

[robin-anil]

Thank you! Once this is done, we will be in a really amazing pace, can't wait to get all the big performance improvements with this release.

All these are painful because these are breaking changes and we have a large code base with hundreds of messages.

[dcodeIO]

May I ask what's the exact format of the json data you are using? Does it somewhat correspond to the official proto3 json mapping?

The reason why I'm asking is that I'd like to start with the official JSON mapping and then build onto that, ideally eliminating as many of the options as possible.

For example, the official json mapping always converts:

• enums to strings
• longs to strings
• bytes to base64 strings
• and it always removes default values to safe space

[robin-anil]

Yes, official mapping will work with few options for exceptions.
we use number for long,(which we can change over in a different pass) and add defaults for arrays(which again is too daunting for this pass).
The mappings don't specify extensions which we use heavily. I would love to keep the current format for that where key is the type.
We can't remove default values if present because that is a proto2 thing and that will be a severe breakage. The only rule we care is that specified values need to be present in JSON form

[robin-anil]

@dcodeIO just checking to see if you have made some progress on this.

[dcodeIO]

Not yet, unfortunately. I really want 6.4.X to become stable now, before going for a 6.5.X with new converters.

[dcodeIO]

Additional notes:

• asJSON is now toObject to mimic official protobuf APIs. This is available both statically (with an additional message parameter) and on instances.
• .from is now just an alias of .fromObject.
• There is now toJSON on message instances (utilized by JSON.stringify) that uses default options (enums=longs=bytes=String) to be somewhat compatible to official json conversion.
• Currently, there is no more bytes: Buffer option. This has been an undocumented gRPC thing, but I'd rather like to drop it than to keep this around forever as I don't see a benefit of having plain objects populated with buffers now that default values are on prototypes anyway. I assume that this has been used solely to work around not having proper default values.
• There is no more fieldsOnly: true|false option.
• There is now objects: true that does for maps what arrays: true does for repeated fields.
• There are no more conversion options for object -> message conversion because unnecessary.

Also worth mentioning: This generates somewhat longish code now, but it is 2-3 times faster than what we had before iirc.

Let me know what you think. It's probably still buggy somehow, but it passes the slightly extended test case (which failed for maps btw. with the old converters).

[robin-anil]

Sweet! Let me try it

[robin-anil]

from is throwing an error. I have a proto which has a message as a nested field

message MyProto { optional MyNestedProto proto = 1 }
where MyNestedProto has a long id in it.

MyProto.from({}) is crashing, its trying to parse MyNestedProto

[robin-anil]

still throwing...

[dcodeIO]

This is working for me:

var root = protobuf.Root.fromJSON({
    nested: {
        MyProto: {
            fields: {
                proto: {
                    type: "MyNestedProto",
                    id: 1
                }
            }
        },
        MyNestedProto: {
            fields: {
                aLong: {
                    type: "uint64",
                    id: 1
                }
            }
        }
    }
}).resolveAll();

var myProto = root.MyProto.from({});
console.log(myProto);

What's the difference?

[robin-anil]

Hmm. I don't know...
I am seeing this

[robin-anil]

Fixed it. Looks like a version skew. I am working off git+https://... in package.json It does not really work well on branches.
All good, All our tests are passing!

[robin-anil]

I have gone through several features so far in my giant protobuf conversion branch and I have yet to find anything obviously broken with the entire slew of apps we have.

I will have more eyes during code-review and QA. I will post if there are more issues.

Closing the bug. please push 6.5.0 when you get a chance, we would prefer not to build production assets directly from git master.