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

Nested message named "Function" now ends up as *_FunctionMessage in generated ts. #1121

Closed
jdillon opened this issue Oct 12, 2024 · 7 comments

Comments

@jdillon
Copy link

jdillon commented Oct 12, 2024

Caused by da048a1#diff-d34bf5280590ab473d1ad488821d664a0746c3459a2d1f3c2d7ea6037be2b042R63
from PR: #1110

This change causes nested messages to break from previous behavior.

message Scope {
   message Function {
   }
}

Used to render as interface Scope_Function but now will render as interface Scope_FunctionMessage ... which is erroneously appending Message to this type name.

@jdillon
Copy link
Author

jdillon commented Oct 12, 2024

Special handling for messages that conflict with special Javascript names (Function, Date) probably should only do so if they are not nested messages?

@stephenh
Copy link
Owner

Hi @jdillon , ah yeah, agreed we don't need the ...Message suffixing for nested messages.

If you could submit a PR that adds that check (where the builtIns array is checked and Message is conditionally appended), that would be great! Thanks!

@jdillon
Copy link
Author

jdillon commented Oct 13, 2024

@stephenh I can look, though at first glance I'm not sure how to tell if message: DescriptorProto is a nested message or not. so if you have any hints, then I can put a simple PR together for you.

@stephenh
Copy link
Owner

Np! I'm 80% sure that protoPrefix is only non-empty for nested messages:

  messages.forEach((message, index) => {
    // I.e. Foo_Bar.Zaz_Inner
    const protoFullName = protoPrefix + message.name;
    // I.e. FooBar_ZazInner
    const tsFullName = tsPrefix + maybeSnakeToCamel(messageName(message), options);

That call-site also has a isRootFile local variable that is probably true for top-level messages, and false for nested messages.

Those are the two things I'd try, and hopefully one of the two works.

halkeye added a commit to halkeye/ts-proto that referenced this issue Oct 15, 2024
"Function" is builtin, but anything prefixed or suffixed for whatever
reason shouldn't be handled the same way. So check if the final result
is in builtin list, not the original unmodified name
@halkeye
Copy link
Contributor

halkeye commented Oct 15, 2024

I have a PR up, i didn't see the new ticket created

#1123

halkeye added a commit to halkeye/ts-proto that referenced this issue Oct 15, 2024
"Function" is builtin, but anything prefixed or suffixed for whatever
reason shouldn't be handled the same way. So check if the final result
is in builtin list, not the original unmodified name
stephenh pushed a commit that referenced this issue Oct 15, 2024
"Function" is builtin, but anything prefixed or suffixed for whatever
reason shouldn't be handled the same way. So check if the final result
is in builtin list, not the original unmodified name

I have concerns about maybeSnakeToCamel being called earlier now, but i
can't find any tests for options.snakeToCamel.includes("keys") so i'm
not sure if i broke anything

```
  /** Converts `key` to TS/JS camel-case idiom, unless overridden not to. */
💡export function maybeSnakeToCamel(key: string, options: Pick<Options, "snakeToCamel">): string {
    if (options.snakeToCamel.includes("keys") && key.includes("_")) {
      return snakeToCamel(key);
    } else {
      return key;
    }
  }
```

But all tests pass
@stephenh
Copy link
Owner

Fixed in #1123 , thanks @halkeye !

stephenh pushed a commit that referenced this issue Oct 15, 2024
## [2.2.4](v2.2.3...v2.2.4) (2024-10-15)

### Bug Fixes

* Unbreak a use case for [#1110](#1110) / fix for [#1121](#1121) ([#1123](#1123)) ([476e99b](476e99b))
@jdillon
Copy link
Author

jdillon commented Oct 25, 2024

thank you!

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

No branches or pull requests

3 participants