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

Return message from setters for chaining #55

Merged
merged 2 commits into from
May 26, 2020

Conversation

rhinodavid
Copy link
Contributor

@rhinodavid rhinodavid commented Apr 18, 2020

I've noticed a discrepancy between the jspb implementation and the type definitions. All the field setters in jspb return the message so that you can chain setters, but the package generates setters with a return type void.

A test case:

// test.proto
syntax = "proto3";

package test;

message GetBalanceRequest {
    string address = 1;
}

Without changes:

// package: test
// file: test.proto

/* tslint:disable */
/* eslint-disable */

import * as jspb from "google-protobuf";

export class Test extends jspb.Message { 
    getAString(): string;
    setAString(value: string): void;

    clearRepeatedStringsList(): void;
    getRepeatedStringsList(): Array<string>;
    setRepeatedStringsList(value: Array<string>): void;
    addRepeatedStrings(value: string, index?: number): string;


    serializeBinary(): Uint8Array;
    toObject(includeInstance?: boolean): Test.AsObject;
    static toObject(includeInstance: boolean, msg: Test): Test.AsObject;
    static extensions: {[key: number]: jspb.ExtensionFieldInfo<jspb.Message>};
    static extensionsBinary: {[key: number]: jspb.ExtensionFieldBinaryInfo<jspb.Message>};
    static serializeBinaryToWriter(message: Test, writer: jspb.BinaryWriter): void;
    static deserializeBinary(bytes: Uint8Array): Test;
    static deserializeBinaryFromReader(message: Test, reader: jspb.BinaryReader): Test;
}


With changes:

// package: test
// file: test.proto

/* tslint:disable */
/* eslint-disable */

import * as jspb from "google-protobuf";

export class Test extends jspb.Message { 
    getAString(): string;
    setAString(value: string): Test;

    clearRepeatedStringsList(): void;
    getRepeatedStringsList(): Array<string>;
    setRepeatedStringsList(value: Array<string>): Test;
    addRepeatedStrings(value: string, index?: number): Test;


    serializeBinary(): Uint8Array;
    toObject(includeInstance?: boolean): Test.AsObject;
    static toObject(includeInstance: boolean, msg: Test): Test.AsObject;
    static extensions: {[key: number]: jspb.ExtensionFieldInfo<jspb.Message>};
    static extensionsBinary: {[key: number]: jspb.ExtensionFieldBinaryInfo<jspb.Message>};
    static serializeBinaryToWriter(message: Test, writer: jspb.BinaryWriter): void;
    static deserializeBinary(bytes: Uint8Array): Test;
    static deserializeBinaryFromReader(message: Test, reader: jspb.BinaryReader): Test;
}

export namespace Test {
    export type AsObject = {
        aString: string,
        repeatedStringsList: Array<string>,
    }
}

@rhinodavid rhinodavid marked this pull request as ready for review April 18, 2020 10:15
@rhinodavid rhinodavid closed this Apr 18, 2020
@rhinodavid rhinodavid reopened this Apr 18, 2020
@rhinodavid rhinodavid marked this pull request as draft April 18, 2020 16:49
@rhinodavid
Copy link
Contributor Author

Did some more digging on this. The protobuf package does indeed output setters which return the message and can be chained, but unfortunately the dependency version in grpc-tools hasn't been bumped since September '18. I opened a pr to bump it, so once this PR should be good to go.

@agreatfool
Copy link
Owner

That's cool. Thank you very much.

@rhinodavid rhinodavid marked this pull request as ready for review May 26, 2020 03:10
@rhinodavid
Copy link
Contributor Author

https://github.com/grpc/grpc-node/releases/tag/grpc-tools%401.9.0 has been released and this should be good to go

@agreatfool agreatfool merged commit ac671c2 into agreatfool:master May 26, 2020
@agreatfool
Copy link
Owner

New version 4.0.0 has been released. Thanks.

@RMHonor
Copy link
Contributor

RMHonor commented Jul 31, 2020

Chaining still doesn't work after setting a field, it looks like we need to upgrade to a later version of google-protobuf, as the version here doesn't return the message: https://github.com/protocolbuffers/protobuf/blob/97dd175a917fcdf1a0e0421659498a974f3a464a/js/message.js#L882

@agreatfool
Copy link
Owner

@RMHonor Yeah, you are right.

Codes in the example dir cannot work properly since the version of lib google-protobuf is too old. Since codes run with generated js are all belongs to this lib.

And there are no changes necessary for grpc_tools_node_protoc_ts, I would update the package.json file in example later.

截屏2020-07-31 下午11 45 32

截屏2020-07-31 下午11 45 48

截屏2020-07-31 下午11 46 11

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

Successfully merging this pull request may close these issues.

3 participants