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

Simplify imports for enums. #576

Closed
r-tock opened this Issue Dec 19, 2016 · 30 comments

Comments

Projects
None yet
4 participants
@r-tock
Contributor

r-tock commented Dec 19, 2016

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

This comment has been minimized.

Owner

dcodeIO commented Dec 19, 2016

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.

dcodeIO added a commit that referenced this issue Dec 19, 2016

@dcodeIO dcodeIO added the enhancement label Dec 19, 2016

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 19, 2016

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

This comment has been minimized.

Owner

dcodeIO commented Dec 19, 2016

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

Does this help?

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 19, 2016

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

@paralin

This comment has been minimized.

paralin commented Dec 19, 2016

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

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 19, 2016

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

This comment has been minimized.

paralin commented Dec 19, 2016

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.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 19, 2016

@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 comment has been minimized.

paralin commented Dec 19, 2016

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.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 19, 2016

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

This comment has been minimized.

Owner

dcodeIO commented Dec 19, 2016

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.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 19, 2016

❤️ 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 added a commit that referenced this issue Dec 19, 2016

Added .create to statically generated types and uppercase nested elem…
…ents to reflection namespaces, see #576; Also added Namespace#getEnum for completeness, see #576; Made pbjs use loadSync for deterministic outputs, see #573
@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 19, 2016

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

This comment has been minimized.

paralin commented Dec 19, 2016

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 20, 2016

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)
@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 20, 2016

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: {"
@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 20, 2016

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

This comment has been minimized.

Owner

dcodeIO commented Dec 20, 2016

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.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 21, 2016

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

@Rranran

This comment has been minimized.

Rranran commented Dec 26, 2016

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

This comment has been minimized.

Owner

dcodeIO commented Dec 27, 2016

@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

This comment has been minimized.

Rranran commented Dec 28, 2016

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

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 28, 2016

@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.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 28, 2016

@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

This comment has been minimized.

Rranran commented Dec 29, 2016

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

@r-tock r-tock closed this Dec 29, 2016

@r-tock r-tock reopened this Dec 30, 2016

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 30, 2016

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

This comment has been minimized.

Owner

dcodeIO commented Dec 30, 2016

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

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 30, 2016

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

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 30, 2016

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?

@r-tock

This comment has been minimized.

Contributor

r-tock commented Dec 30, 2016

Ha found it! I was exporting this

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

instead of

root.resolveAll().lookup('tocktix.messages')            
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment