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

Has json-module with reflection changed since 6.8? #828

Open
hnfmr opened this issue Jun 10, 2017 · 9 comments
Open

Has json-module with reflection changed since 6.8? #828

hnfmr opened this issue Jun 10, 2017 · 9 comments

Comments

@hnfmr
Copy link

hnfmr commented Jun 10, 2017

protobuf.js version: 6.8

I have been using protobuf.js to generate json-module with reflection information, it's been working fine until 6.7.3 and a recent upgrade to 6.8 is throwing errors.

For example I use the following command to generate json-module:

pbjs --target json-module -w commonjs --keep-case -o dest.js source.proto

The errors:

$ mocha test/protobuf.spec.js
/Users/xxx/xxx/node_modules/xxx/xxxx/dest.js:6
var $root = ($protobuf.roots["default"] || ($protobuf.roots["default"] = new $protobuf.Root()))
                                                                         ^

TypeError: $protobuf.Root is not a constructor
    at Object.<anonymous> (/Users/xxx/xxx/node_modules/xxx/xxxx/dest.js:6:74)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)

Thanks!!

@dcodeIO
Copy link
Member

dcodeIO commented Jun 10, 2017

There have been a couple of changes along the road, but without additional context from your generated file it's hard to tell. roots and Root should be there. What's the require statement prior to your snippet? Looks like it doesn't require protobuf.js.

@hnfmr
Copy link
Author

hnfmr commented Jun 10, 2017

Wow thanks for the prompt reply.

Here is the header part of the generated dest.js using protobuf.js 6.8:

/*eslint-disable block-scoped-var, no-redeclare, no-control-regex, no-prototype-builtins*/
"use strict";

var $protobuf = require("protobufjs/minimal");

var $root = ($protobuf.roots["default"] || ($protobuf.roots["default"] = new $protobuf.Root()))
.addJSON({
  pb: {
    nested: {

Here is the same header part generated using protobufjs 6.7.3:

/*eslint-disable block-scoped-var, no-redeclare, no-control-regex, no-prototype-builtins*/
"use strict";

var $protobuf = require("protobufjs");

var $root = ($protobuf.roots["default"] || ($protobuf.roots["default"] = new $protobuf.Root()))
.addJSON({
  pb: {
    nested: {

@dcodeIO
Copy link
Member

dcodeIO commented Jun 10, 2017

Ah, for some reason this is now including the minimal library, which doesn't include reflection at all (so no Root).

Could be related to #813

Ideally, it should be require("protobufjs/light"); at this place as the parser isn't required when using JSON modules.

@hnfmr
Copy link
Author

hnfmr commented Jun 10, 2017

Yep, a comparison made it clear. The protobufjs/minimal is probably the cause. Thanks a lot!

@hnfmr
Copy link
Author

hnfmr commented Jun 10, 2017

Also that I have the following errors if I change the require("protobufjs/minimal") to
require("protobufjs/light") or just require("protobufjs"):

Suppose I have a type from reflection lookup:

const CustomT = root.lookup('pb.SomeCustomType')

It complains CustomT.from is not a function, and if I change from to create, it works, but it then complains that toObject is not a function:

const a = CustomT.from(other);
const decodedMessage = CustomT.decode(buffer);
decodedMessage.toObject({defaults: true});

@dcodeIO
Copy link
Member

dcodeIO commented Jun 10, 2017

from has been dropped in favor of fromObject and both this one and toObject are solely static methods now.

const a = CustomT.fromObject(other);
const decodedMessage = CustomT.decode(buffer);
CustomT.toObject(decodedMessage , {defaults: true});

@hnfmr
Copy link
Author

hnfmr commented Jun 10, 2017

Thanks for the quick update!

@gilly3
Copy link
Contributor

gilly3 commented Jun 12, 2017

from has been dropped in favor of fromObject and both this one and toObject are solely static methods now.

When you remove methods from your public API, that is a breaking change, and warrants a major version bump. The current version should be 7.0.0, not 6.8.0. It would be nice to give a release or two with a deprecation notice before removing methods.

@dcodeIO
Copy link
Member

dcodeIO commented Jun 12, 2017

See also, note at https://github.com/dcodeIO/protobuf.js#nodejs

Szpadel pushed a commit to Szpadel/protobuf.js that referenced this issue Jul 25, 2017
"minimal" doesn't include `Root`, as mentioned in protobufjs#828 and protobufjs#856. Probably caused by protobufjs#813.
ccdunder pushed a commit to ccdunder/protobuf.js that referenced this issue Nov 27, 2017
"minimal" doesn't include `Root`, as mentioned in protobufjs#828 and protobufjs#856. Probably caused by protobufjs#813.
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

3 participants