Simplify imports for enums.

[robin-anil]

protobuf.js version: 6.2.1

Enums are a common thing used across our code. in v5. I would be able to do things like

import { MyMessage } from 'protos';
if (something === MyMessage.Type.CAT) {
   // Do something.
}

The same thing now is

import root from 'protos';
const MyMessage = root.lookup('MyMessage');
const Type = MyMessage.lookup('Type');

if (something === Type.values.CAT) {
}

I like the lookup pattern for messages, I understands its pros. But for enums its very clunky when you have values in between.

Can enums live in its own shadow hierarchy? which makes it easy to do something like.

import { MyMessage: { Type } } from 'enums'; 

if (something === Type.CAT) {
   // Do something.
}

[dcodeIO]

The reason why there aren't any magically created object trees anymore basically is that there is no way to make this work the same with typed dialects like TypeScript, which requires proper types on everything.

What do you think about adding

/**
 * Gets the values of the nested enum with the specified name.
 * This methods differs from {@link Namespace#get} in that it returns the enum's values directly and throws instead of returning `null`.
 * @param {string} name Nested enum name
 * @returns {Object.<string,number>} Enum values
 * @throws {Error} If there is no such enum
 */
NamespacePrototype.getEnum = function getEnum(name) {
    if (this.nested && this.nested[name] instanceof Enum)
        return this.nested[name].values;
    throw Error("no such enum");
};

so you can do the following (if Type is a direct child of MyMessage):

if (something === MyMessage.getEnum("Type").CAT) {
}

Side note: You can also use lookup and its variants on any namespace, including Type instances.

[robin-anil]

Thanks that's better.
However, still I don't like that dynamic interface is it doesn't throw error during import. Explicit import checking is something we have gotten used to and I would love if there was some way of keeping that...

[dcodeIO]

These methods throw: lookupType, lookupService, lookupEnum and getEnum
These don't: lookup, get

Does this help?

[robin-anil]

¯_(ツ)_/¯ Still a runtime check. I am thinking if there is something I can add so that the whole thing barfs during babel bundling.

[paralin]

Interesting idea to use static checking for this somehow, I like the idea but I'm not quite sure how we can implement that.

[robin-anil]

This idea is very raw: I feel like there is a middle ground between the current static-module (3.2MB in our case) and json-module (56KB)

I think you can create exports within json-module by not just export $root but the entire hierarchy as symbols. For all Types, Services and Enums.

I mean it is easy for me to do the following, Therefore I believe we can do something like that in the generated json-module itself whereby we can make these import statements work.

import * as protobuf from 'protobufjs';

const root = protobuf.Root.fromJSON(require('core/dist/core.json'));
  		  
const protos = {	
  ...root.lookup('tocktix.messages').nested
};
  		  
module.exports = protos;	

And for bonus, if we can somehow eliminate subsets of protos what is not in the dependency tree, the whole compiled bundle will be smaller. We have a huge proto tree for the server side.

But our clients depending on what APIs they call use different subsets of them. If we can somehow atomize and let babel/uglify be able to eliminate dead-code, that would be wonderful, But not required. I would take syntax and safety over this.

[paralin]

So why not just generate Typescript definitions for the static code, and actually generate + execute the static code at runtime?

By that I mean, generate a strongly typed TypeScript tree for the expected output of protobuf.Root.fromJSON. Then don't actually compile that to code. Just use fromJSON at runtime.

[robin-anil]

@paralin Could you mock a example for me. I would like to see what that looks like. I am trying to convert a large react codebase from v5-v6. I am also trying to automate this conversion, examples help me visuale the refactor.

[paralin]

This would have to be a few steps, I think.

Given a proto:

message MyMessage {
  string my_string = 1;
}

Step 1 is to generate typescript interfaces for these:

interface IMyMessage {
  myString?: string;
}

Then step 2 is to allow Type to take a Type<T>:

let myMessage: Type<IMyMessage>;

The T would be used for the return type of asJSON, as the argument type for create, validate, encode, etc.

Next, generate a tree:

{
  MyNamespace: {
     MyMessage: <Type<IMyMessage>>root.nested.MyNamespace.nested.MyMessage,
  },
}

Then usage would be:

generatedTree.MyNamespace.MyMessage.encode({myString: "test"})

The myString literal would be TypeScript validated, as the argument type would be of IMyMessage.

[robin-anil]

Yes! The generated tree portion. This is exactly what I had in my mind, which I started the conversation when I said shadow hierarchy. The trouble we have right now is nesting Types, Enums together, so we will have to create such trees independently for Types, Services and Enums. And we can do that in JS too... Doesn't have to be just typescript.

[dcodeIO]

