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

Array message type generates invalid code on account of name collision. #927

Closed
jinfiesto opened this issue Sep 29, 2023 · 8 comments · Fixed by #930 or #934
Closed

Array message type generates invalid code on account of name collision. #927

jinfiesto opened this issue Sep 29, 2023 · 8 comments · Fixed by #930 or #934
Labels

Comments

@jinfiesto
Copy link

I unfortunately am working with a schema where I have an Array message type. This causes issues with the generated code, as the Array message type that gets generated collides with the native Array type, see:

fromJSON(object: any): Whatever {
return { array: Array.isArray(object?.array) ? object.array.map((e: any) => Number(e)) : [] };
},

In this case, since I have an Array message type, that is unfortunately what is being referred to here in this code snippet. Did I miss some configuration in the docs which I can use to fix this?

@Hilzu
Copy link
Contributor

Hilzu commented Sep 30, 2023

I ran into the same issue when working with pg_query.proto from libpg_query but with Boolean and String messages. I have reproduced the issue here: https://github.com/Hilzu/ts-proto-with-pg-query#ts-proto-creates-invalid-code-for-pg_queryproto

@jcready
Copy link

jcready commented Sep 30, 2023

The simple fix here would be for ts-proto to prefix all of its global variable usage with globalThis. E.g.

globalThis.Array.isArray(object?.array) ? object.array.map((e: any) => globalThis.Number(e)) : []

@stephenh
Copy link
Owner

Hey, thanks for the report; this should be fixed in #930 ... disclaimer I did not technically add a schema with Date, String, Boolean, and Array message types in it ... probably should as a regression test, but 🤷 , skipped it this time around.

But lmk if I missed any.

Another disclaimer is that the globalThis that ts-proto currently uses is a kind-of-ugly-but-bullet-proof fallback; we could probably stand use to a globalThis=true flag for modern runtimes that don't need the fallback, which could remove the polyfill. I might do that while I'm here I guess.

@stephenh
Copy link
Owner

🎉 This issue has been resolved in version 1.158.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Hilzu
Copy link
Contributor

Hilzu commented Oct 1, 2023

@stephenh Adding the test with the message types might be a good idea as the fix in #930 doesn't cover all the cases. I'm still getting errors with the latest ts-proto v.1.159.1: https://github.com/Hilzu/ts-proto-with-pg-query#ts-proto-creates-invalid-code-for-pg_queryproto

pg_query.ts:13254:41 - error TS2349: This expression is not callable.
  Type '{ encode(message: String, writer?: Writer): Writer; decode(input: Uint8Array | Reader, length?: number | undefined): String; fromJSON(object: any): String; toJSON(message: String): unknown; }' has no call signatures.

13254     return { fval: isSet(object.fval) ? String(object.fval) : "" };
                                              ~~~~~~

...

Found 328 errors in the same file, starting at: pg_query.ts:13254

@stephenh stephenh reopened this Oct 1, 2023
@stephenh
Copy link
Owner

stephenh commented Oct 1, 2023

Ah, thanks for the report @Hilzu -- I've reopened the issue.

Fwiw if you could submit a PR that repros + fixes the issue, based on the general pattern of #930 , that'd be great/really appreciated. I think you could use integration/simple as our catch-all integration test to add some new messages in there, that'd be great. Or feel to add a new integration/... with a minimal repro of the message(s) as well.

Thanks!

@Hilzu
Copy link
Contributor

Hilzu commented Oct 1, 2023

Sure I can check it out. Should be straightforward enough.

Hilzu added a commit to Hilzu/ts-proto that referenced this issue Oct 2, 2023
Hilzu added a commit to Hilzu/ts-proto that referenced this issue Oct 2, 2023
Hilzu added a commit to Hilzu/ts-proto that referenced this issue Oct 2, 2023
Hilzu added a commit to Hilzu/ts-proto that referenced this issue Oct 2, 2023
stephenh pushed a commit that referenced this issue Oct 2, 2023
* fix: Support using messages called String/Boolean/Number/Array

Fixes #927

* chore: bin2ts
stephenh pushed a commit that referenced this issue Oct 2, 2023
## [1.159.2](v1.159.1...v1.159.2) (2023-10-02)

### Bug Fixes

* Support using messages called String/Boolean/Number/Array ([#934](#934)) ([f75159b](f75159b)), closes [#927](#927)
@stephenh
Copy link
Owner

stephenh commented Oct 2, 2023

🎉 This issue has been resolved in version 1.159.2 🎉

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
Labels
Projects
None yet
4 participants