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

pbjs static-module and from (JSON) #588

Closed
mkmik opened this issue Dec 27, 2016 · 4 comments
Closed

pbjs static-module and from (JSON) #588

mkmik opened this issue Dec 27, 2016 · 4 comments

Comments

@mkmik
Copy link
Contributor

mkmik commented Dec 27, 2016

protobuf.js version: 6.3.1

How can read JSON data with static-module ?

With dynamic modules I can just do:

protobuf.load("src/foo.proto").then(function(root) {
  var Bar = root.lookup("bar");
  return bar.from({baz: {qux: 3}});
});

and it yields Message { baz: Message { qux: 3 } }

However, the code generated with pbjs -t static-module only defines the create method, which doesn't create the right child object types.

This is particularly painful when having things like oneof which depend on functionality defined in the generated type's protototype to guide the encoding (and thus basically encoding an empty message since it cannot determine which field of the oneof to render).

Should we just generate the from method ?

@dcodeIO
Copy link
Member

dcodeIO commented Dec 28, 2016

I have also been thinking about how to handle this properly. Currently, converters rely on reflection information to find out how to handle field values, which is why it isn't supported for static code yet as it has no reflection capabilities.

Not only for performance, converters would benefit from code generation just like encoders and decoders do, which would also allow us to generate the missing from/asJSON methods statically. The reason why I didn't implement this yet is that static code is quite longish already, and converters would allow for alternatives: Field names could be mapped to converter functions (i.e. something like ensureLong for long fields, maybe located within runtime util) instead of code generation, at the cost of additional function invocations and hashmap lookups - or this could be implemented in a hybrid form eliminating just the lookups.

Haven't come to a conclusion, yet, unfortunately.

@mkmik
Copy link
Contributor Author

mkmik commented Dec 28, 2016

I wouldn't mind using the reflection code; I use the generated code mainly because:

  1. it supports TS.
  2. when rendering to string it shows the message type name.

would it be possible to achieve these goals in some different way?

@dcodeIO
Copy link
Member

dcodeIO commented Jan 3, 2017

Closing this for now as 6.4.0, which is now released, should solve the issue. Feel free to reopen the issue if necessary.

@dcodeIO
Copy link
Member

dcodeIO commented Jan 3, 2017

@mmikulicic: This now generates named constructors just like static code does and it should already be possible to use a static .d.ts with its json-module counterpart.

See: https://github.com/dcodeIO/protobuf.js#descriptors-vs-static-modules

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

2 participants