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

field typings for `create` #717

Closed
masonk opened this Issue Mar 23, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@masonk

masonk commented Mar 23, 2017

protobuf.js version: 6.6

Thanks for a fantastic package. This is an enhancement request, something I would only make because I care!

I'm using protobufjs to generate TS definition files, but I've noticed that the various creation methods such as create, fromObject and new, aren't typed with the allowable fields. This is important to me, because those fields are exactly the ones that I want intellisense for.

Are there any plans to add this? If not, would you accept a patch for them? (Would you be willing to guide me if I commit to adding this patch?)

@dcodeIO dcodeIO added the enhancement label Mar 23, 2017

dcodeIO added a commit that referenced this issue Mar 23, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

Still rather basic: This now documents an interface named like the message with a trailing "$Properties" and makes the message implement the new interface. Additionally, it references the new interface as the type of the parameters to the constructor and .create.

However, parameter types of .fromObject and .verify are still plain objects because these methods are actually intended to be used with plain objects but their types could be changed to reference both (MyMessage$Properties|Object.<string,*>)= instead if that helps. Let me know!

@masonk

This comment has been minimized.

masonk commented Mar 23, 2017

Can't wait to try this out tonight! Will let you know.

@tamird

This comment has been minimized.

tamird commented Mar 24, 2017

a75625d didn't quite do the right thing - for instance, it doesn't do objects-all-the-way-down. I get this generated definition:

            type EventsResponse$Properties = {
                events?: cockroach.server.serverpb.EventsResponse.Event[];
            };

Where it should probably be:

            type EventsResponse$Properties = {
                events?: cockroach.server.serverpb.EventsResponse.Event$Properties[];
            };

Note the $Properties on the inner array.

dcodeIO added a commit that referenced this issue Mar 24, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 24, 2017

Does that solve it?

@tamird

This comment has been minimized.

tamird commented Mar 24, 2017

Yes, but there is now also #723.

@masonk

This comment has been minimized.

masonk commented Mar 25, 2017

I was able to drop in the new changes with "$Properties all the way down" without compilation issues. My app still compiles and runs fine, and the intellisense works beautifully now. It's a huge boon not to have to look up field names or types and to be incapable of misspelling them.

I'm not familiar enough with the pbjs runtime to identify any mismatches between the typings and the runtime reality (but if I discover any in due course I will be sure to raise them).

I'm blown away by how fast you made these changes too. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment