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

Generated typescript file is not usable anymore #834

Open
sherlock1982 opened this issue Jun 14, 2017 · 7 comments · Fixed by Szpadel/protobuf.js#2
Open

Generated typescript file is not usable anymore #834

sherlock1982 opened this issue Jun 14, 2017 · 7 comments · Fixed by Szpadel/protobuf.js#2

Comments

@sherlock1982
Copy link

sherlock1982 commented Jun 14, 2017

protobuf.js version: 6.8.0

Here's a part of my proto file:

message RequestUpdateDocument
{
        required ActionType Action = 1;
        optional WebMeetingDocument Document = 2;
        optional string Content = 3;
        optional int32 MeetingId = 4; 
}

Here's a part of generated typescript file using pbts:

        /** Properties of a RequestUpdateDocument. */
        interface IRequestUpdateDocument {

            /** RequestUpdateDocument Action */
            Action: MyPhone.Notifications.ActionType;

            /** RequestUpdateDocument Document */
            Document?: MyPhone.Notifications.IWebMeetingDocument;

            /** RequestUpdateDocument Content */
            Content?: string;

            /** RequestUpdateDocument MeetingId */
            MeetingId?: number;
        }

        /** Represents a RequestUpdateDocument. */
        class RequestUpdateDocument {

            /**
             * Constructs a new RequestUpdateDocument.
             * @param [properties] Properties to set
             */
            constructor(properties?: MyPhone.Notifications.IRequestUpdateDocument);

            /** RequestUpdateDocument Action. */
            public Action: MyPhone.Notifications.ActionType;

            /** RequestUpdateDocument Document. */
            public Document?: (MyPhone.Notifications.IWebMeetingDocument|null);

            /** RequestUpdateDocument Content. */
            public Content: string;

            /** RequestUpdateDocument MeetingId. */
            public MeetingId: number;
        }

You see that optional fields Document, Content, MeetingId have different types which makes this classes incompatible and this unusable. Class doesn't implement interface.

Expected behavior:

Interface and class should have same propeties. Probably it even makes sense to make class implement interface.

Also I don't understand why this:

properties?: MyPhone.Notifications.IRequestUpdateDocument

If we can make

properties?: Partial<MyPhone.Notifications.RequestUpdateDocument>

Alexendoo added a commit to Alexendoo/protobuf.js that referenced this issue Jun 24, 2017
Messages with optional fields will now implement their corresponding
interfaces (IMessage)

fixes protobufjs#834
fixes protobufjs#837
Szpadel pushed a commit to Szpadel/protobuf.js that referenced this issue Jul 26, 2017
Messages with optional fields will now implement their corresponding
interfaces (IMessage)

fixes protobufjs#834
fixes protobufjs#837
@wb14123
Copy link

wb14123 commented Oct 15, 2017

Hi, I'm facing this problem, too. Have you found any workaround for this?

@wb14123
Copy link

wb14123 commented Oct 15, 2017

@dcodeIO I think this is very important because currently there is no easy way to get the interface from the class.

@dcodeIO
Copy link
Member

dcodeIO commented Nov 24, 2017

Does this issue still exist in master? There have been a couple merges that might, or might not, prevent this. Otherwise, do you have an updated example causing the issue?

@sherlock1982
Copy link
Author

sherlock1982 commented Feb 14, 2018

6.8.3 still reproducable.
For example:

    interface IWebRTCCall {
        /** WebRTCCall otherPartyDisplayname */
        otherPartyDisplayname?: (string|null);
    }

    class WebRTCCall implements IWebRTCCall {

        /** WebRTCCall otherPartyDisplayname. */
        public otherPartyDisplayname: string;
    }

Actually I don't understand why these interfaces were added. They add additional noise and doesn't bring anything new to the app.

@dcodeIO
Copy link
Member

dcodeIO commented Feb 14, 2018

What about 6.8.4 (the version on npm)?

Btw. the interfaces are there because messages can be plain objects as well, without any methods but just data.

@mhoeger
Copy link

mhoeger commented Mar 11, 2019

@dcodeIO - are interfaces for data that's to be sent and classes for data that is being received? Is that an ok rule of thumb?

@ikokostya
Copy link

I have the same issue with protobufjs@6.8.8 from npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants