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

TS error if .proto file has the same message name as imported #896

Closed
skycrazyk opened this issue Aug 2, 2023 · 7 comments · Fixed by #904
Closed

TS error if .proto file has the same message name as imported #896

skycrazyk opened this issue Aug 2, 2023 · 7 comments · Fixed by #904
Labels

Comments

@skycrazyk
Copy link

skycrazyk commented Aug 2, 2023

Dependencies

Problem description

There is common/common.proto which has FindOptions message

syntax = "proto3";
package common;

message FindOptions {
  repeated string sort = 1;
  int32 page_num = 2;
  int32 page_size = 3;
}

and items/items.proto which has FindOptions too. Also items.proto imports common.proto and uses common.FindOptions message inside their own FindOptions message.

syntax = "proto3";
import "common/common.proto";

package content.items;

message FindOptions {
  common.FindOptions options = 2;
  bool regular = 4;
  bool hidden = 5;
  bool templates = 6;
}

message FindRequest {
  string space_id = 1;
  FindOptions options = 5;
}

Finally during the build process we have TS error

(spoiler) config/dist/items/items.ts(1511,27): error TS2345: Argument of type 'import("/builds/perxis/perxis- js/config/dist/items/items").FindOptions' is not assignable to parameter of type 'import("/builds/perxis/perxis- js/config/dist/common/common").FindOptions'.
config/dist/items/items.ts(1511,27): error TS2345: Argument of type 'import("/builds/perxis/perxis- 
js/config/dist/items/items").FindOptions' is not assignable to parameter of type 'import("/builds/perxis/perxis- 
js/config/dist/common/common").FindOptions'.
Type 'FindOptions' is missing the following properties from type 'FindOptions': sort, pageNum, pageSize, fields, excludeFields
config/dist/items/items.ts(1540,11): error TS2739: Type 'FindOptions' is missing the following properties from type 
'FindOptions': deleted, regular, hidden, templates
config/dist/items/items.ts(1584,5): error TS2322: Type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions | undefined' is not assignable to type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions | undefined'.
Type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions' is not assignable to type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions'.
config/dist/items/items.ts(1585,34): error TS2559: Type '{ options?: ... | undefined; deleted?: boolean | undefined; regular?: boolean | undefined; hidden?: boolean | undefined; templates?: boolean | undefined; }' has no properties in common with type '{ sort?: string[] | undefined; pageNum?: number | undefined; pageSize?: number | undefined; fields?: string[] | undefined; excludeFields?: boolean | undefined; }'.
config/dist/items/items.ts(1883,27): error TS2345: Argument of type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions' is not assignable to parameter of type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions'.
config/dist/items/items.ts(1912,11): error TS2322: Type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions' is not assignable to type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions'.
config/dist/items/items.ts(1956,5): error TS2322: Type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions | undefined' is not assignable to type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions | undefined'.
config/dist/items/items.ts(1957,34): error TS2559: Type '{ options?: ... | undefined; deleted?: boolean | undefined; regular?: boolean | undefined; hidden?: boolean | undefined; templates?: boolean | undefined; }' has no properties in common with type '{ sort?: string[] | undefined; pageNum?: number | undefined; pageSize?: number | undefined; fields?: string[] | undefined; excludeFields?: boolean | undefined; }'.
config/dist/items/items.ts([197](https://git.perx.ru/perxis/perxis-js/-/jobs/105621#L197)4,27): error TS2345: Argument of type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions' is not assignable to parameter of type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions'.
config/dist/items/items.ts([199](https://git.perx.ru/perxis/perxis-js/-/jobs/105621#L199)1,11): error TS2322: Type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions' is not assignable to type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions'.
config/dist/items/items.ts([200](https://git.perx.ru/perxis/perxis-js/-/jobs/105621#L200)7,5): error TS2322: Type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions | undefined' is not assignable to type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions | undefined'.
config/dist/items/items.ts(2008,34): error TS2559: Type '{ options?: ... | undefined; deleted?: boolean | undefined; regular?: boolean | undefined; hidden?: boolean | undefined; templates?: boolean | undefined; }' has no properties in common with type '{ sort?: string[] | undefined; pageNum?: number | undefined; pageSize?: number | undefined; fields?: string[] | undefined; excludeFields?: boolean | undefined; }'.
config/dist/items/items.ts([202](https://git.perx.ru/perxis/perxis-js/-/jobs/105621#L202)1,27): error TS2345: Argument of type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions' is not assignable to parameter of type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions'.
config/dist/items/items.ts([203](https://git.perx.ru/perxis/perxis-js/-/jobs/105621#L203)8,11): error TS2322: Type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions' is not assignable to type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions'.
config/dist/items/items.ts([205](https://git.perx.ru/perxis/perxis-js/-/jobs/105621#L205)4,5): error TS2322: Type 'import("/builds/perxis/perxis-js/config/dist/common/common").FindOptions | undefined' is not assignable to type 'import("/builds/perxis/perxis-js/config/dist/items/items").FindOptions | undefined'.
config/dist/items/items.ts(2055,34): error TS2559: Type '{ options?: ... | undefined; deleted?: boolean | undefined; regular?: boolean | undefined; hidden?: boolean | undefined; templates?: boolean | undefined; }' has no properties in common with type '{ sort?: string[] | undefined; pageNum?: number | undefined; pageSize?: number | undefined; fields?: string[] | undefined; excludeFields?: boolean | undefined; }'.

The problem that final items.d.ts file doesn't imports FindOptions from common at all

image

so inside items.d.ts FindOptions interface refers on themself

image

I have similar story with Filter message, which also exists in both files, but inside items.proto it is used with repeated, and in final items.d.ts it imported correctly, with rename to Filter1 as you can see on the image above.

@skycrazyk skycrazyk changed the title TS error if .proto has same name interface as imported TS error if .proto file has the same interface name as imported Aug 2, 2023
@stephenh
Copy link
Owner

stephenh commented Aug 3, 2023

Huh, yeah, we're supposed to have the infra in place to support this...the ts-poet codegen library knows about "symbols defined in this file" as well as "symbols imported from other files" and will rename the imports if it detects a conflict.

I.e. that is why Filter is getting renamed to Filter1 on import, is that ts-poet realized there is already a Filter name in-scope, so it synthesized a different/unique name for the import, and then in theory in code that would have using Filter-from-common will use Filter1 instead.

It seems like that did not work for FindOption, like it seems to not realize that "there are two FindOptions" in the first place.

The code that seems items.FindOption has a field of common.FindOption is here:

https://github.com/stephenh/ts-proto/blob/main/src/main.ts#L875

Which eventually gets here:

https://github.com/stephenh/ts-proto/blob/main/src/types.ts#L586

And should be getting .common.FindOptions as the protoType of that field, which should know it comes from another module, and so should generate an impProto that ends up creating an import statement in the output.

If you'd like to track down where that is going wrong, that would be great! Thanks!

@skycrazyk skycrazyk changed the title TS error if .proto file has the same interface name as imported TS error if .proto file has the same message name as imported Aug 3, 2023
@skycrazyk
Copy link
Author

I find out that everything worked fine until version 1.152.0.

In version 1.152.0 there is a bug, and I have error during build process

image

and then in version 1.152.1 I see problem described in this issue

image

@xmkhan
Copy link

xmkhan commented Aug 13, 2023

+1 Noticing the same problem. It now fails to compile because the toJSON and fromJSON fail to use the alias'ed import as the type.

I notice the problem even in v1.151.1.

@andrehp
Copy link
Contributor

andrehp commented Aug 15, 2023

I'm having the same issue, it's easy to add a case to fail the tests, you just need to add this message to integration/avoid-import-conflicts/simple.proto:

message DifferentSimple {
  string name = 1;
  optional simple2.Simple otherSimple = 2;
}

The generated interface is incorrect:

export interface DifferentSimple {
  name: string;
  otherSimple?: Simple | undefined;
}

otherSimple should be Simple3 | undefined instead of Simple | undefined

Edit: I did some testing and the commit that breaks the gen is indeed this one: 1405d4b . I will try to understand what changed there

@andrehp
Copy link
Contributor

andrehp commented Aug 16, 2023

I've opened the PR to fix this issue here. It seems that the issue was the check to emit a warning, which transforms the Code object.

I've also tested it with the example provided by skycrazyk and it generates the code correctly now.

@stephenh
Copy link
Owner

I just merged @andrehp 's fix, #904 , so report back if the latest release doesn't fix your issue. Thanks @andrehp !

@stephenh
Copy link
Owner

🎉 This issue has been resolved in version 1.156.6 🎉

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
Development

Successfully merging a pull request may close this issue.

4 participants