Well, now that you mention it, it's theoretically possible to expose everything uppercase (there are no other uppercased properties) on the reflection objects so that a .d.ts from a static module could be used with reflection. encode, decode are the same anyway. Generated static classes would benefit from also having create in addition to make it mostly have the same runtime API as there is no constructor otherwise for runtime-generated types. This should, if I didn't miss something, make static and runtime classes mostly interchangeable - despite not having typed access to reflection (which isn't in the static .d.ts) of course.

[robin-anil]

❤️ I strongly support that theoretical possibility and would encourage to be made a reality. A clean API goes a long way; this is one of the last remaining kinks with v6.

[dcodeIO]

Give it a shot! Requires calling resolveAll() once on the root to populate the additional properties (json modules call this automatically now). Might still be missing something.

[paralin]

Damn, you're quick.

…

On Mon, Dec 19, 2016 at 2:53 PM, Daniel Wirtz ***@***.***> wrote: Give it a shot! Might still be missing something. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#576 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgpZ5YNWCBNNjAYaSaL1ki0DpVTE_6dks5rJwrkgaJpZM4LQbza> .

[robin-anil]

It looks like the generated json is missing that type entirely.
I am getting this error calling resolveAll().

field.js:251 Uncaught Error: unresolvable field type: BusinessOnboarding
    at Field.resolve (field.js:251)
    at Type.resolveAll (type.js:263)
    at Namespace.resolveAll (namespace.js:323)
    at Namespace.resolveAll (namespace.js:323)
    at Root.resolveAll (namespace.js:323)
    at Object.eval (protos.js:15)
    at Object.eval (protos.js:21)
    at eval (protos.js:22)
    at Object.<anonymous> (index.js:7048)
    at __webpack_require__ (index.js:556)

[robin-anil]

robinanil 21:14:10 :~/work/core$ cat src/main/model/proto/*.proto |grep "message BusinessOnboarding"
message BusinessOnboarding {

robinanil 21:14:34 :~/work/core$ ./node_modules/protobufjs/bin/pbjs src/main/model/proto/*.proto -s proto -t json -w commonjs -p src/main/model/proto/*.proto -p node_modules/protobufjs/ -o dist/core.json
robinanil 21:14:37 :~/work/core$ cat dist/core.json |grep "BusinessOnboarding: {"

[robin-anil]

This does not happen at v6.2.1 Just at head. -t static fails too

./node_modules/protobufjs/bin/pbjs src/main/model/proto/*.proto -s proto -t static -w commonjs -p src/main/model/proto/*.proto -p node_modules/protobufjs/ -o dist/core.json

[dcodeIO]

That's probably because v6.2.1 didn't call resolveAll(), hence missing definitions caused an error only if used directly or if a type using one needed to be resolved. Hence I'd assume that the definition wasn't in the JSON with v6.2.1, too.

[robin-anil]

Yes, Let me know if you need additional information for figuring out a fix

[Rranran]

The .proto file is

syntax = "proto3";
message person {
string name=1;
int32 id=2;
string email=3;
enum PhoneType {
MOBILE=0;
HOME=1;
WORK=2;
}
message PhoneNumber {
string number=1;
PhoneType type=2 [default=HOME];
}
repeated PhoneNumber phone=4;
}
how to init it? especially the enum type

[dcodeIO]

@r-tock: I don't know if this last issue actually requires a fix. If I understood you correctly, there are missing definitions, which should, to my understanding, cause an error.

[Rranran]

Your means if a enum type is included in the .proto file, we can't serialize it use protobufjs now ?

[dcodeIO]

@Rranran: Sorry, forgot to mention r-tock. My reply was directed to him.

Regarding your question: I am not quite sure what your issue is. You can just look up the enum and access its values:

var PhoneTypeValues = root.lookup("person.PhoneType").values

Or create a PhoneNumber from JSON and enum strings using Type#from:

var PhoneNumber = root.lookup("PhoneNumber");
var phoneNumber = PhoneNumber.from({
  number: "01234",
  type: "WORK"
});

Or you could rename "person" to "Person" to benefit from what we talked about above.

[robin-anil]

@dcodeIO the missing definition is caused by the pbjs compiler. All the Protos are in the valid path. See my grep command. The compiler is the one that is skipping those definitions

[Rranran]

@dcodeIO
According to your code, I solved my problem. Thanks a lot

[robin-anil]

Reopening this, there is one more case needs to be handled. which are Enums nested in a namespace not within a message, needs to be resolved into the namespace

[dcodeIO]

Erm, are those missing on top of the namespace after resolveAll()?

[robin-anil]

They are present in a different form. Where to access the values I need to do Type.values.CAT instead of Type.CAT

[dcodeIO]

Hmm, that's strange. This is what happens there for any subclass of namespace alike: https://github.com/dcodeIO/protobuf.js/blob/master/src/namespace.js#L306

Any idea?

[robin-anil]

Ha found it! I was exporting this

root.resolveAll().lookup('tocktix.messages').nested            

instead of

root.resolveAll().lookup('tocktix.messages')            

[MaheshSasidharan]

If anyone wants solution to what @Rranran