Add support for special case type handling in the binding generation #4819
Conversation
WalkthroughReplaced filesystem-based binding tests with inline in-memory Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
v2/internal/binding/generate.go (1)
247-263: Generic type naming produces trailing underscores.The sanitization logic correctly creates valid JavaScript identifiers, but produces names with trailing underscores (e.g.,
ListData[foo.Bar]→ListData_foo_Bar_). While functional, this could be cleaned up for better readability.🔎 Suggested improvement
// Clean parameter: remove invalid symbols for JS identifiers safeParam := strings.NewReplacer( "[", "_", "]", "_", "*", "", "/", "_", ".", "_", ).Replace(param) + safeParam = strings.Trim(safeParam, "_") return base + "_" + safeParam + "_" + return base + "_" + safeParam } } }v2/internal/binding/binding_test/binding_type_alias_test.go (1)
7-48: LGTM! Good coverage of map and array types.The test validates the new
Record<K, V>andArray<T>TypeScript syntax for Go maps and slices, including package-qualified value types. The expected output correctly reflects all method signatures.Consider adding test coverage for map keys with package-qualified types (e.g.,
map[foo.Bar]string) to catch potential edge cases in the map parsing logic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v2/internal/binding/binding_test/binding_conflicting_package_name_test.go(2 hunks)v2/internal/binding/binding_test/binding_custom_type_test.go(1 hunks)v2/internal/binding/binding_test/binding_returned_promises_test.go(2 hunks)v2/internal/binding/binding_test/binding_type_alias_test.go(2 hunks)v2/internal/binding/generate.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
v2/internal/binding/binding_test/binding_custom_type_test.go (1)
v2/internal/binding/binding_test/binding_test.go (1)
BindingTest(13-21)
v2/internal/binding/binding_test/binding_type_alias_test.go (1)
v2/internal/binding/binding_test/binding_test.go (1)
BindingTest(13-21)
v2/internal/binding/binding_test/binding_returned_promises_test.go (1)
v2/internal/binding/binding_test/binding_test.go (1)
BindingTest(13-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
- GitHub Check: Run Go Tests (ubuntu-22.04, 1.23)
- GitHub Check: Run Go Tests (ubuntu-24.04, 1.23)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
v2/internal/binding/binding_test/binding_conflicting_package_name_test.go (1)
10-46: LGTM! Clean refactor to inline test fixtures.The test successfully migrates from filesystem-based verification to a declarative inline fixture, improving maintainability and execution speed. The expected TypeScript output correctly reflects the method signatures with properly namespaced imported types.
v2/internal/binding/binding_test/binding_custom_type_test.go (1)
9-41: LGTM! Test validates the PR's core objective.The test correctly demonstrates that
uuid.UUIDandtime.Timefields are mapped to TypeScriptstringtypes in generated models. The expected output aligns with thecustomTypeMapintroduced ingenerate.go. This provides good coverage for the special-case type handling feature.Note: This tests struct field mapping. Additional test coverage for methods that directly return
time.Timeoruuid.UUID(not as struct fields) would strengthen validation of the binding generation path.v2/internal/binding/binding_test/binding_returned_promises_test.go (1)
1-60: LGTM! Comprehensive promise handling test.The refactored test covers various return type scenarios including void, single values, errors, structs, pointers, slices, and combinations. The expected TypeScript bindings correctly represent each case with appropriate
Promise<T>types and array syntax.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
v2/internal/binding/generate.go (1)
294-296: Consider removing the unused parameter in a follow-up.The
_ *slicer.StringSlicerparameter is now unused. This maintains compatibility with existing call sites (lines 95, 111), but could be cleaned up if those callers are updated.v2/internal/binding/generate_test.go (1)
140-164: Good test coverage for custom type mappings.The new test cases comprehensively cover:
- Direct type mapping (
time.Time,uuid.UUID→string)- Slice composition (
[]time.Time→Array<string>)- Map composition (
map[string]uuid.UUID→Record<string, string>)- Pointer handling (
*time.Time→string)Consider adding a test case for generics containing custom types to verify the behavior:
🔎 Suggested additional test case
{ name: "pointer to time.Time", input: "*time.Time", want: "string", }, + { + name: "generic containing time.Time", + input: "main.ListData[time.Time]", + want: "main.ListData_time_Time_", + },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/internal/binding/generate.go(3 hunks)v2/internal/binding/generate_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (5)
v2/internal/binding/generate.go (4)
148-151: LGTM! Custom type mappings for time.Time and uuid.UUID.This correctly maps standard library types that serialize to strings in JSON. Previous review concerns about this map not being checked in
goTypeToJSDocTypehave been addressed at lines 263-267 and 270-273.
208-212: LGTM! Pointer handling now correctly strips leading*characters.This addresses the previous review concern where types like
*time.Timewould fall through to the defaultanycase.
226-252: LGTM! Map parsing now correctly handles nested brackets.The bracket-depth tracking logic properly handles cases like
map[[]int]stringandmap[map[int]string]bool. This addresses the previous review concern.
262-273: LGTM! customTypeMap is now properly checked for fully-qualified types.This addresses the previous critical review concern where
time.Timewould return the literal type name instead of"string". The dual lookup pattern (lines 263-267 for qualified names, 270-273 for base names) ensures both forms are handled.v2/internal/binding/generate_test.go (1)
166-172: LGTM! Test runner correctly updated for new function signature.The call site properly reflects the simplified
goTypeToJSDocType(input string)signature.
Add time.Time and uuid.UUID handling to start and demonstrate Ad# dress goTypeToJSDocType CodeRabbit issues
symball
force-pushed
the
time_bindings
branch
from
3cb1191 to
503f9c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/internal/binding/generate.go (1)
254-260: Nested generics produce unexpected results with greedy regex.As noted in a previous review, the pattern
^(.*)\[(.*)\]$uses greedy matching. For nested generics likemain.Outer[main.Inner[string]], this producesbase="main.Outer[main.Inner"instead ofbase="main.Outer". If nested generics are not a current use case, consider adding a clarifying comment.
🧹 Nitpick comments (3)
v2/internal/binding/generate_test.go (1)
140-164: Good test coverage for the new custom type mappings.The test cases comprehensively cover the new
time.Timeanduuid.UUIDmappings across different type constructs (direct, slice, map, pointer). Consider adding a test case for custom types within generics to ensure complete coverage:{ name: "generic with custom type", input: "main.ListData[time.Time]", want: "main.ListData_time_Time_", },v2/internal/binding/binding_test/binding_returned_promises_test.go (1)
26-27: UnusedGet()method adds noise.The
Get()method returns the receiver's value but isn't referenced in the expected bindings output. If it's truly optional and unused, consider removing it to keep the test fixture minimal.v2/internal/binding/generate.go (1)
299-301: Consider removing the unusedimportNamespacesparameter.The
_parameter indicatesimportNamespacesis no longer used. While keeping it maintains backward compatibility, if there are no external callers outside this package, consider removing it entirely to simplify the API.#!/bin/bash # Check if goTypeToTypescriptType is called from outside this package rg -n "goTypeToTypescriptType" --type go -g '!*_test.go' | grep -v "v2/internal/binding/generate.go"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/internal/binding/binding_test/binding_conflicting_package_name_test.go(2 hunks)v2/internal/binding/binding_test/binding_custom_type_test.go(1 hunks)v2/internal/binding/binding_test/binding_returned_promises_test.go(2 hunks)v2/internal/binding/binding_test/binding_type_alias_test.go(2 hunks)v2/internal/binding/generate.go(3 hunks)v2/internal/binding/generate_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/internal/binding/binding_test/binding_conflicting_package_name_test.go
- v2/internal/binding/binding_test/binding_custom_type_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
v2/internal/binding/binding_test/binding_returned_promises_test.go (1)
v2/internal/binding/binding_test/binding_test.go (1)
BindingTest(13-21)
v2/internal/binding/binding_test/binding_type_alias_test.go (1)
v2/internal/binding/binding_test/binding_test.go (1)
BindingTest(13-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run Go Tests (ubuntu-24.04, 1.23)
- GitHub Check: Run Go Tests (ubuntu-22.04, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
v2/internal/binding/generate_test.go (1)
166-172: LGTM!The test loop correctly uses the updated single-parameter
goTypeToJSDocTypesignature.v2/internal/binding/binding_test/binding_returned_promises_test.go (1)
30-59: LGTM!The test fixture correctly defines expected TypeScript bindings for various promise return scenarios. The expected output properly reflects the Go-to-TypeScript type mappings.
v2/internal/binding/binding_test/binding_type_alias_test.go (2)
34-48: LGTM!The expected TypeScript output correctly maps the various return types including the imported
int_package.SomeStructtype.
27-31: The code is correct.&MapAlias{}is intentional here—this test file explicitly verifies that type aliases, including map type aliases, are properly handled by the binding generator. The expected output confirms MapAlias is recognized as a named type. No changes needed.v2/internal/binding/generate.go (4)
148-151: LGTM!The
customTypeMapcorrectly mapstime.Timeanduuid.UUIDto"string"for TypeScript generation, addressing the PR objective.
153-182: LGTM!The
fullyQualifiedNamefunction correctly checkscustomTypeMapbefore falling back to primitive type handling.
275-278: Defensive lookup for future extensibility.This
customTypeMaplookup handles non-fully-qualified type names, which is currently not used but provides extensibility. The code is correct.
303-310: LGTM!The
entityFullReturnTypefunction correctly handles namespace extraction for import statements.
Add url.URL test
symball
force-pushed
the
time_bindings
branch
from
07bc375 to
67c3f3a
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
v2/internal/binding/generate.go (2)
154-183: Function correctly checks customTypeMap for fully-qualified names.The updated logic properly constructs the fully-qualified type name and checks
customTypeMapbefore falling through to basic type mappings. This ensures custom types liketime.Timeare resolved to"string".💡 Optional: Minor clarity improvement
Line 155 assigns
fullType := typeName, then line 157 reassigns it. Consider simplifying:func fullyQualifiedName(packageName string, typeName string) string { - fullType := typeName + var fullType string if len(packageName) > 0 { fullType = packageName + "." + typeName + } else { + fullType = typeName }
278-300: Comprehensive type mappings with safe fallback.The function includes a second
customTypeMapcheck for base type names (line 279) and comprehensive primitive mappings for all Go numeric types. The fallback to"any"on line 299 safely handles unknown types.The
customTypeMapis checked in three places:fullyQualifiedName(line 160),goTypeToJSDocTypefor fully-qualified names (line 268), and for base names (line 279). While this ensures complete coverage, it could be consolidated if maintainability becomes a concern in the future.v2/internal/binding/generate_test.go (1)
140-169: Good test coverage for custom type mappings.The six new test cases effectively validate the core functionality:
- Direct custom type mappings (time.Time, url.URL, uuid.UUID → string)
- Custom types in slices ([]time.Time → Array<string>)
- Custom types in maps (map[string]uuid.UUID → Record<string, string>)
- Pointer to custom type (*time.Time → string)
💡 Consider additional edge-case tests
The existing tests cover the common scenarios well. For more thorough coverage, consider adding:
{ name: "nested slice of time.Time", input: "[][]time.Time", want: "Array<Array<string>>", }, { name: "map with time.Time key", input: "map[time.Time]string", want: "Record<string, string>", }, { name: "nested map with custom types", input: "map[string]map[string]uuid.UUID", want: "Record<string, Record<string, string>>", },These would validate the recursive type resolution for more complex compositions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/internal/binding/generate.go(3 hunks)v2/internal/binding/generate_test.go(1 hunks)
🔇 Additional comments (6)
v2/internal/binding/generate.go (5)
148-152: LGTM! Custom type mappings correctly defined.The
customTypeMapproperly mapstime.Time,url.URL, anduuid.UUIDto"string"for TypeScript/JavaScript binding generation, aligning with the PR objective to serialize these types as frontend strings.
227-253: Map parsing with bracket-depth tracking correctly implemented.The bracket-depth logic properly handles nested brackets in map keys (e.g.,
map[[]int]stringormap[map[int]string]bool). The defensive check on line 247 ensures malformed inputs safely fall back to"any".
255-263: Generic type handling with documented limitation.The regex-based generic type flattening correctly transforms types like
main.ListData[string]tomain.ListData_string_. The comment on lines 256-257 appropriately documents that nested generics are not currently supported, which is acceptable for the current use case.
265-276: Fully-qualified type handling with customTypeMap check.The code correctly checks
customTypeMapfor fully-qualified types (line 268) before returning them unchanged. This ensures that method return types liketime.Timeare properly mapped to"string"instead of emitting non-existent TypeScript types.
302-304: Function simplified to delegate to centralized type resolution.The signature retains the
importNamespacesparameter (as_) for backward compatibility with calling code (lines 95, 111), while delegating all type conversion logic togoTypeToJSDocType. This centralization improves maintainability.v2/internal/binding/generate_test.go (1)
173-173: Function call correctly updated for new signature.The test properly uses the simplified single-parameter
goTypeToJSDocTypesignature, aligning with the centralized type resolution approach in the implementation.
1 participant
Add time.Time and uuid.UUID handling to start and demonstrate
Description
Add support for special case type handling in the binding generation. I have included this because in my time using Wails, quite a lot of issues people have tends to do with exporting time.Time.
This is a small simple change that allows for customHandling support within
fullyQualifiedNameI noticed there were some other tests failing so, have addressed them be rewriting in the seemingly new test style
Type of change
How Has This Been Tested?
Created a test project with the following struct that previously would have created some warnings
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.Now, on the frontend, a response like the following is returned without any intervention
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Tests
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.