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

Support for FieldOptions.JSType #958

Closed
beyers-luno opened this issue Oct 26, 2023 · 6 comments · Fixed by #1030 · May be fixed by Graysonbarton/synthetics-sdk-nodejs#2 or VaniHaripriya/data-science-pipelines-tekton#1
Closed

Support for FieldOptions.JSType #958

beyers-luno opened this issue Oct 26, 2023 · 6 comments · Fixed by #1030 · May be fixed by Graysonbarton/synthetics-sdk-nodejs#2 or VaniHaripriya/data-science-pipelines-tekton#1
Labels

Comments

@beyers-luno
Copy link

Reference: https://protobuf.dev/reference/java/api-docs/com/google/protobuf/DescriptorProtos.FieldOptions.JSType

Hi!

In my proto file I have:

int64 foobar = 3 [jstype = JS_STRING];

In my generated ts file I get:

foobar: number;

Using the arguments:

--ts_proto_opt=onlyTypes=true

It should be this?

foobar: string;

Is there a configuration setting I am missing?

@beyers-luno
Copy link
Author

The closest configuration setting I can find is forceLong=string, which isn't exactly the same thing.

@stephenh
Copy link
Owner

Hi @beyers-luno , you're right, we don't currently support jstype--this is the first I'm seeing it.

Happy to have you work on a PR if you'd like to add support for it, although I'm a little worried it might be complicated, like I'm not sure the matrix of "proto type <-> different jstype". I assume that's documented somewhere, but just might be tedious to handle all of the cases.

Would be great if you want to take a look though; thanks!

@stephenh stephenh changed the title No support for jstype Support for FieldOptions.JSType Oct 26, 2023
@dasco144
Copy link
Contributor

I'd be willing to take a stab at this

@stephenh
Copy link
Owner

@dasco144 sounds good!

I think the toTypeName in types.ts is probably the best slice-point to say "this field should actually be type xyz".

Unfortunately that's just the type, and the encoding/decoding of that type is probably scattered around the encode/decode/toJSON/fromJSON methods. Like getEncodeWriteSnippet and getDecodeReadSnippet.

@dasco144
Copy link
Contributor

dasco144 commented Apr 16, 2024

@stephenh I've got an PR ready for review 🙏

stephenh pushed a commit that referenced this issue Apr 30, 2024
Closes #958

---------

Co-authored-by: Daniel Santiago <dsantiago@luno.com>
stephenh pushed a commit that referenced this issue Apr 30, 2024
# [1.173.0](v1.172.0...v1.173.0) (2024-04-30)

### Features

* Add js type support ([#1030](#1030)) ([0dd951b](0dd951b)), closes [#958](#958)
@stephenh
Copy link
Owner

🎉 This issue has been resolved in version 1.173.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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