reimplement serialize as serializeBinary & serializeBinaryToWriter about protoc-gen-ts HOT 20 CLOSED

I'm kind of a freak who would like to tidy up things so that why I have done something like this. Probably you understand it already but let me clarify what I have with those methods.
I have managed to put all serializing and deserializing stuff into those two methods.
I had done this because having four methods doing the different part of the serializing and deserializing work seemed kinda confusing from the user perspective.
Besides theese two methods are only used internally by other message types.
But yeah when it comes to backward compatibility this change led to a breaking change which is not so important in my opinion cause literally you can just rename everything in your codebase and you are good to go.

from protoc-gen-ts.

Also, these changes have some interopability issues with @types/google-protobuf. When I realized this it was too late cause I was almost done writing the plugin.
May be we can send a PR to make those methods optional?

from protoc-gen-ts.

I'll vote for following the interface from google-protobuf.

My current problem is that the deserialize method is not defined on jspb.Message:

Previously, I could pass my web-socket handler a (msg_class: typeof jspb.Message)
and find the deserializer from that 'class' object. msg_class.deserializeBinary(data)
But jspb.Message does not have a ".deserialize"
(and the websocket parser does not want to hard code the message class/static)
First workaround using: msg_class['deserialize'] but even so eslint complains...

                // eslint-disable-next-line @typescript-eslint/no-unsafe-call
                let message = (this.messageParser.msg_class)['deserialize'](data) as jspb.Message

Alternate workaround: hack google-protobuf/index.dt.t to include the declaration for .deserialize

Hopefully not related:
When MyMessage.deserialize(data) is ultimately called, it fails due to: (bytes instanceof Uint8Array)
not recognizing my ArrayBuffer as being a Uint8Array; so:
const reader = (bytes instanceof Uint8Array) ? ... : bytes; // and bytes makes a terrible reader

Workaround: hand edit MyProto.ts/js to use (bytes instanceof ArrayBuffer)
Im using: "ws": "^7.4.4"

from protoc-gen-ts.

One option would be that we implement these methods and make the forward the call to serialize and deserialize to have a backward-compatible solution.

from protoc-gen-ts.

I'm all in favor of backward and standard compatibilty.

As for (bytes instanceof Uint8Array), i'll advocate for reversing the test:
let reader = (bytes instanceof pb_1.BinaryReader) ? bytes : new pb_1.BinaryReader(bytes), message = new AMessage();

That way there are fewer prototypes to confuse javascript...

from protoc-gen-ts.

that makes sense. this way we wouldn’t have to add an additional OR expression for everything.
I'll send a pr for this.

A work in progress will be available on alpha branch where I do a general refactor which will include this feature as well.

from protoc-gen-ts.

FWIW, I am making progress using these "scripts" on the 0.3.5 generated *Proto.ts files::

    "patch-serial": "replace 'serializeBinary(): Uint8Array { throw new Error(\"Method not implemented.\"); }' 'serializeBinary(): Uint8Array { return this.serialize(); }' -- *Proto.ts",
    "patch-return": "replace 'return writer.getResultBuffer();' 'return writer.getResultBuffer();\n        return undefined;' -- *Proto.ts",
    "patch-reader": "replace -s 'Uint8Array ? new pb_1.BinaryReader(bytes) : bytes' 'pb_1.BinaryReader ? bytes : new pb_1.BinaryReader(bytes)' --  *Proto.ts",

"patch-return" is just to avoid some noise from eslint.

from protoc-gen-ts.

Hey jack. In alpha branch i have implemented a new feature that puts 'serializeBinary' and 'deserializeBinary' methods which in turn delegates themselves to "serialize" and "deserialize" respectively. Would that work for you?

from protoc-gen-ts.

Yes, that would obviate the patch-serial script.
(I made the patch to update the API in anticipation of your new release)

from protoc-gen-ts.

@jackpunt a PR has landed for this. Will you be able to review and give insights on if there is room for improvements?

from protoc-gen-ts.

I'll give it a try; what is the appropriate incantation that I should use to get the updated version?

from protoc-gen-ts.

I'll give it a try; what is the appropriate incantation that I should use to get the updated version?

I'll release it today.

from protoc-gen-ts.

this has been released with version: v0.3.6-rc1 let me know if it works for you.

from protoc-gen-ts.

neither -rc1, nor -rc2 working for me...

:wspbclient>npm install [email protected] -g

added 2 packages, changed 1 package, and audited 4 packages in 1s

found 0 vulnerabilities
:wspbclient>npm ls -g google-protobuf
/Users/jp/.nvm/versions/node/v15.14.0/lib
├── [email protected]
└─┬ [email protected]
  └── [email protected] deduped
:wspbclient>protoc --version
libprotoc 3.15.8
:wspbclient>npm run protots

> [email protected] protots
> protoc -I src/proto --ts_out=src src/proto/*.proto

/Users/jp/.nvm/versions/node/v15.14.0/bin/protoc-gen-ts: line 1: syntax error near unexpected token `('
/Users/jp/.nvm/versions/node/v15.14.0/bin/protoc-gen-ts: line 1: `const plugin = require("./compiler/plugin");'
--ts_out: protoc-gen-ts: Plugin failed with status code 2.

from protoc-gen-ts.

Ah, that is something I can reproduce. Turns out to be a chmod issue of the binary file.

from protoc-gen-ts.

@jackpunt Could you try with 0.3.6-rc3?

from protoc-gen-ts.

It works! (installs, protoc runs, code looks good, tests pass)
Have not recoded to use 'deserializeBinary' but good to have it there for compatibility.

from protoc-gen-ts.

Hmm, I discover that a message with a 'repeated' field now comes as required rather than optional
(generated code missing the '?' in constructor signature)

from protoc-gen-ts.

@jackpunt that is not supposed to do that right? if not can you create another issue so that I can fix it as well?

0.3.6-rc4 has been released with a fix for this.

from protoc-gen-ts.

LGTM, thx!

from protoc-gen-ts.