[v3] Fix binding generator bugs #4001
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 177 files out of 268 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThis pull request introduces comprehensive changes across the Wails project's internal generator, focusing on code generation, template management, and service method handling. The modifications streamline the process of generating JavaScript and TypeScript bindings, improve type handling, and refactor how services and models are processed. Key updates include renaming service methods, simplifying import management, and enhancing the flexibility of code generation for different types of models and services. Changes
Sequence DiagramsequenceDiagram
participant Generator
participant Renderer
participant PackageIndex
participant ImportMap
participant Models
Generator->>PackageIndex: Collect package information
PackageIndex-->>Generator: Return package details
Generator->>Renderer: Generate Models
Renderer->>ImportMap: Prepare imports
Renderer->>Models: Render models
Renderer-->>Generator: Return generated models
Generator->>Renderer: Generate Typedefs
Renderer->>Models: Process type definitions
Renderer-->>Generator: Return type definitions
Possibly related PRs
Suggested Labels
Poem
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
Documentation and Community
|
fbbdev
force-pushed
the
v3/bindgen-fixes
branch
2 times, most recently
from
f9e930b to
bc41709
Compare
fbbdev
changed the title
fbbdev
force-pushed
the
v3/bindgen-fixes
branch
2 times, most recently
from
08a2a99 to
43ad161
Compare
fbbdev
force-pushed
the
v3/bindgen-fixes
branch
from
43ad161 to
075f389
Compare
This is a leftover from an older version of the static analyser. It should have been removed long ago.
gotypesalias=1 is the default starting with go1.23
![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 (15)
v3/internal/generator/testdata/package.json (1)
8-8: LGTM! Good addition for detecting circular dependencies.Adding
madgealigns well with the PR's objective to detect and prevent circular dependencies in the generated code. The version^8.0.0is appropriate for a dev dependency.Consider documenting the
madgedependency's purpose in the package.json description or in a comment, as it's not immediately obvious that this is used for circular dependency detection in the generator's output.v3/internal/generator/testdata/output/lang=TS/UseInterfaces=true/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.ts (3)
1-2: Confirm the inclusion of Welsh language commentsThe file header includes a comment in Welsh:
// Cynhyrchwyd y ffeil hon yn awtomatig. PEIDIWCH Â MODIWLPlease confirm whether the inclusion of Welsh is intentional and appropriate for the project's audience. If not, consider removing or translating it to maintain consistency.
17-17: Avoid using magic numbers and improve type safetyUsing hard-coded IDs like
1411160069reduces code readability and maintainability. Define a constant or enumeration for service IDs to make the code clearer. Additionally, avoid using type assertions likeas anyto maintain type safety.Suggested changes:
Define a constant for the service ID:
const GREET_SERVICE_ID = 1411160069;Update the function to use the constant and improve type safety:
-export function Greet(name: string): Promise<string> & { cancel(): void } { - let $resultPromise = $Call.ByID(1411160069, name) as any; +export function Greet(name: string): Promise<string> & { cancel(): void } { + let $resultPromise = $Call.ByID<string>(GREET_SERVICE_ID, name); return $resultPromise; }
9-10: Remove unnecessary ESLint and TypeScript suppression commentsThe following comments suppress linting and TypeScript checks:
// eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore: Unused importsIf the import statement is correctly used, these suppression comments may not be necessary. Review the import usage and remove these comments to adhere to best practices.
v3/internal/generator/testdata/output/lang=TS/UseInterfaces=true/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.ts (2)
11-11: Consider removing the @ts-ignore comment.The import is actually used in the code, so the
@ts-ignore: Unused importscomment is unnecessary and potentially misleading.-// @ts-ignore: Unused imports
16-19: Consider adding input validation.The function lacks input validation for the
nameparameter. While TypeScript provides type safety, runtime validation would improve robustness.export function Greet(name: string): Promise<string> & { cancel(): void } { + if (typeof name !== 'string' || name.trim() === '') { + return Promise.reject(new Error('Name must be a non-empty string')); + } let $resultPromise = $Call.ByName("main.GreetService.Greet", name) as any;v3/internal/generator/testdata/output/lang=JS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.js (1)
10-12: Consider removing unused import.The
$Createimport is not used in this file. While it's properly marked with@ts-ignore, it would be cleaner to remove it.-import {Call as $Call, Create as $Create} from "/wails/runtime.js"; +import {Call as $Call} from "/wails/runtime.js";v3/internal/generator/models.go (1)
9-24: Good refactoring of the generateModels function.The changes improve the code by:
- Simplifying the function signature to use a single
PackageIndexparameter- Standardizing error messages
- Streamlining file path handling
This refactoring aligns well with the PR's objective of improving the binding generator.
However, consider adding debug logging to help troubleshoot generation issues:
func (generator *Generator) generateModels(index *collect.PackageIndex) { + generator.logger.Debugf("Generating models for package: %s", index.Package.Path) file, err := generator.creator.Create(filepath.Join(index.Package.Path, generator.renderer.ModelsFile())) if err != nil { generator.logger.Errorf("%v", err)v3/internal/generator/testcases/cyclic_imports/main.go (1)
31-34: Consider enhancing test coverage.The
MakeCyclesmethod could be enhanced to actually return initialized structures to ensure the binding generator handles concrete instances correctly.func (GreetService) MakeCycles() (_ StructA, _ StructC) { - return + return StructA{B: &structB{A: nil}}, StructC{D: structD{E: StructE{}}} }v3/internal/generator/typedefs.go (1)
22-26: Enhance error handling specificity.The error message could be more specific about the file creation failure. Consider including the attempted file path in the error message.
if err != nil { - generator.logger.Errorf("%v", err) - generator.logger.Errorf("package %s: models generation failed", info.Path) + generator.logger.Errorf("Failed to create typedef file at %s: %v", + filepath.Join(info.Path, generator.renderer.InternalFile()), err) return }v3/internal/generator/testcases/function_single_internal/main.go (2)
38-40: Document empty HTTP handler implementation.The empty
ServeHTTPimplementation should be documented to explain why it's empty. If it's intentionally empty for testing, add a comment indicating this.-// Serve some routes +// ServeHTTP implements http.Handler interface. +// Empty implementation for testing purposes. func (*GreetService) ServeHTTP(http.ResponseWriter, *http.Request) { }
28-36: Consider documenting lifecycle method behavior.The lifecycle methods (
ServiceStartupandServiceShutdown) should have more descriptive documentation explaining their purpose in the test case.-// Lifecycle +// ServiceStartup implements the startup lifecycle hook. +// Returns nil as this is a test implementation. func (*GreetService) ServiceStartup(context.Context, application.ServiceOptions) error { return nil } -// Lifecycle +// ServiceShutdown implements the shutdown lifecycle hook. +// Returns nil as this is a test implementation. func (*GreetService) ServiceShutdown() error { return nil }v3/internal/generator/testcases/aliases/main.go (1)
16-18: Consider consolidating type aliases.You have two type aliases (
AliasedPersonandStrangelyAliasedPerson) that reference the samePersontype. This might lead to confusion in the generated bindings.Consider consolidating these into a single alias or documenting the specific use case that requires two separate aliases for the same type.
Also applies to: 52-54
v3/internal/generator/render/renderer.go (1)
90-94: Consider using a more descriptive variable name thanvisited.The
visitedslice is actually used as a sentinel value to mark processed aliases. A more descriptive name likeprocessedSentinelwould better convey its purpose.v3/internal/generator/collect/service.go (1)
165-173: Consider adding method descriptions in comments.While the map implementation is good, adding brief descriptions for each internal method would improve maintainability.
var internalServiceMethods = map[string]bool{ + // Returns the service's unique identifier "ServiceName": true, + // Called when the service is initialized "ServiceStartup": true, + // Called before the service is terminated "ServiceShutdown": true, + // HTTP handler implementation for the service "ServeHTTP": true, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
docs/src/content/docs/changelog.mdx(1 hunks)v3/internal/generator/Taskfile.yaml(2 hunks)v3/internal/generator/analyse.go(4 hunks)v3/internal/generator/analyse_test.go(1 hunks)v3/internal/generator/collect/imports.go(1 hunks)v3/internal/generator/collect/index.go(4 hunks)v3/internal/generator/collect/properties.go(1 hunks)v3/internal/generator/collect/service.go(2 hunks)v3/internal/generator/errors.go(3 hunks)v3/internal/generator/generate.go(3 hunks)v3/internal/generator/includes.go(2 hunks)v3/internal/generator/models.go(1 hunks)v3/internal/generator/render/create.go(2 hunks)v3/internal/generator/render/default.go(2 hunks)v3/internal/generator/render/renderer.go(5 hunks)v3/internal/generator/render/templates.go(1 hunks)v3/internal/generator/render/templates/index.tmpl(1 hunks)v3/internal/generator/render/templates/internal.js.tmpl(2 hunks)v3/internal/generator/render/templates/internal.ts.tmpl(1 hunks)v3/internal/generator/render/templates/models.tmpl(1 hunks)v3/internal/generator/render/templates/service.js.tmpl(1 hunks)v3/internal/generator/render/templates/service.ts.tmpl(1 hunks)v3/internal/generator/render/type.go(2 hunks)v3/internal/generator/testcases/aliases/main.go(2 hunks)v3/internal/generator/testcases/cyclic_imports/main.go(1 hunks)v3/internal/generator/testcases/cyclic_types/bound_types.json(1 hunks)v3/internal/generator/testcases/function_single_internal/bound_types.json(1 hunks)v3/internal/generator/testcases/function_single_internal/main.go(1 hunks)v3/internal/generator/testcases/no_bindings_here/person.go(1 hunks)v3/internal/generator/testcases/no_bindings_here/somemethods.go(1 hunks)v3/internal/generator/testdata/output/lang=JS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.js(1 hunks)v3/internal/generator/testdata/output/lang=JS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.js(1 hunks)v3/internal/generator/testdata/output/lang=JS/UseInterfaces=false/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.js(1 hunks)v3/internal/generator/testdata/output/lang=JS/UseInterfaces=false/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.js(1 hunks)v3/internal/generator/testdata/output/lang=JS/UseInterfaces=true/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.js(1 hunks)v3/internal/generator/testdata/output/lang=JS/UseInterfaces=true/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.js(1 hunks)v3/internal/generator/testdata/output/lang=JS/UseInterfaces=true/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.js(1 hunks)v3/internal/generator/testdata/output/lang=JS/UseInterfaces=true/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.js(1 hunks)v3/internal/generator/testdata/output/lang=TS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.ts(1 hunks)v3/internal/generator/testdata/output/lang=TS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.ts(1 hunks)v3/internal/generator/testdata/output/lang=TS/UseInterfaces=false/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.ts(1 hunks)v3/internal/generator/testdata/output/lang=TS/UseInterfaces=false/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.ts(1 hunks)v3/internal/generator/testdata/output/lang=TS/UseInterfaces=true/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.ts(1 hunks)v3/internal/generator/testdata/output/lang=TS/UseInterfaces=true/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.ts(1 hunks)v3/internal/generator/testdata/output/lang=TS/UseInterfaces=true/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.ts(1 hunks)v3/internal/generator/testdata/output/lang=TS/UseInterfaces=true/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.ts(1 hunks)v3/internal/generator/testdata/package.json(1 hunks)v3/internal/generator/typedefs.go(1 hunks)v3/pkg/application/application_options.go(0 hunks)
💤 Files with no reviewable changes (1)
- v3/pkg/application/application_options.go
✅ Files skipped from review due to trivial changes (12)
- v3/internal/generator/testcases/function_single_internal/bound_types.json
- v3/internal/generator/testcases/cyclic_types/bound_types.json
- v3/internal/generator/testdata/output/lang=TS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.ts
- v3/internal/generator/testdata/output/lang=TS/UseInterfaces=true/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.ts
- v3/internal/generator/testdata/output/lang=JS/UseInterfaces=true/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.js
- v3/internal/generator/testdata/output/lang=TS/UseInterfaces=true/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.ts
- v3/internal/generator/collect/properties.go
- v3/internal/generator/testdata/output/lang=JS/UseInterfaces=false/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.js
- v3/internal/generator/testdata/output/lang=TS/UseInterfaces=false/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.ts
- v3/internal/generator/testdata/output/lang=JS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.js
- v3/internal/generator/testdata/output/lang=JS/UseInterfaces=true/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/index.js
- v3/internal/generator/testdata/output/lang=TS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.ts
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/changelog.mdx
[grammar] ~42-~42: The correct preposition appears to be “on”.
Context: ...2) - Updated the minimum system version in macOS .plist files from 10.13.0 to 10.1...
(IN_WINDOWS)
[style] ~51-~51: This phrase might sound more natural if you rearrange it.
Context: ...r fields of array types of fixed length has now the expected length instead of being em...
(WORD_ORDER_HAVE_NOW)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Go Tests (ubuntu-latest, 1.22)
- GitHub Check: Run Go Tests (windows-latest, 1.22)
🔇 Additional comments (39)
v3/internal/generator/testdata/package.json (1)
7-7: Verify TypeScript version compatibility.The TypeScript version
^5.7.3appears to be from the future relative to my knowledge cutoff. Please verify that this version exists and is stable.Run this script to check the version:
✅ Verification successful
TypeScript version is valid and stable
The specified version
^5.7.3matches the current latest stable version in the npm registry.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify TypeScript version exists and get latest stable version curl -s https://registry.npmjs.org/typescript | jq -r '."dist-tags".latest'Length of output: 83
v3/internal/generator/render/templates/service.ts.tmpl (1)
35-35: LGTM! Import path change aligns with circular dependency fix.The change to import models from the internal file instead of the models file is part of the broader effort to prevent circular dependencies in the generated code.
Let's verify that this change effectively prevents circular dependencies in the generated code:
✅ Verification successful
Import path change successfully prevents circular dependencies
The change to import from internal.ts is verified to be effective. The test cases demonstrate proper handling of cyclic types while maintaining a clean public API through re-exports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for circular dependencies in the generated TypeScript code # Note: The PR mentions using madge for dependency checking # Find all generated .ts files echo "Searching for generated TypeScript files..." fd -e ts -t f # If madge is available, check for circular dependencies if command -v madge &> /dev/null; then echo "Checking for circular dependencies..." madge --circular --extensions ts . fiLength of output: 64864
Script:
#!/bin/bash # Check the cycles test case and import patterns echo "=== Examining cycles test case ===" cat v3/internal/generator/testdata/output/lang=TS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/cycles/{models,internal,index}.ts 2>/dev/null echo -e "\n=== Checking import patterns ===" # Look for imports between models.ts and internal.ts ast-grep --pattern 'import * from "./models"' ast-grep --pattern 'import * from "./internal"'Length of output: 1389
v3/internal/generator/testcases/no_bindings_here/somemethods.go (1)
11-11: Verify binding generator output for removed return type.The removal of
_ Impersonatorfrom the return types could affect generated bindings. Please ensure that:
- This change doesn't break existing frontend code
- The binding generator correctly handles the updated signature
Run this script to check the binding generator output:
v3/internal/generator/testcases/no_bindings_here/person.go (1)
8-9: Well-documented field with clear purpose.Good job on the clear and concise documentation for both fields. The comment "Exactly 4 sketchy friends" effectively communicates the fixed-length array requirement.
v3/internal/generator/render/templates/service.js.tmpl (2)
Line range hint
1-100: Verify type handling with the new import structure.While the import change looks good, we should ensure that all type definitions and service method generation continue to work correctly with models now being imported from the internal file.
Let's verify the type handling:
✅ Verification successful
Type handling verification completed successfully
The type system remains intact with the new import structure. The generated files maintain proper TypeScript checking with
@ts-check, correct type documentation, and consistent type handling for service methods.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that type definitions are correctly handled with the new import structure # Check for any type-related patterns in the generated code ast-grep --pattern 'export function $_($$_) { let $resultPromise = $Call.ByName($_, $$$); let $typingPromise = $resultPromise.then($$$); $$$ }' # Look for any type creation patterns ast-grep --pattern 'const $$createType$_ = function($_) { $$$ }'Length of output: 218
Script:
#!/bin/bash # Find generated service files and examine their content fd -e js -E '*.tmpl' | xargs grep -l "// This file is automatically generated" | head -n 2 | xargs cat # Look for type creation patterns in actual files rg "function.*\$Create" -A 5 # Check template usage fd service.js.tmpl | xargs grep -A 10 "@ts-check"Length of output: 959
36-36: LGTM! Import change aligns with circular dependency fix.The change to import models from the internal file instead of the models file aligns with the PR objective to resolve circular dependencies. This is achieved by centralizing all model definitions in the internal file and re-exporting public models.
Let's verify that this change effectively prevents circular dependencies in the generated code:
✅ Verification successful
✓ Import change verified: Effectively prevents circular dependencies
The change to import from internal.js instead of models.js is correct and effectively prevents circular dependencies by centralizing all model definitions in the internal file. The template system ensures this pattern is consistently applied across all generated service files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the generated code has no circular dependencies # Note: The PR mentions using madge for circular dependency detection # Find all generated .js files fd -e js -x echo "Checking {} for circular dependencies:" # Use ast-grep to find any potential circular imports in the generated files ast-grep --pattern 'import * from "./models.js"' ast-grep --pattern 'import * from "./internal.js"'Length of output: 66866
Script:
#!/bin/bash # Find and examine the content of models.js and internal.js files # Look for import statements between them # Find models.js and internal.js files echo "=== Finding models.js and internal.js files ===" fd -e js -E "testdata" -x echo "Found: {}" \; -x cat {} \; \( -name "models.js" -o -name "internal.js" \) # Look for import statements between these files echo -e "\n=== Checking import relationships ===" rg "import.*from.*[\"'].*(?:models|internal)[\"']" --type js -g "!testdata" # Look for re-export statements echo -e "\n=== Checking re-export patterns ===" rg "export.*from.*[\"'].*(?:models|internal)[\"']" --type js -g "!testdata"Length of output: 785
Script:
#!/bin/bash # Find and examine models.js and internal.js files in the template directory echo "=== Finding relevant files ===" fd "models\.js$|internal\.js$" v3/internal/generator/render/templates echo -e "\n=== Checking file contents ===" # Use simpler patterns for ripgrep echo "Looking in template directory:" rg "import|export" v3/internal/generator/render/templates --type js echo -e "\n=== Checking generated files ===" echo "Looking for import/export patterns in non-test files:" rg "import .* from ['\"]./models['\"]|import .* from ['\"]./internal['\"]" v3/internal --type js -g "!testdata"Length of output: 680
Script:
#!/bin/bash echo "=== Finding template files ===" fd "\.tmpl$" v3/internal/generator/render/templates echo -e "\n=== Examining service template content ===" cat v3/internal/generator/render/templates/service.js.tmpl echo -e "\n=== Looking for related templates ===" fd "models\.js\.tmpl|internal\.js\.tmpl" v3/internal/generator/render echo -e "\n=== Checking import patterns in templates ===" rg "import.*from" v3/internal/generator/render/templates --type jsLength of output: 4438
v3/internal/generator/generate.go (1)
155-160: Ensure correct iteration overservicesThe variable
servicesis of typeiter.Seq[*types.TypeName]. Therangekeyword in Go works with slices, arrays, maps, strings, and channels, but may not work as intended with custom sequence types likeiter.Seq. Please ensure thatservicesimplements theIteratorinterface or adjust the iteration accordingly.Consider updating the loop to use the appropriate iteration method:
-for obj := range services { +services.ForEach(func(obj *types.TypeName) {v3/internal/generator/testdata/output/lang=JS/UseInterfaces=true/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.js (1)
20-20: Verify the stability of the numeric method ID.The use of a numeric ID (1411160069) for method identification could be fragile if the method signature changes. Consider documenting the ID generation mechanism or using named methods for better maintainability.
Run this script to check the ID generation consistency:
✅ Verification successful
Method ID is part of the framework's binding system design
The numeric ID (1411160069) is consistently used across generated files and is documented as part of Wails' binding system. This is a stable, framework-level implementation detail rather than a maintenance concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for method ID usage consistency across generated files # Look for other occurrences of this ID to understand its generation rg -l "1411160069"Length of output: 14506
v3/internal/generator/testdata/output/lang=JS/UseInterfaces=true/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.js (1)
19-22: LGTM! Consistent with TypeScript version.The implementation correctly uses the named method approach and maintains consistency with the TypeScript version, including proper JSDoc type annotations.
v3/internal/generator/testdata/output/lang=JS/UseInterfaces=false/UseNames=false/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.js (1)
19-22: LGTM! Using numeric IDs helps prevent circular dependencies.The implementation using numeric IDs (
1411160069) instead of string names is a good approach to prevent circular dependencies, which aligns with the PR objectives.v3/internal/generator/testdata/output/lang=JS/UseInterfaces=false/UseNames=true/github.com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.js (1)
19-22: Verify if string-based service names could cause circular dependencies.Using string-based service names (
"main.GreetService.Greet") instead of numeric IDs might contribute to circular dependencies. Consider if this variant should also use numeric IDs for consistency with the PR's objective of preventing circular dependencies.v3/internal/generator/render/templates/index.tmpl (1)
31-33: LGTM! Improved model export condition.The change from
.Modelsto.HasExportedModelsis more precise and helps prevent unnecessary exports, which could contribute to circular dependencies. This aligns well with the PR's objective of fixing circular import issues.v3/internal/generator/testcases/cyclic_imports/main.go (2)
13-19: Well-structured test case for cyclic imports.The circular reference between
StructAandstructBeffectively tests the binding generator's handling of cyclic imports. The use of unexportedstructBadds complexity to the test case, ensuring the generator handles mixed visibility correctly.
21-29: Good test coverage for multi-level dependencies.The
StructC → structD → StructEchain provides additional test coverage for nested type dependencies, complementing the direct circular reference test case.v3/internal/generator/typedefs.go (1)
13-19: Effective import management.The import merging strategy effectively consolidates all required imports while maintaining clean separation between models. The explicit clearing of
ImportModelsensures clean typedef generation.v3/internal/generator/render/templates.go (2)
21-24: Clean separation of typedef templates.The introduction of separate typedef templates (
internal.js.tmplandinternal.ts.tmpl) effectively supports the resolution of circular import issues by allowing internal type definitions to be generated independently.
26-26: Simplified model template handling.The consolidation of model templates into a single template improves maintainability and reduces the potential for inconsistencies between JavaScript and TypeScript output.
v3/internal/generator/testcases/aliases/main.go (1)
65-68: LGTM! Good test coverage for alias handling.The
GetButAliasedmethod effectively tests the handling of aliased types in both parameter and return positions.v3/internal/generator/analyse_test.go (1)
100-108: LGTM! Improved service retrieval pattern.The change from callback-based to direct return of services simplifies the code and makes it more maintainable. This aligns well with the PR's objective of improving code quality.
v3/internal/generator/render/templates/models.tmpl (2)
51-66: LGTM! Robust type parameter handling.The type parameter building logic is well-structured and handles all cases correctly:
- Single parameter
- Multiple parameters
- No parameters
10-12: Verify the export logic for circular dependencies.The template's export logic separates objects and types, which should help prevent circular dependencies. However, we should verify this works as intended.
Also applies to: 20-33
✅ Verification successful
Export logic for circular dependencies is properly implemented
The template's separation of objects and types through distinct export blocks and different import mechanisms effectively prevents circular dependencies. The implementation correctly handles:
- Runtime objects (classes, enums) through direct exports
- Type information through separate type-only exports
- Clean separation between object references and type references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for circular dependencies in generated JS/TS files echo "Checking for circular dependencies in generated files..." fd -e js -e ts | xargs cat | rg -A 5 "^import.*from.*" | rg "circular|cycle"Length of output: 337
Script:
#!/bin/bash # View the full template content and surrounding context echo "=== Template Content ===" cat v3/internal/generator/render/templates/models.tmpl echo -e "\n=== Checking for actual circular dependency errors ===" # Look for actual circular dependency errors/warnings in the codebase rg -i "cannot|invalid|error.*circular" -A 3 echo -e "\n=== Examining import statements and their patterns ===" # Look for complex import patterns that might create cycles ast-grep --pattern 'import { $$$ } from "$_"'Length of output: 70364
v3/internal/generator/collect/index.go (2)
21-22: LGTM! Good addition of exported models tracking.The
HasExportedModelsflag is a clean way to track the presence of exported models, which helps optimize the template rendering process.
Line range hint
84-97: Verify the model sorting implementation.The model sorting logic looks correct, but we should verify it handles all edge cases properly.
v3/internal/generator/render/renderer.go (2)
138-149: LGTM: Models method refactored for better encapsulation.The refactoring of the Models method to take a PackageIndex parameter improves encapsulation by keeping all model-related data together.
84-124: Consider adding cycle detection in class alias sorting.The topological sorting of class aliases could potentially enter an infinite loop if there are circular dependencies between class aliases. Consider adding cycle detection to fail gracefully in such cases.
Let's verify if circular dependencies are possible in the codebase:
v3/internal/generator/render/default.go (2)
116-119: LGTM: Simplified prefix logic improves maintainability.The removal of the exported type check simplifies the logic while maintaining correct behavior through the ImportModels flag.
26-32: Verify performance impact of Array.from initialization.While the Array.from initialization ensures correct array length, it might be less performant than direct array initialization for large arrays. Consider adding a threshold for switching to direct initialization.
Let's check for large fixed-size arrays in the codebase:
v3/internal/generator/includes.go (1)
Line range hint
20-30: LGTM: Improved filename collision detection logic.The use of HasExportedModels and explicit length checks provides clearer semantics for filename collision detection.
v3/internal/generator/errors.go (1)
122-122: LGTM: Replaced external dependency with standard library functions.Good change to use standard library's maps.Keys and slices.Collect instead of the external lo package. This reduces external dependencies while maintaining the same functionality.
Also applies to: 132-132
v3/internal/generator/render/templates/internal.ts.tmpl (1)
12-12: LGTM! Import handling improvements.The removal of direct imports and the simplified import structure should help prevent circular dependencies, which aligns with the PR objectives.
v3/internal/generator/collect/imports.go (1)
149-149: LGTM! Import model handling is correctly updated.Setting ImportModels flag when the package path matches Self is the correct approach after removing the ImportInternal field.
v3/internal/generator/render/type.go (2)
57-57: LGTM! Improved documentation accuracy.The comment now correctly specifies that encoding/json marshals byte slices as base64 strings, which is more precise than the previous description.
214-216: LGTM! Simplified import model handling.The renderNamedType logic has been simplified to use only ImportModels flag, which is cleaner and aligns with the removal of ImportInternal.
v3/internal/generator/render/templates/internal.js.tmpl (2)
210-214: LGTM! Improved ES module compatibility.Adding a dummy export when in interface mode ensures the file is properly recognized as an ES module, which is important for modern JavaScript tooling.
Line range hint
1-1: Verify circular dependency detection.The changes look good and should prevent circular imports. Let's verify this using the madge tool mentioned in the PR objectives.
✅ Verification successful
Circular dependency prevention verified successfully.
The template structure implements proper import handling and dependency management through:
- Single-direction import flow
- Namespaced external imports
- Clear separation between runtime and model imports
- Type-safe module organization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for circular dependencies in generated TypeScript/JavaScript files # Find all generated .ts and .js files echo "Looking for generated TypeScript and JavaScript files..." fd -e ts -e js | while read -r file; do if grep -q "This file is automatically generated" "$file"; then echo "Checking $file for circular dependencies..." npx madge --circular "$file" || echo "No circular dependencies found in $file" fi doneLength of output: 131070
Script:
#!/bin/bash # Check the template file structure and import handling echo "=== Template Content ===" cat v3/internal/generator/render/templates/internal.js.tmpl echo -e "\n=== Related Template Files ===" # Find other related template files fd -e tmpl . v3/internal/generator/render/templates/Length of output: 8935
v3/internal/generator/collect/service.go (1)
122-123: Efficient internal method filtering!Good optimization using a map lookup instead of multiple string comparisons for filtering internal service methods.
v3/internal/generator/render/create.go (1)
257-261: Verify the impact of removing the exported status check.The simplified condition for importing models could potentially expose internal models. Please verify that this doesn't break encapsulation.
v3/internal/generator/Taskfile.yaml (1)
14-14: Great improvements to test reliability and dependency checking!The changes achieve two important goals:
-count=1ensures tests aren't cached, providing more reliable resultsmadge --circularhelps detect and prevent circular dependencies in the generated codeAlso applies to: 19-19, 23-23, 27-27, 33-33, 43-43
docs/src/content/docs/changelog.mdx (1)
45-46: Well-documented changes and their impact!The changelog clearly communicates:
- The fixes for circular dependencies and use-before-define errors
- The breaking change regarding array field initialization
Also applies to: 51-51
....com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.ts
Show resolved
Hide resolved
....com/wailsapp/wails/v3/internal/generator/testcases/function_single_internal/greetservice.ts
Show resolved
Hide resolved
fbbdev
force-pushed
the
v3/bindgen-fixes
branch
from
075f389 to
7415949
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
v3/internal/generator/models.go (1)
9-24: LGTM! Function refactoring improves maintainability.The simplified function signature and standardized error handling enhance code clarity. The consolidation of parameters into a single
indexparameter aligns well with the Single Responsibility Principle.Consider adding error wrapping to provide more context:
- generator.logger.Errorf("%v", err) + generator.logger.Errorf("failed to create models file: %v", err)v3/internal/generator/render/templates/models.tmpl (1)
7-8: Consider using English for all comments.The Welsh comment "Cynhyrchwyd y ffeil hon yn awtomatig" might confuse developers who don't speak Welsh.
-// Cynhyrchwyd y ffeil hon yn awtomatig. PEIDIWCH Â MODIWL +// This file is automatically generated. DO NOT MODIFYv3/internal/generator/collect/index.go (1)
69-82: Consider pre-allocating Models slice for better performance.When gathering models, pre-allocating the slice based on the known size would improve performance by avoiding reallocations.
+ modelCount := len(info.models.Range) + index.Models = make([]*ModelInfo, 0, modelCount)v3/internal/generator/render/renderer.go (1)
Line range hint
84-133: Consider extracting the sorting logic into a separate method.The implementation is solid and well-commented, but the complexity of the class alias sorting logic (lines 90-122) could be extracted into a separate method for better readability and maintainability.
Consider this structure:
func (renderer *Renderer) Typedefs(w io.Writer, imports *collect.ImportMap, models []*collect.ModelInfo) error { if !renderer.options.UseInterfaces { - // Sort class aliases after the class they alias... - aliases := make(map[types.Object][]*collect.ModelInfo, len(models)) - visited := make([]*collect.ModelInfo, 0) - models = slices.Clone(models) - for i, j := 0, 0; i < len(models); i++ { - // ... existing sorting logic ... - } + models = renderer.sortClassAliases(models, imports) } return renderer.typedefs.Execute(w, &struct { // ... rest of the code ... }) } func (renderer *Renderer) sortClassAliases(models []*collect.ModelInfo, imports *collect.ImportMap) []*collect.ModelInfo { aliases := make(map[types.Object][]*collect.ModelInfo, len(models)) visited := make([]*collect.ModelInfo, 0) result := slices.Clone(models) for i, j := 0, 0; i < len(result); i++ { // ... extracted sorting logic ... } return result }docs/src/content/docs/changelog.mdx (1)
45-46: Changelog entries accurately reflect the code changes.The changelog properly documents the key changes:
- Fixed dependency cycles in binding generator output
- Fixed use-before-define errors
- Changed behavior of fixed-length array fields initialization
However, consider rephrasing the array fields entry for better clarity:
-Default value for fields of array types of fixed length has now the expected length instead of being empty +Fields of fixed-length array types are now initialized with their expected length instead of being emptyAlso applies to: 51-51
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
docs/src/content/docs/changelog.mdx(1 hunks)v3/internal/generator/Taskfile.yaml(2 hunks)v3/internal/generator/analyse.go(4 hunks)v3/internal/generator/analyse_test.go(1 hunks)v3/internal/generator/collect/imports.go(1 hunks)v3/internal/generator/collect/index.go(4 hunks)v3/internal/generator/collect/properties.go(1 hunks)v3/internal/generator/errors.go(3 hunks)v3/internal/generator/generate.go(3 hunks)v3/internal/generator/includes.go(2 hunks)v3/internal/generator/models.go(1 hunks)v3/internal/generator/render/create.go(2 hunks)v3/internal/generator/render/default.go(2 hunks)v3/internal/generator/render/renderer.go(5 hunks)v3/internal/generator/render/templates.go(1 hunks)v3/internal/generator/render/templates/index.tmpl(1 hunks)v3/internal/generator/render/templates/internal.js.tmpl(2 hunks)v3/internal/generator/render/templates/internal.ts.tmpl(1 hunks)v3/internal/generator/render/templates/models.tmpl(1 hunks)v3/internal/generator/render/templates/service.js.tmpl(1 hunks)v3/internal/generator/render/templates/service.ts.tmpl(1 hunks)v3/internal/generator/render/type.go(2 hunks)v3/internal/generator/testcases/aliases/main.go(2 hunks)v3/internal/generator/testcases/cyclic_imports/main.go(1 hunks)v3/internal/generator/testcases/cyclic_types/bound_types.json(1 hunks)v3/internal/generator/testcases/function_single_internal/bound_types.json(1 hunks)v3/internal/generator/testcases/function_single_internal/main.go(1 hunks)v3/internal/generator/testcases/no_bindings_here/person.go(1 hunks)v3/internal/generator/testcases/no_bindings_here/somemethods.go(1 hunks)v3/internal/generator/testdata/package.json(1 hunks)v3/internal/generator/typedefs.go(1 hunks)v3/pkg/application/application_options.go(0 hunks)
💤 Files with no reviewable changes (1)
- v3/pkg/application/application_options.go
🚧 Files skipped from review as they are similar to previous changes (18)
- v3/internal/generator/testcases/function_single_internal/bound_types.json
- v3/internal/generator/testcases/cyclic_types/bound_types.json
- v3/internal/generator/collect/properties.go
- v3/internal/generator/render/create.go
- v3/internal/generator/testcases/no_bindings_here/person.go
- v3/internal/generator/testdata/package.json
- v3/internal/generator/testcases/no_bindings_here/somemethods.go
- v3/internal/generator/includes.go
- v3/internal/generator/errors.go
- v3/internal/generator/render/templates/service.js.tmpl
- v3/internal/generator/render/templates/index.tmpl
- v3/internal/generator/render/type.go
- v3/internal/generator/render/templates/service.ts.tmpl
- v3/internal/generator/typedefs.go
- v3/internal/generator/testcases/cyclic_imports/main.go
- v3/internal/generator/Taskfile.yaml
- v3/internal/generator/render/default.go
- v3/internal/generator/testcases/function_single_internal/main.go
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/changelog.mdx
[grammar] ~42-~42: The correct preposition appears to be “on”.
Context: ...2) - Updated the minimum system version in macOS .plist files from 10.13.0 to 10.1...
(IN_WINDOWS)
[style] ~51-~51: This phrase might sound more natural if you rearrange it.
Context: ...r fields of array types of fixed length has now the expected length instead of being em...
(WORD_ORDER_HAVE_NOW)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run Go Tests (windows-latest, 1.22)
🔇 Additional comments (18)
v3/internal/generator/testcases/aliases/main.go (2)
46-54: LGTM! Well-structured type definitions.The generic struct and type alias are properly defined with clear comments. The placement of
StrangelyAliasedPersonafter its target typePersonis good for avoiding potential "use-before-define" issues in generated code.
16-18: Consider movingAliasedPersonafter its target type definition.The alias is defined before its target type
Person. While this works in Go, it might cause "use-before-define" issues in the generated JavaScript/TypeScript code, which is one of the problems this PR aims to fix.Let's verify if this is a potential issue by checking the generated output:
v3/internal/generator/render/templates.go (1)
21-26: LGTM! Template organization improved.The separation of typedef templates and simplification of model templates enhances maintainability and follows the Single Responsibility Principle.
v3/internal/generator/render/templates/models.tmpl (1)
51-66: LGTM! Well-structured type parameter handling.The type parameter building logic is clear and handles edge cases properly.
v3/internal/generator/collect/index.go (2)
8-15: LGTM! Documentation improvements enhance clarity.The updated documentation clearly specifies the sorting order and handling of exported/unexported models.
Line range hint
84-97: LGTM! Efficient model sorting implementation.The sorting function correctly prioritizes exported models and maintains stable ordering by name.
v3/internal/generator/render/renderer.go (3)
23-23: LGTM: Clean addition of typedefs field.The new field appropriately supports the separation of type definitions from models.
40-41: LGTM: Proper initialization of templates.The initialization correctly handles language-specific templates and maintains consistency with other template initializations.
138-149: LGTM: Clean implementation of Models method.The updated signature and implementation properly handle the package index and maintain consistency with other template execution methods.
v3/internal/generator/generate.go (4)
143-150: Improved service discovery with better error handling.The refactored code simplifies the service discovery process by using a more functional approach with direct error handling, making the code more maintainable.
151-153: Good memory management practice.Explicitly setting
pkgs = nilhelps release memory after the data is no longer needed.
178-182: Consistent pattern for model generation and stats collection.The code follows a consistent pattern for both model generation scheduling and stats collection, which is good for maintainability.
Also applies to: 189-191
154-160: Verify service generation scheduling.The new scheduling approach looks good but we should verify that all services are properly processed.
✅ Verification successful
Service generation scheduling implementation is robust and correct.
The scheduling mechanism ensures proper service processing through:
- Synchronized concurrent execution using WaitGroup
- Error tracking and logging for each service
- One-time status reporting via sync.OnceFunc
- Guaranteed processing of all discovered services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any potential service processing issues # Test: Look for any error handling patterns around service generation # Search for service generation error handling patterns rg -A 5 'generateService.*error' # Look for any TODOs or FIXMEs related to service generation rg -i "TODO|FIXME" --glob "**/*service*.go"Length of output: 160
Script:
#!/bin/bash # Find generateService implementation and context ast-grep --pattern 'func $_ generateService($_) { $$$ }' # Check for error returns in service generation rg 'func.*generateService.*\).*error' # Look for scheduler implementation details rg -A 5 'type.*scheduler.*struct' # Check for panic recovery in generation context rg 'recover\(\)' --glob "**/*generate*.go"Length of output: 527
Script:
#!/bin/bash # Find generateService with more context rg -B 5 -A 10 "func.*generateService" # Look for error handling in generator rg "if err.*return|return.*error" --glob "**/*generate*.go" # Check for service generation result collection rg -A 5 "serviceFound|ServiceFound" --glob "**/*generate*.go"Length of output: 2245
v3/internal/generator/analyse.go (2)
23-23: LGTM! Function signature change improves API design.The return type change to
(iter.Seq[*types.TypeName], error)is a good improvement as it:
- Makes the function composable with other iterator operations
- Maintains proper error handling
- Follows Go 1.22's new iterator patterns
140-206: Verify error handling in the closure.The closure implementation looks solid, but we should verify that all error cases are properly propagated to the caller.
Run this script to check for potential error cases:
✅ Verification successful
Error handling in the closure is appropriate and verified.
The closure correctly handles edge cases by logging warnings and skipping unsupported types (anonymous, interface, generic) while continuing to process valid types. This non-fatal error handling pattern is consistent with the iterator's purpose of yielding only valid TypeName objects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for error handling patterns in the codebase # Look for similar functions that return (iter.Seq, error) and verify consistent error handling # Search for similar iterator returning functions ast-grep --pattern 'func $name($_) (iter.Seq[$_], error) { $$$ }' # Search for logger.Warning calls to see error handling patterns rg "logger\.Warning" -A 2Length of output: 5141
v3/internal/generator/render/templates/internal.ts.tmpl (1)
Line range hint
211-214: LGTM! Good practice for ES module recognition.The addition of a dummy export statement when in interface mode is a good practice as it:
- Ensures the file is properly recognized as an ES module
- Maintains TypeScript module semantics
- Prevents potential issues with module resolution
v3/internal/generator/collect/imports.go (1)
149-149: LGTM! Import handling simplification.The removal of the
ImportInternaldistinction and simplified import handling is a good improvement as it:
- Reduces complexity in the import system
- Maintains necessary functionality through
ImportModels- Aligns with the PR's goal of fixing circular imports
v3/internal/generator/render/templates/internal.js.tmpl (1)
210-214: LGTM! Consistent ES module handling across JS/TS.The addition of the dummy export statement matches the TypeScript template and:
- Maintains consistency between JS and TS code generation
- Ensures proper ES module recognition
- Follows best practices for module systems
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/src/content/docs/changelog.mdx (1)
42-42: Fix preposition in changelog entry.Change "in macOS" to "on macOS" for better grammatical accuracy.
-Updated the minimum system version in macOS .plist files from 10.13.0 to 10.15.0 +Updated the minimum system version on macOS .plist files from 10.13.0 to 10.15.0🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: The correct preposition appears to be “on”.
Context: ...2) - Updated the minimum system version in macOS .plist files from 10.13.0 to 10.1...(IN_WINDOWS)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/content/docs/changelog.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/changelog.mdx
[grammar] ~42-~42: The correct preposition appears to be “on”.
Context: ...2) - Updated the minimum system version in macOS .plist files from 10.13.0 to 10.1...
(IN_WINDOWS)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Go Tests (ubuntu-latest, 1.22)
- GitHub Check: Run Go Tests (macos-latest, 1.22)
- GitHub Check: Run Go Tests (windows-latest, 1.22)
🔇 Additional comments (3)
docs/src/content/docs/changelog.mdx (3)
29-31: LGTM! Clear documentation of breaking changes.The breaking changes are well documented with proper attribution and clear before/after states.
33-37: LGTM! Well-structured documentation of new features.The additions are clearly documented with proper attribution and references to PRs/commits.
49-52: LGTM! Clear documentation of changes to existing functionality.The changes are well documented with proper attribution and references to PRs/commits.
| obj := models[i].Type.(interface{ Obj() *types.TypeName }).Obj() | ||
| if obj.Pkg().Path() == imports.Self { | ||
| // models[i] aliases a type from the current module. | ||
| if a := aliases[obj]; a == nil || len(a) > 0 { |
There was a problem hiding this comment.
a == nil || len(a) > 0 is this right?
There was a problem hiding this comment.
Correct but stupid 🤦
I'm using the empty, non-nil array as a marker for visited models, but I should have tested for key presence in the map.
Thanks for spotting this, will push a fix in a minute
|
I'm good if you want to push the test fixes 👍 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/src/content/docs/changelog.mdx (2)
29-31: Consider adding migration guidance for breaking changes.The renaming of service methods (
Name->ServiceName, etc.) is a breaking change that will affect existing codebases. Consider adding:
- Code migration examples
- Deprecation notices for old method names
- Automated migration script references
Would you like me to help create a migration guide or script to assist users in updating their code?
46-47: Enhance the binding generator fix descriptions.The fixes for "dependency cycles" and "use-before-define errors" in the binding generator are significant improvements that deserve more detailed explanations:
- What types of dependency cycles were fixed?
- What scenarios triggered the use-before-define errors?
Consider adding technical details about the fixes to help users understand if their code was affected.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/src/content/docs/changelog.mdx(1 hunks)v3/pkg/application/application_options.go(0 hunks)
💤 Files with no reviewable changes (1)
- v3/pkg/application/application_options.go
🧰 Additional context used
🪛 LanguageTool
docs/src/content/docs/changelog.mdx
[grammar] ~43-~43: The correct preposition appears to be “on”.
Context: ...2) - Updated the minimum system version in macOS .plist files from 10.13.0 to 10.1...
(IN_WINDOWS)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Go Tests (ubuntu-latest, 1.23)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (1)
docs/src/content/docs/changelog.mdx (1)
52-52: Document potential impact of array initialization change.The change in array field initialization behavior (
fixed-length array types are now initialized with their expected length instead of being empty) could affect existing frontend code that relies on empty arrays.Let's verify the impact:
✅ Verification successful
Fixed-length array initialization change is safe
The change only affects the generated bindings code and improves array initialization by setting the correct length upfront. TypeScript's type system ensures that fixed-length arrays are used correctly in frontend code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for array field declarations in TypeScript/JavaScript files rg -g '*.{ts,js}' 'Array<.*>\[\d+\]|.*\[\d+\]' # Search for array initialization patterns rg -g '*.{ts,js}' 'new Array\(\d+\)|Array\(\d+\)'Length of output: 73160
|
I just pushed testdata updates |
Description
NOTE: the current changeset omits test data updates for easier review. The full patch is available at
fbbdev:v3/bindgen-fixes-tests. The updates will have to be pushed to the PR after reviews, but before merge.This patch fixes a number of problems with circular imports and use-before-define situations in JS/TS code output by the binding generator. It also contains a number of (very) minor code quality improvements.
The fix is expected to be entirely non-breaking and backwards compatible. It should require no action on the part of downstream developers.
Fixes “Circular
modelsandinternalimports in bindings” from the discord feedback channel.Additionally, the patch fixes a bug whereby array fields of fixed length in classes would be initialised with an empty array. This change will break frontend code that expects the default value to be empty.
Major changes:
internal.js/tsfile that used to contain private types only. Public models (i.e. those that are exported in Go terms) are then re-exported from themodels.js/tsfile;wails3 task generator:testin the project directory to run go tests, typescript checker and madge);Type of change
How Has This Been Tested?
I updated and run the binding generator's test harness. I updated the binding example and verified that it runs correctly.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRI have made corresponding changes to the documentationSummary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
wails doctorcommand with a new diagnostics section.GreetService.Improvements
Bug Fixes
Changes
OnStartup→ServiceStartup).WindowIDKeyandWindowNameKey.package.json.