-
Notifications
You must be signed in to change notification settings - Fork 349
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
fix: use correct imports for optional fields #904
Conversation
It seems that the ts-poet.Code.toString() changes the struct and it was causing the code gen to fail to use the correct imports in some situations. Added a struct which was failing to compile before the fix.
@@ -873,9 +873,6 @@ function generateInterfaceDeclaration( | |||
const name = maybeSnakeToCamel(fieldDesc.name, options); | |||
const isOptional = isOptionalProperty(fieldDesc, messageDesc.options, options); | |||
const type = toTypeName(ctx, messageDesc, fieldDesc, isOptional); | |||
if (isOptional && !type.toString().includes("undefined")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definitive fix probably lays within ts-poet, I believe the toString method shouldn't have any side-effects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I agree that is very strange that toString
has side-effects. Also kinda funny this console.warn
was just hanging out here 🤷 . Thanks for finding! I'll see if I can poke around at the ts-poet root cause at some point...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah great, thanks @andrewparmet !
@@ -873,9 +873,6 @@ function generateInterfaceDeclaration( | |||
const name = maybeSnakeToCamel(fieldDesc.name, options); | |||
const isOptional = isOptionalProperty(fieldDesc, messageDesc.options, options); | |||
const type = toTypeName(ctx, messageDesc, fieldDesc, isOptional); | |||
if (isOptional && !type.toString().includes("undefined")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I agree that is very strange that toString
has side-effects. Also kinda funny this console.warn
was just hanging out here 🤷 . Thanks for finding! I'll see if I can poke around at the ts-poet root cause at some point...
## [1.156.6](v1.156.5...v1.156.6) (2023-08-16) ### Bug Fixes * use correct imports for optional fields ([#904](#904)) ([fa13ec7](fa13ec7))
🎉 This PR is included in version 1.156.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
It seems that the ts-poet.Code.toString() changes the struct and it was causing the code gen to fail to use the correct imports in some situations.
Added a struct which was failing to compile before the fix.
This should fix #896