Repository navigation
fix(v3/generator): Fix generation of mapped types with enum keys - #4943
Conversation
WalkthroughGenerator now treats numeric type aliases as map keys, emits mapped-type optional index signatures for JS/TS maps, adds an IsNumberAlias predicate and wiring, introduces enum-keyed map testcases and generated modules, and generalizes the runtime Map typing to accept non-string PropertyKey keys. Changes
Sequence Diagram(s)sequenceDiagram
participant Go as rgba(0,128,0,0.5) Go service
participant Gen as rgba(0,0,255,0.5) Generator (collect → render)
participant RT as rgba(255,165,0,0.5) Runtime typings
participant Out as rgba(128,0,128,0.5) Generated JS/TS
participant Consumer as rgba(192,192,192,0.5) Consumer
Go->>Gen: Provide types & service signatures (including map[T]U)
Gen->>Gen: Run predicates (IsStringAlias, IsNumberAlias) and collect deps
Gen->>Gen: Render map types as mapped-type signatures
Gen->>Out: Emit generated JS/TS modules (models + services)
Gen->>RT: Reference generalized Map<K,V> typing for helpers
RT->>Out: Provide runtime Map typing used by generated code
Consumer->>Out: Import generated modules and compile
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
fbbdev
changed the title
fbbdev
changed the title
a9438e6 to
df0cbdd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/internal/runtime/desktop/@wailsio/runtime/src/create.ts (1)
47-55:⚠️ Potential issue | 🟡 MinorAvoid shadowing the global
Mapconstructor.Biome's
lint/suspicious/noShadowRestrictedNamesrule will flag this. If the public API name must stay, use a scoped lint ignore; note that Biome requires//comment syntax, not/* */:🔧 Suggested lint suppression if renaming isn't viable
+// biome-ignore lint/suspicious/noShadowRestrictedNames: public API export function Map<K extends PropertyKey = any, V = any>(key: (source: any) => K, value: (source: any) => V): (source: any) => Record<K, V> {
🤖 Fix all issues with AI agents
In `@v3/internal/generator/collect/predicates.go`:
- Around line 460-462: Fix the typo in the comment above the numeric-type check:
change "numers" to "numbers" in the comment that precedes the code using
typ.Underlying(), basic and ok (the helper that returns ok &&
basic.Info()&(types.IsInteger|types.IsUnsigned|types.IsFloat) != 0). Ensure only
the comment text is updated and no code logic is changed.
In
`@v3/internal/generator/testdata/output/lang`=TS/UseInterfaces=true/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/enum_map_keys/enummapservice.ts:
- Around line 20-57: The declared functions (GetColorCodes, GetNestedEnumMap,
GetOptionalEnumMap, GetPersonsByStatus, GetPriorityWeights, GetStatusMessages)
return specific mapped types but call $Call.ByID which currently returns a
non-generic CancellablePromise, causing a TS incompatibility; fix by making
$Call.ByID generic so callers can specify the concrete return type (e.g. change
the signature of $Call.ByID to something like ByID<T>(id: number):
$CancellablePromise<T>) in the runtime/generator type definitions so each
function call $Call.ByID(…) is inferred as the exact mapped type.
🧹 Nitpick comments (3)
v3/internal/generator/testdata/output/lang=TS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/enum_map_keys/enummapservice.ts (1)
71-80: Private type creators use$Create.Anyfor both key and value.The type creators at lines 72-80 all use
$Create.Anyfor keys, which loses type information at runtime. While this may be intentional for flexibility, it contributes to the type mismatch issue since TypeScript cannot verify the created map matches the declared mapped type.Consider whether the generator should produce more specific type creators when the key type is known (e.g., enum types).
v3/internal/generator/collect/imports.go (1)
215-219: Correct extension to handle number aliases as map keys.The logic correctly extends import handling to include number aliases alongside string aliases. Both types can serve as valid TypeScript object keys.
Consider updating the comment on line 216-217 to reflect that this now covers both string and number aliases:
📝 Suggested comment update
if IsStringAlias(t.Key()) || IsNumberAlias(t.Key()) { - // This model type is always rendered as a string alias, + // This model type is always rendered as a string or number alias, // hence we can generate it and use it as a type for JS object keys. imports.addTypeImpl(t.Key(), visited) }v3/internal/generator/render/type.go (1)
152-168: Core fix for mapped type syntax is correct.The changes correctly address the PR objectives:
Line 152: Extending key eligibility to include
IsNumberAliasallows numeric alias types (likeIntAlias = int) to be used as map keys in generated TypeScript.Line 168: Switching from index signature
[_: Key]to mapped type[_ in Key]syntax fixes TypeScript error TS1337 for enum keys, as mapped types correctly handle finite key sets like enums.Minor suggestion: The comment on lines 153-154 mentions "string" but now also applies to number aliases.
📝 Suggested comment update
if collect.IsStringAlias(k) || collect.IsNumberAlias(k) { - // Alias or named type is a string and therefore + // Alias or named type is a string or number and therefore // safe to use as a JS object key.,
b9e0ce7 to
6cb047c
Compare
|
I had missed the error caught by test pipelines and highlighted by @coderabbitai because I had forgotten to build the runtime package locally. The issue was caused by the fact that TS's syntax for mapped types with enum keys Unfortunately this introduces a small breaking change: suppose you have a People who did not implement appropriate null checks (or assertions, as in I updated the changelog accordingly. |
|
Tip For best results, initiate chat on the files or code changes.
You're right that this is a breaking change in the type signatures, but it's definitely the right call. The previous type was actually lying to TypeScript users—it claimed that every key access would return a defined value, when Go maps can have missing keys. The new signature accurately reflects the runtime behavior and will help catch potential bugs at compile time rather than at runtime. The trade-off you're making is a classic "make illegal states unrepresentable" decision: users will get type errors now if they're not handling missing keys, but those type errors are highlighting real potential runtime issues. Anyone who hits these errors should either:
This is much better than having undefined behavior slip through unchecked. Great decision to go forward with the more accurate types! 👍 ^~^ ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
You're absolutely right that making the keys optional with The breaking change from The requirement to use either proper null checks ( Just to confirm: is this breaking change documented in the changelog entry? It might be worth explicitly mentioning that map value access now returns |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v3/internal/runtime/desktop/`@wailsio/runtime/src/create.ts:
- Line 52: The exported function Map(...) shadows the global Map and triggers
Biome's noShadowRestrictedNames rule; add a lint suppression immediately above
the function declaration (preserving the exported name and signature of Map<K
extends PropertyKey = any, V = any>(key: (source: any) => K, value: (source:
any) => V): (source: any) => Record<K, V>) using the repository's Biome
disable-next-line syntax for noShadowRestrictedNames so the public API name
remains unchanged.
🧹 Nitpick comments (1)
v3/internal/generator/testcases/enum_map_keys/main.go (1)
3-8: Remove unusedembedimport.The
_ "embed"blank import on line 4 is not used—there is no//go:embeddirective anywhere in this file. This appears to be leftover from a template or copy-paste.🧹 Proposed fix
import ( - _ "embed" "log" "github.com/wailsapp/wails/v3/pkg/application" )
@coderabbitai I couldn't find a concise enough way to explain that, so I mentioned null checks instead. Do you have alternative proposals? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3030
Option 1 (Most concise, technical): Option 2 (Balanced): Option 3 (Current + explicit type mention): My recommendation would be Option 1 if you want maximum conciseness with the explicit type change front and center, or Option 2 if you want to balance brevity with explanation. Both clearly state the type change while being shorter than the current version. What do you think? 🧠 Learnings used |
|
@leaanthony as usual for review you'll want to initially exclude the last two commits with test data noise, here's a quick link: |
|
|
Hi @fbbdev, this looks good and CI is all green. Could you rebase on |
cadf257 to
bbc8b72
Compare
…lsapp#4943) * Fix rendering of map types in binding generator * Add support for numeric map keys in binding generator * Add test case for enum map keys * Update UNRELEASED_CHANGELOG.md * Add testdata for enum map keys * Update binding generator testdata * Update UNRELEASED_CHANGELOG.md --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
2 participants
Description
A cleaner rewrite of #4452 that solves the same issue (invalid TS output for mapped types whose keys are enums).
This approach is similar to @leaanthony's original solution (and embeds test cases from that PR) but adopts a more general TS syntax for all cases instead of trying to detect enums and treating them as special cases.
As an added bonus, more types than before can be used as map keys now, i.e. not just string aliases but number aliases as well. This is just a DX/cosmetic improvement with no impact. Before:
After:
Fixes #4437
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Regenerated test data and inspected the diff.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRI have made corresponding changes to the documentationSummary by CodeRabbit
Breaking Changes
New Features
Bug Fixes
Documentation