Fix miscellaneous bindings and typescript export bugs #3978
Conversation
WalkthroughThis pull request introduces comprehensive improvements to the TypeScript binding generation process in the Wails framework. The changes focus on enhancing type handling, particularly for generic types, maps, and complex struct scenarios. Key modifications include refining the logic for processing struct fields, improving type name generation, and implementing more robust checks for JSON serialization. The updates aim to resolve several long-standing issues with TypeScript binding generation, including problems with generic types, map representations, and handling of non-serializable types. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
v2/internal/typescriptify/typescriptify.go (4)
289-291: Pointer unwrapping for map values.
Double-check if multi-level pointers require repeated unwrapping. Factor in any nil-safety checks if runtime issues could occur.
323-324: Direct assignment for non-struct map values.
Straight-to-the-point assignment is good. Just ensure key types remain strings or handle them explicitly if broader support is needed.
693-696: Diagnostic print statements.
These prints are useful for debugging but could cause noise in release builds. Consider gating them behind a debug flag.
958-958: Commented code snippet.
If this is no longer needed, removing it would reduce visual cruft.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v2/internal/binding/binding.go(3 hunks)v2/internal/binding/binding_test/binding_notags_test.go(1 hunks)v2/internal/binding/generate.go(1 hunks)v2/internal/typescriptify/typescriptify.go(13 hunks)website/src/pages/changelog.mdx(1 hunks)
🔇 Additional comments (20)
v2/internal/typescriptify/typescriptify.go (13)
43-45: Regex-based validation for unsafe variable characters.
Declaring jsVariableUnsafeChars is a neat way to standardize name sanitization.
47-55: Generic type sanitization function.
nameTypeOf properly handles bracketed generics by replacing disallowed characters, which is important for generating valid TypeScript identifiers. Consider adding a brief comment to explain the logic for future maintainers.
278-281: Pointer type extraction in map fields.
This logic is valid if you expect nil pointers or multiple pointer levels. Confirm your code handles unexpected pointer chains gracefully.
286-286: Appending “[]” to convey arrays.
This approach ensures TypeScript array notation is preserved and is consistent with the rest of the code.
320-321: Struct map value conversion.
Using convertValues(..., true) ensures each map entry is processed as a distinct struct. Neat solution.
527-527: Enum naming convention.
Appending prefix/suffix to the sanitized name is consistent. No major concerns here.
579-586: Skipping JSON for unsupported kinds.
Great approach to avoid encoding errors for channels, functions, complex numbers, etc.
633-633: Constructing entity name.
Similar logic to enums. The consistency helps keep naming uniform across the codebase.
836-837: Array field naming consistency.
Applying nameTypeOf to the element type is consistent with the rest of the data structure handling.
857-858: Simple field naming logic.
Mirrors the approach used elsewhere in typescriptify for uniform code generation.
882-882: Enum field naming with nameTypeOf.
Keeps naming patterns consistent and clarifies the generated TypeScript.
892-892: Qualified struct field assignment.
Using a fully qualified name helps avoid collisions and maintains type clarity in the generated TypeScript.
911-911: Array-of-structs field naming.
Again, consistent usage of nameTypeOf ensures uniform naming for nested structures.
v2/internal/binding/binding_test/binding_notags_test.go (1)
8-8: New function field in struct.
func() string fields can’t be JSON-encoded. Confirm if this addition is intentional and won’t break existing logic that relies on JSON encoding.
v2/internal/binding/generate.go (2)
226-228: Introducing a shared regex for unsafe characters.
Mirrors the logic in typescriptify.go, promoting consistency and reducing duplication.
231-237: Generic type sanitization for TypeScript.
Using SplitN and substituting unsafe characters ensures valid TypeScript type names for generics. Matches existing logic in typescriptify.go.
v2/internal/binding/binding.go (3)
280-280: SplitN usage to parse struct name.
A safer alternative to strings.Split, preventing accidental splitting if dotted package paths appear.
296-296: Repeat usage of SplitN for field references.
Maintains consistent parsing logic with the earlier line.
353-360: Ignoring non-json-encodable field types.
Aligns with the typescriptify changes to skip channels, functions, and complex fields. This ensures safe JSON operations.
website/src/pages/changelog.mdx (1)
31-31: Changelog entry for fixing non-json-encodable Go types.
Accurately highlights the fix introduced via PR #3975, aiding historical reference.
| @@ -277,7 +277,7 @@ func (b *Bindings) AddStructToGenerateTS(packageName string, structName string, | |||
| continue | |||
| } | |||
| fqname := field.Type.String() | |||
| sNameSplit := strings.Split(fqname, ".") | |||
| sNameSplit := strings.SplitN(fqname, ".", 2) | |||
There was a problem hiding this comment.
line 284 throws away the rest, we want to keep them
| @@ -261,15 +275,22 @@ func (t *TypeScriptify) AddType(typeOf reflect.Type) *TypeScriptify { | |||
| func (t *typeScriptClassBuilder) AddMapField(fieldName string, field reflect.StructField) { | |||
| keyType := field.Type.Key() | |||
| valueType := field.Type.Elem() | |||
| valueTypeName := valueType.Name() | |||
| valueTypeName := nameTypeOf(valueType) | |||
| if valueType.Kind() == reflect.Ptr { | |||
There was a problem hiding this comment.
matches Ptr unwrapping everywhere else in this file
without this, a map value that is a pointer to a struct looses its namespace #3709
| valueTypeName = valueType.Elem().Name() | ||
| valueTypeName = nameTypeOf(valueType.Elem()) | ||
| } | ||
| if valueType.Kind() == reflect.Map { |
There was a problem hiding this comment.
previously there was no reflect.Map here, so valueTypeName defaulted to "" and caused invalid typescript with e.g. nested map[string]map[string]string.
any here gives valid code at least, future work to recursively generate?
| @@ -659,8 +690,10 @@ func (t *TypeScriptify) convertType(depth int, typeOf reflect.Type, customCode m | |||
| } | |||
|
|
|||
| isKnownType := t.KnownStructs.Contains(getStructFQN(field.Type.String())) | |||
| println("KnownStructs:", t.KnownStructs.Join("\t")) | |||
| println(getStructFQN(field.Type.String())) | |||
| if !isKnownType { | |||
There was a problem hiding this comment.
This is very loud with a lot of structs. Quieting it unless it misses something
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
v2/internal/binding/generate.go (1)
231-238: Consider adding a comment explaining the generic type handling logic.The function now correctly handles generic types by sanitizing them for TypeScript compatibility. However, consider adding a comment to explain the transformation, e.g., how
Map[string]intbecomesMap_string_int.func goTypeToTypescriptType(input string, importNamespaces *slicer.StringSlicer) string { tname := goTypeToJSDocType(input, importNamespaces) tname = strings.ReplaceAll(tname, "*", "") + // Handle generic types by converting brackets and type parameters into a valid TypeScript identifier + // Example: Map[string]int -> Map_string_int gidx := strings.IndexRune(tname, '[') if gidx > 0 { // its a generic type rem := strings.SplitN(tname, "[", 2)v2/internal/typescriptify/typescriptify.go (1)
278-293: Document the limitation with nested maps.The changes fix pointer unwrapping (#3709) but nested maps default to
any. Consider adding a comment in the code to document this limitation.if valueType.Kind() == reflect.Map { - // TODO: support nested maps - valueTypeName = "any" // valueType.Elem().Name() + // LIMITATION: Nested maps are currently not supported and default to 'any'. + // This is a known limitation that will be addressed in a future update. + // See: <link-to-issue-tracking-nested-maps> + valueTypeName = "any" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/internal/binding/generate.go(1 hunks)v2/internal/binding/reflect.go(1 hunks)v2/internal/typescriptify/typescriptify.go(12 hunks)
🔇 Additional comments (6)
v2/internal/binding/generate.go (1)
226-228: LGTM! Well-defined regex for sanitizing JavaScript variable names.The regex pattern correctly identifies characters that are unsafe for JavaScript variable names.
v2/internal/typescriptify/typescriptify.go (4)
43-55: LGTM! Consistent type name handling across packages.The implementation matches the pattern used in
generate.go, ensuring consistent handling of type names across the codebase.
579-586: LGTM! Proper handling of non-JSON-encodable types.The function now correctly excludes types that cannot be JSON-encoded (channels, functions, complex numbers, and unsafe pointers).
892-892: LGTM! Consistent type name handling in struct fields.The change ensures that struct field types are handled consistently with other type name transformations.
693-696: LGTM! Improved logging verbosity.The change reduces log noise by only printing debug information when a struct is not found in
KnownStructs.v2/internal/binding/reflect.go (1)
169-171: Verify handling of nested type namesThe change from
strings.Splittostrings.SplitNaffects how nested type names are processed. For example,a.b.cnow returns("a", "b.c")instead of splitting into all components.Let's verify the impact on nested types in the codebase:
✅ Verification successful
Let me gather more context about the
getSplitReturnfunction implementation.
Let me check where this function is used to understand its purpose and impact.
Let me check one more thing to understand the impact on type generation.
Change to
strings.SplitNis safe and intentionalAfter analyzing the code, I can confirm that the change from
strings.Splittostrings.SplitNingetSplitReturnis safe and intentional. Here's why:
- The function
getSplitReturnis only used inentityFullReturnTypewhich handles type names in the binding generator- The function is specifically designed to split a fully qualified type name into namespace and type components, where:
- First part (index 0) = namespace
- Second part (index 1) = remaining type name
- Using
SplitNwith n=2 is the correct approach as we only need to separate the namespace from the rest of the type name, and any additional dots in the type name should be preserved (e.g., for nested types)The change actually improves the handling of nested type names by ensuring that only the first dot is used as a separator, which is the correct behavior for processing fully qualified type names in the TypeScript bindings generation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for nested type references that might be affected by the splitting change # Look for type references with multiple dots rg -g '*.go' '[a-zA-Z0-9]+\.[a-zA-Z0-9]+\.[a-zA-Z0-9]+' # Look for calls to getSplitReturn to understand usage patterns ast-grep --pattern 'getSplitReturn($expr)'Length of output: 72763
Script:
#!/bin/bash # Look for getSplitReturn implementation ast-grep --pattern 'func getSplitReturn($$$)' -l rg -A 10 "func getSplitReturn"Length of output: 753
Script:
#!/bin/bash # Find where getSplitReturn is called rg -B 5 -A 5 "getSplitReturn\(" # Look for any type references with dots that might be affected rg -g '*.go' 'reflect\.Type\.' -A 1Length of output: 1325
Script:
#!/bin/bash # Look for the usage context in generate.go rg -B 10 -A 10 "entityFullReturnType" # Also check for any test files that might verify this behavior fd ".*_test\.go" -x rg -l "getSplitReturn"Length of output: 4544
| result := strings.SplitN(in, ".", 2) | ||
| return result[0], result[1] | ||
| } |
There was a problem hiding this comment.
Add input validation to prevent panic
The current implementation will panic if the input string doesn't contain a "." character. Consider adding validation:
func getSplitReturn(in string) (string, string) {
result := strings.SplitN(in, ".", 2)
+ if len(result) != 2 {
+ return in, ""
+ }
return result[0], result[1]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result := strings.SplitN(in, ".", 2) | |
| return result[0], result[1] | |
| } | |
| func getSplitReturn(in string) (string, string) { | |
| result := strings.SplitN(in, ".", 2) | |
| if len(result) != 2 { | |
| return in, "" | |
| } | |
| return result[0], result[1] | |
| } |
caught these examples in http.Request.TLS: PeerCertificates []*x509.Certificate VerifiedChains [][]*x509.Certificate
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/internal/binding/binding.go (1)
Line range hint
693-696: Consider using structured logging.While the conditional logging reduces noise, consider using structured logging for better debugging:
- println("KnownStructs:", t.KnownStructs.Join("\t")) - println("Not found:", getStructFQN(field.Type.String())) + t.logger.Debug("Unknown struct type", + "known_structs", t.KnownStructs.Join("\t"), + "missing_type", getStructFQN(field.Type.String()))v2/internal/typescriptify/typescriptify.go (1)
278-293: Track the nested maps TODO as a separate issue.While the current implementation uses 'any' for nested maps, consider tracking this limitation:
Would you like me to create a GitHub issue to track the implementation of nested map support?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/internal/binding/binding.go(3 hunks)v2/internal/typescriptify/typescriptify.go(12 hunks)
🔇 Additional comments (9)
v2/internal/binding/binding.go (3)
Line range hint
271-288: LGTM! Improved struct field processing.The changes correctly handle:
- Skipping anonymous fields that shouldn't be exposed in TypeScript
- Proper handling of exported field checks
- More robust splitting of fully qualified names using
SplitN
289-305: LGTM! Enhanced pointer type handling.The changes properly handle pointer-to-struct fields by:
- Recursively dereferencing pointer types
- Applying consistent struct field processing logic
- Fixing pointer unwrapping issues (addresses Loss of models Scope When Using map[string]*models.XX convert to go/models.ts #3709)
352-359: LGTM! Added safety checks for JSON-incompatible types.The changes correctly exclude types that cannot be JSON-encoded:
- Channel types
- Function types
- Unsafe pointers
- Complex number types
This prevents potential runtime issues by ensuring only JSON-serializable types are processed.
v2/internal/typescriptify/typescriptify.go (6)
43-45: LGTM! Added regex for JavaScript identifier validation.The regex correctly identifies characters that are unsafe for JavaScript identifiers.
47-55: LGTM! Added robust type name handling.The
nameTypeOffunction properly:
- Handles generic type names
- Sanitizes unsafe characters
- Preserves type name readability
320-324: LGTM! Improved map field initialization.The changes correctly handle:
- Struct value types with proper conversion
- Non-struct value types with direct assignment
836-841: LGTM! Enhanced array field type handling.The changes properly handle:
- Type name sanitization
- Safe fallback to 'any' for unknown types
860-866: LGTM! Consistent simple field type handling.The changes maintain consistency with array field handling:
- Type name sanitization
- Safe fallback to 'any' for unknown types
899-899: LGTM! Consistent type name handling in struct fields.The change ensures struct field types use the same name sanitization as other field types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/internal/typescriptify/typescriptify.go (2)
278-294: Enhance nested map type support.While the current implementation correctly handles pointer unwrapping and basic map types, the handling of nested maps could be improved.
Consider implementing recursive type generation for nested maps instead of defaulting to
any. Example approach:if valueType.Kind() == reflect.Map { - // TODO: support nested maps - valueTypeName = "any" // valueType.Elem().Name() + keyType := valueType.Key() + elemType := valueType.Elem() + keyTypeName := t.getTypeScriptType(keyType) + elemTypeName := t.getTypeScriptType(elemType) + valueTypeName = fmt.Sprintf("{[key: %s]: %s}", keyTypeName, elemTypeName) }
694-697: Improve logging consistency.While reducing log verbosity is good, the implementation could be improved:
- Use structured logging instead of
println- Maintain consistency with
t.logfused elsewhere in the codebaseConsider this approach:
if !isKnownType { - println("KnownStructs:", t.KnownStructs.Join("\t")) - println("Not found:", getStructFQN(field.Type.String())) + t.logf(depth, "Unknown struct type %s not found in known structs: %s", + getStructFQN(field.Type.String()), t.KnownStructs.Join(", ")) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/internal/typescriptify/typescriptify.go(12 hunks)
🔇 Additional comments (2)
v2/internal/typescriptify/typescriptify.go (2)
580-587: LGTM! Improved type safety for JSON serialization.The changes correctly prevent JSON serialization of incompatible Go types, aligning with
json.Marshalbehavior and preventing potential runtime errors.
43-55: LGTM! Robust handling of generic type names.The implementation correctly sanitizes type names for TypeScript compatibility, particularly for generic types.
Let's verify the handling of generic types:
✅ Verification successful
Let me gather more specific information about the type conversion process.
Generic type handling implementation is correctly integrated and used
The verification shows that the
nameTypeOffunction is well integrated into the TypeScript type generation system:
- It's used consistently throughout the codebase for handling type names in various contexts (field types, array elements, struct fields)
- The function is called in key type conversion scenarios:
- Map value types
- Array element types
- Struct field types
- Direct type conversions
The implementation correctly sanitizes generic type names by:
- Detecting generic types via
[character- Splitting the type name and parameters
- Replacing unsafe characters with underscores
- Maintaining type name compatibility across the Go-to-TypeScript boundary
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for generic type usage in the codebase # Test: Search for generic type declarations and their TypeScript bindings # Expect: Properly sanitized type names in the TypeScript output # Find Go files with generic type declarations rg -U "type\s+\w+\[" # Find TypeScript files that might contain the sanitized versions fd -e ts | xargs rg "_"Length of output: 6821
Script:
#!/bin/bash # Look for the TypeScript type generation code and its test cases rg -U "func.*Type.*\{" v2/internal/typescriptify/typescriptify.go -A 5 # Look for test files that verify generic type handling fd -e go -x rg -l "Test.*Generic" {} # Check for actual usage of the nameTypeOf function rg -U "nameTypeOf\(" v2/internal/typescriptify/Length of output: 7755
|
Thanks for taking the time to do this. That is indeed a lot of issues fixed 😅 Could you please add tests for the changes so we can be sure they don't break in the future? Many thanks 🙏 |
prior syntax is primarily useful for naming keys so not useful here, and this syntax avoids square brackets in output which greatly simplifies generation for Go generics
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
v2/internal/binding/binding_test/binding_nonstringmapkey_test.go (1)
21-21: Consider refining the type ofany.Using
Record<number, any>is valid, but it might obscure the map value’s structure or constraints. If you know the expected shape of the map values, refininganyto a more specific type (e.g.,unknown, or a domain model interface) can improve type safety and readability.v2/internal/typescriptify/typescriptify.go (2)
278-294: Consider implementing recursive map type generation.The current implementation defaults nested maps to 'any', which is a safe fallback but could be improved. The TODO comment suggests future work to handle nested maps recursively.
Consider implementing recursive type generation for nested maps. Example approach:
- // TODO: support nested maps - valueTypeName = "any" // valueType.Elem().Name() + keyType := valueType.Key() + elemType := valueType.Elem() + keyTypeName := t.getTypeScriptType(keyType) + elemTypeName := t.getTypeScriptType(elemType) + valueTypeName = fmt.Sprintf("Record<%s, %s>", keyTypeName, elemTypeName)
694-697: Consider using structured logging for better debugging.The current debug logging is verbose and could be improved for better readability and filtering.
Consider using a more structured approach:
- println("KnownStructs:", t.KnownStructs.Join("\t")) - println("Not found:", getStructFQN(field.Type.String())) + log.Printf("Unknown struct type encountered: %s\nKnown structs: %s", + getStructFQN(field.Type.String()), + strings.Join(t.KnownStructs.AsSlice(), ", "))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/internal/binding/binding_test/binding_importedmap_test.go(1 hunks)v2/internal/binding/binding_test/binding_nonstringmapkey_test.go(1 hunks)v2/internal/binding/binding_test/binding_type_alias_test.go(1 hunks)v2/internal/binding/generate.go(2 hunks)v2/internal/binding/generate_test.go(1 hunks)v2/internal/typescriptify/typescriptify.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/internal/binding/generate.go
🔇 Additional comments (8)
v2/internal/binding/binding_test/binding_importedmap_test.go (1)
53-53: Consistency with modern TypeScript practices.Using
Record<string, binding_test_nestedimport.A>is a clearer and more idiomatic approach compared to a mapped type, improving readability and maintainability.v2/internal/binding/binding_test/binding_type_alias_test.go (2)
18-18: Consistent usage ofRecord<string, ...>Switching to
Recordsimplifies type declarations and is a more standard approach in modern TypeScript projects. This change is aligned with community best practices.
22-22: Appropriate match withint_package.SomeStructUsing
Record<string, int_package.SomeStruct>is a logical extension of the newRecordconvention applied to more complex types. The change is consistent and beneficial for readability.v2/internal/binding/generate_test.go (2)
119-119: Improved clarity usingRecord<string, number>Switching from mapped types to the
Recordutility type clarifies the intended structure. This also helps align with widely adopted TypeScript conventions.
124-124: NestedRecordusage for multi-level mapsAdopting a nested
Recordstructure for multi-level maps offers a cleaner, more explicit type definition. This approach reduces confusion when dealing with complex data hierarchies.v2/internal/typescriptify/typescriptify.go (3)
319-325: LGTM! Improved map type definition and value handling.The changes properly use TypeScript's
Recordtype and correctly handle struct values in maps usingconvertValues.
837-842: LGTM! Consistent application of type name handling.The changes consistently use the new
nameTypeOffunction across different contexts (simple fields, arrays, enums, structs), ensuring uniform handling of type names, especially for generic types.Also applies to: 861-867, 890-890, 900-900, 919-919
43-55: LGTM! Improved handling of generic type names.The new
nameTypeOffunction properly handles generic type names by converting them to valid JavaScript identifiers. This fixes issues with generic type bindings.Let's verify the generic type handling:
✅ Verification successful
Based on the evidence gathered, I can now provide the final response:
The
nameTypeOffunction is correctly implemented and used throughout the codebaseThe function is well integrated into the TypeScript generation pipeline and is used in multiple contexts:
- Converting struct field types
- Handling array and slice types
- Processing map types
- Managing enum types
- Dealing with imported types
While we didn't find explicit generic type test cases, the function's usage in the codebase shows it properly handles various type scenarios, including:
- Basic type names via
typeOf.Name()- Generic type notation splitting at
[- Safe conversion of potentially unsafe characters to valid JavaScript identifiers
- Proper integration with the type system's prefix/suffix handling
Note: The function is part of a larger type conversion system that successfully handles various Go types as evidenced by the test file
binding_type_alias_test.go, which verifies the generation of TypeScript definitions for maps, slices, and imported types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find examples of generic type usage in the codebase ast-grep --pattern 'type $name[$_] $_'Length of output: 40
Script:
#!/bin/bash # Let's try a different approach to find generic type usage and test files # First, let's look for test files that might contain generic type definitions fd ".*_test.go$" --exec grep -l "type.*\\[.*\\]" # Then, let's look for actual usage of the nameTypeOf function rg "nameTypeOf" -A 2 -B 2Length of output: 4655
Script:
#!/bin/bash # Let's examine the test file that contains generic type definitions cat ./v2/internal/binding/binding_test/binding_type_alias_test.go # Also check for any related test files that might show the conversion behavior rg -l "TestType.*Generic"Length of output: 2272
| // function, complex, and channel types cannot be json-encoded | ||
| if field.Type.Kind() == reflect.Chan || | ||
| field.Type.Kind() == reflect.Func || | ||
| field.Type.Kind() == reflect.UnsafePointer || | ||
| field.Type.Kind() == reflect.Complex128 || | ||
| field.Type.Kind() == reflect.Complex64 { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
LGTM! Added essential JSON encoding safety checks.
The changes prevent JSON serialization of types that cannot be properly encoded (channels, functions, complex numbers, etc.), improving type safety and preventing runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
v2/internal/typescriptify/typescriptify.go (2)
278-303: Consider implementing nested map support.While the changes properly handle pointer types and nested arrays in map values, the support for nested maps is currently limited. The code falls back to
anytype for nested maps, which might not be ideal for type safety.Consider implementing recursive map type generation to properly handle nested maps. For example:
- // TODO: support nested maps - valueTypeName = "any" // valueType.Elem().Name() + keyType := valueType.Key() + elemType := valueType.Elem() + keyTypeName := nameTypeOf(keyType) + elemTypeName := nameTypeOf(elemType) + valueTypeName = fmt.Sprintf("Record<%s, %s>", keyTypeName, elemTypeName)
703-706: Consider using structured logging for debugging.The current debug logging is verbose and might clutter the output. Consider using a proper logging framework with debug levels.
- println("KnownStructs:", t.KnownStructs.Join("\t")) - println("Not found:", getStructFQN(field.Type.String())) + if debug { + log.Printf("Unknown struct type: %s (known: %s)", + getStructFQN(field.Type.String()), + t.KnownStructs.Join(", ")) + }v2/internal/binding/binding_test/binding_deepelements_test.go (1)
20-30: Validate large map usage.
map[string][4][]DeepMessageand similar fields could become large at runtime. Ensure memory usage or potential constraints (like maximum array lengths) are well-documented or enforced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/internal/binding/binding_test/binding_deepelements_test.go(1 hunks)v2/internal/binding/binding_test/binding_generics_test.go(1 hunks)v2/internal/binding/binding_test/binding_ignored_test.go(1 hunks)v2/internal/binding/binding_test/binding_test.go(1 hunks)v2/internal/binding/generate_test.go(1 hunks)v2/internal/typescriptify/typescriptify.go(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/internal/binding/generate_test.go
🔇 Additional comments (16)
v2/internal/typescriptify/typescriptify.go (6)
43-55: LGTM! Well-implemented type name sanitization.The new
nameTypeOffunction properly handles generic type names by safely extracting and sanitizing the type parameters. This is a good solution for issues #3900 and #3709 where generic types weren't being properly converted.
328-334: LGTM! Proper TypeScript map type generation.The code correctly generates TypeScript map types using
Record<K,V>and properly handles field name escaping and optional fields.
589-596: LGTM! Added essential JSON encoding safety checks.The changes prevent JSON serialization of types that cannot be properly encoded (channels, functions, complex numbers, etc.), improving type safety and preventing runtime errors.
752-760: LGTM! Robust handling of nested arrays and pointer elements.The code properly handles both nested arrays/slices and pointer element types, maintaining correct type information throughout the conversion.
Line range hint
850-880: LGTM! Safe and consistent type handling.The code properly handles type names and provides a safe fallback to
anyfor unknown types, ensuring robust type generation.
Line range hint
903-913: LGTM! Proper handling of enum and struct field types.The code correctly handles type names and namespace differences, ensuring proper type references in the generated TypeScript code.
v2/internal/binding/binding_test/binding_ignored_test.go (4)
9-16: Consider serialization behavior for fields referencing non-JSON encodable types.Fields such as
unsafe.Pointer,complex64,complex128, andchan stringtypically do not serialize to standard JSON. If these fields are intended to be excluded or cause errors in the generated bindings, ensure the logic properly handles them.
11-11: Confirm usage of function pointer fields in JSON.The
func() intfield tagged withjson:"Total"may not serialize or deserialize as intended. Verify that ignoring or converting this field is consistent with desired behavior.
18-20: Method return type alignment check.Returning the struct by value here is fine, but confirm that it does not conflict with any references or mutating operations in test scenarios.
22-47: Good coverage for 'IgnoredTest'.The test thoroughly represents a struct with various complex fields. Ensure that negative or edge-case behaviors (e.g., serialization errors, ignored fields) are also validated in future expansions.
v2/internal/binding/binding_test/binding_test.go (1)
54-57: Expanded coverage with additional test cases.Adding
Generics1Test,Generics2Test,IgnoredTest, andDeepElementsTestimproves confidence in the binding logic. Consider validating edge cases and error paths for each new test in order to ensure robust coverage.v2/internal/binding/binding_test/binding_deepelements_test.go (2)
9-31: Complex nested structures may affect performance or memory.
DeepElementscontains extensive nesting (multi-dimensional arrays, maps, and slices). Confirm if tests demonstrate acceptable serialization performance for large or deeply nested data, to safeguard against potential overhead.
37-126: Positive coverage for 'DeepElementsTest'.This test effectively validates multi-layer serialization of nested types. Includes thorough TypeScript definitions to confirm type correctness. The approach seems comprehensive and should handle real-world scenarios effectively.
v2/internal/binding/binding_test/binding_generics_test.go (3)
7-16: Generic type usage looks good.
ListData[T]is well-defined. Confirm if constraints (e.g., implementingfmt.Stringer) would reduce potential issues with non-serializable fields in T.
18-48: ‘Generics1Test’ coverage is straightforward.Testing
ListData[string]ensures the fundamental structure works. Consider adding negative test cases or verifying how conversions behave under invalid inputs.
51-154: Comprehensive generics testing in ‘Generics2Test’.Covers more complex scenarios with custom structs. The approach for converting values in TS is clear. Consider adding edge case tests (e.g., null or nested slices with missing fields).
|
Bugs #3900, #3371, #2323 tested in I have some ideas for #2602 but it may not be worth the effort... |
|
Thanks 🙏 Please could you add an entry to the changelog located at |
|
Thanks for all your time on this 🙏 An amazing contribution to the community! |
Description
This is building upon the changes in v2.9.2 and #3678, and on top of PR #3975, to fix a lot of issues around generation in
models.ts. Mostly this is intended to clear out some of the old issues but also as a stop-gap so that I can start transitioning my own projects to v3 =)Some of these don't have clear reproduction examples, but I've tried to include examples of these issues in my local testing, I would appreciate some other eyes:
Fixes #3900
Fixes #3709
Fixes #3371
Sorta fixes #2602 (already fixed via 'any' / KnownStructs, though for time.Time specifically there are no exported fields)
Fixes #2551
Fixes #2303
Fixes #2323
Fixes #3755
Fixes #3442 (from crashing, not full type though)
Given the number of issues, I fully expect some updates to come for these changes! A few of them I'm not sure if they were holdovers from prior work or some edge case I don't know about or what. Happy to learn more and update!
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Example test suite in a gist: https://gist.github.com/pbnjay/73ef5a944dd12f6f2962f4fe14099912
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Release Notes
New Features
Fixed
models.tsbuild failure caused by non-JSON-encodable Go typeswebview2loaderusinggo-webview2version 0.1.17WindowSetSizeImprovements