fixing typescript generation of map of struct arrays #4209
Conversation
WalkthroughThis pull request updates the type definitions for various map properties within the Changes
Assessment against linked issues
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 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 (1)
v2/internal/typescriptify/typescriptify.go (1)
273-336: Consider adding a comment explaining the change to Array syntax.While the implementation is correct, it would be helpful to add a comment explaining why the
Array<type>syntax is preferred over the bracket notation, particularly for maps with array values.func (t *typeScriptClassBuilder) AddMapField(fieldName string, field reflect.StructField) { + // Using Array<type> syntax instead of type[] notation for better compatibility with + // TypeScript when generating maps with values that are arrays of custom types keyType := field.Type.Key() valueType := field.Type.Elem()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/internal/binding/binding_test/binding_deepelements_test.go(2 hunks)v2/internal/typescriptify/typescriptify.go(4 hunks)
🔇 Additional comments (9)
v2/internal/typescriptify/typescriptify.go (5)
43-43: Code style improvement looks good.The simplified single-line variable declaration is cleaner and maintains the same functionality.
278-278: Preparation for array type syntax change looks good.Adding this variable to prepare for changing from bracket notation to generic notation.
291-293: Core fix for TypeScript array type notation.This change switches from using bracket notation (
[]) to generic notation (Array<type>) for representing arrays in TypeScript. This addresses the issue mentioned in PR #4144 where array syntax was causing problems with maps that contain arrays of custom types.The combination of prefix and suffix ensures proper nesting of multiple array dimensions.
328-328: Implementation of new array type format in the field declaration.Correctly applies the new
valueTypePrefixandvalueTypeSuffixin the TypeScript field declaration.
330-332: Updated constructor reference to match new type format.The constructor body now correctly references the type with the new Array generic syntax format, maintaining consistency with the field declaration.
v2/internal/binding/binding_test/binding_deepelements_test.go (4)
66-71: Fixed primitive type array representation in maps.Successfully updated the map type definitions from bracket notation to generic
Array<type>notation for numeric arrays, ensuring consistency with the changes in the TypeScript code generation.
75-77: Fixed custom type array representation in maps.Successfully updated the map type definitions for arrays of custom types (DeepMessage), which directly addresses the issue mentioned in PR #4144.
100-102: Updated constructor calls to match new type format.The constructor calls to
convertValuesnow correctly use the new Array generic syntax.
37-127:❓ Verification inconclusive
Verify test coverage for all array depth scenarios.
The changes look good and cover multiple array depth scenarios. Consider adding explicit tests for maps with deeply nested arrays and complex custom types to ensure the fix works for all edge cases.
🏁 Script executed:
#!/bin/bash # Check for any additional tests that might validate this fix for edge cases # Look for any other test files that use map with array values rg -l "map\[string\]\[\]" --type goLength of output: 178
Action: Review and expand test coverage for deeply nested maps and custom types.
The updated DeepElements conversion in the test file for binding (i.e.
binding_deepelements_test.go) effectively handles multiple array depth scenarios. Our search results indicate that map usages with array values also appear in other parts of the codebase (e.g. inpm.goandevents.go), but explicit tests for these complex cases aren’t clearly defined.
- Confirm that tests explicitly cover edge cases involving maps with deeply nested arrays.
- Consider adding dedicated tests to validate conversions for complex custom types in maps.
joshuapare
force-pushed
the
invalid-mapkey-array-arg-generation
branch
from
d4b8315 to
bc7b816
Compare
|
I tested it on the original issue from #4144, everything works! 🚀 |
|
Description
The current generation of TS typings using the
[]syntax in instance generation. However, this is the array accessor syntax on type instances of custom types, so the previous typings broke generations of maps with values being an array of a custom type. This PR fixes both the tests for this (which were invalid) and changes to the saferArray<type>syntax.Fixes #4144
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
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.Wails
Version | v2.10.1
System
┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
| OS | MacOS |
| Version | 14.7.5 |
| ID | ------ |
| Branding | |
| Go Version | go1.23.8 |
| Platform | darwin |
| Architecture | arm64 |
| CPU 1 | Apple M1 Pro |
| CPU 2 | Apple M1 Pro |
| GPU | Chipset Model: Apple M1 Pro Type: GPU Bus: Built-In Total Number of Cores: 16 Vendor: Apple (0x106b) Metal Support: Metal 3 |
| Memory | 16GB |
└────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Dependencies
┌────────────────────────────────────────────────────────────────┐
| Dependency | Package Name | Status | Version |
| Xcode command line tools | N/A | Installed | 2408 |
| Nodejs | N/A | Installed | 20.18.1 |
| npm | N/A | Installed | 10.8.2 |
| *Xcode | N/A | Available | |
| *upx | N/A | Available | |
| *nsis | N/A | Available | |
| |
└─────────────────── * - Optional Dependency ────────────────────┘
Diagnosis
Optional package(s) installation details:
SUCCESS Your system is ready for Wails development!
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Refactor
Bug Fixes