[v3] Service API cleanup and comments#4024
Conversation
WalkthroughThis pull request introduces significant updates to the Wails application framework's service management and documentation. Key changes include the addition of the Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Service as New Service
participant Options as ServiceOptions
App->>Options: Define Service Options
Options-->>Service: Configure Name, Route
App->>Service: NewServiceWithOptions
Service-->>App: Create Service Instance
App->>Service: Register Service
Possibly related PRs
Poem
Finishing Touches
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
v3/pkg/application/services.go (2)
68-77: Provide a callback or hook for startup errorsCurrently, if
ServiceStartupreturns an error, the application logs the error and quits. This might not be desirable in all cases. Consider providing a mechanism for the application to handle startup errors more gracefully, such as a callback or a configurable option to decide whether to abort startup.
83-85: Enhance error handling inServiceShutdownIf
ServiceShutdownreturns a non-nil error, it's only logged with the service name. Depending on the severity of the error, it might be beneficial to allow applications to handle these errors explicitly or perform additional cleanup.docs/src/content/docs/changelog.mdx (3)
40-40: Ensure consistent formatting for changelog entriesIn line 40, the new changelog entry for adding doc comments lacks the contributor's GitHub handle in parentheses, which is used in other entries. For consistency, please include the contributor's handle.
Suggested change:
-- Add doc comments for Service API by [@fbbdev](https://github.com/fbbdev) in [#4024](https://github.com/wailsapp/wails/pull/4024) +- Add doc comments for Service API by [@fbbdev](https://github.com/fbbdev) in [#4024](https://github.com/wailsapp/wails/pull/4024)
Line range hint
68-68: Correct the formatting of the bullet pointThe bullet point starting at line 68 seems to have an extra hyphen, which might disrupt the markdown rendering.
Suggested correction:
-- Removed `WebviewGpuIsDisabled` and `EnableFraudulentWebsiteWarnings` (superseded by `EnabledFeatures` and `DisabledFeatures` options) by [leaanthony](https://github.com/leaanthony) - +- Removed `WebviewGpuIsDisabled` and `EnableFraudulentWebsiteWarnings` (superseded by `EnabledFeatures` and `DisabledFeatures` options) by [leaanthony](https://github.com/leaanthony)
Line range hint
324-324: Fix typo in changelog entryIn line 324, there's a minor typo: "Modified" is misspelled as "Modifed".
Suggested fix:
-- Modifed default context menus by [mmghv](https://github.com/mmghv) in [#2753](https://github.com/wailsapp/wails/pull/2753). +- Modified default context menus by [mmghv](https://github.com/mmghv) in [#2753](https://github.com/wailsapp/wails/pull/2753).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/src/content/docs/changelog.mdx(1 hunks)v3/pkg/application/application_options.go(0 hunks)v3/pkg/application/services.go(1 hunks)
💤 Files with no reviewable changes (1)
- v3/pkg/application/application_options.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
| // NewService returns a Service value wrapping the given pointer. | ||
| // If T is not a concrete named type, the returned value is invalid. | ||
| // | ||
| // It accepts optionally at most one instance of [ServiceOptions]. | ||
| func NewService[T any](instance *T, options ...ServiceOptions) Service { | ||
| switch len(options) { | ||
| case 0: | ||
| return Service{instance, DefaultServiceOptions} | ||
| case 1: | ||
| return Service{instance, options[0]} | ||
| default: | ||
| panic("NewService accepts at most one instance of ServiceOptions") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning an error instead of panicking in NewService
In the NewService function, if more than one ServiceOptions instance is provided, the function panics. Panicking in library code can be disruptive for users and makes error handling less flexible. It's recommended to return an error instead, allowing users to handle it gracefully.
Here's a suggested refactor:
-func NewService[T any](instance *T, options ...ServiceOptions) Service {
+func NewService[T any](instance *T, options ...ServiceOptions) (Service, error) {
switch len(options) {
case 0:
- return Service{instance, DefaultServiceOptions}
+ return Service{instance, DefaultServiceOptions}, nil
case 1:
- return Service{instance, options[0]}
+ return Service{instance, options[0]}, nil
default:
- panic("NewService accepts at most one instance of ServiceOptions")
+ return Service{}, fmt.Errorf("NewService accepts at most one instance of ServiceOptions")
}
}This change modifies the function to return a (Service, error) and replaces the panic with an error return. Callers of NewService can then handle the error appropriately.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@coderabbitai the panic is meant to catch an invalid invocation that should never make it to production. Returning errors should be reserved for recoverable error conditions.
There was a problem hiding this comment.
@fbbdev Understood, thank you for the clarification about the error handling pattern. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: fbbdev
PR: wailsapp/wails#4024
File: v3/pkg/application/services.go:38-51
Timestamp: 2025-01-18T21:19:47.668Z
Learning: In the Wails v3 codebase, panic is used to catch invalid invocations that should never occur in production (programming errors), while error returns are reserved for recoverable error conditions (runtime errors). For example, the `NewService` function in `v3/pkg/application/services.go` uses panic to handle the case where more than one `ServiceOptions` instance is provided, as this is considered an unrecoverable programming error.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
v3/pkg/application/services.go (1)
16-34: Consider adding validation for Route field.The Route field is used for mounting HTTP handlers but lacks validation rules in the documentation. Consider documenting:
- Valid route format (e.g., must start with '/')
- Route uniqueness requirements
- Empty route behavior
v3/internal/generator/analyse.go (1)
129-143: Consider improving error messages.The warning log at line 135 dumps raw values without context. Consider making it more descriptive:
-logger.Warningf("Param Len: %d, Results Len: %d, tp.Len: %d, tp.At(0).Obj(): %v", +logger.Warningf("Invalid signature for %s: expected %d params and %d results, got %d params and %d results. Type params: expected 1, got %d", + fn.Name(), + definition.Inputs, + definition.Outputs, signature.Params().Len(), signature.Results().Len(), - tp.Len(), - tp.At(0).Obj()) + tp.Len())docs/src/content/docs/learn/services.mdx (3)
69-71: Fix inconsistent list indentation.The indentation in the ServiceOptions fields list is inconsistent. The
Namefield uses tabs whileRouteuses spaces.- - Name - Specify a custom name for the Service - - Route - A route to bind the Service to the frontend (more on this below) +- Name - Specify a custom name for the Service +- Route - A route to bind the Service to the frontend (more on this below)
88-89: Fix grammatical error in ServiceName description.There's a missing verb in the sentence describing the default behavior.
-This method returns the name of the service. By default, it will the struct name of the Service but can be +This method returns the name of the service. By default, it will be the struct name of the Service but can be🧰 Tools
🪛 LanguageTool
[grammar] ~88-~88: It seems that a verb or a comma is missing.
Context: ...the name of the service. By default, it will the struct name of the Service but can ...(MD_DT_JJ)
55-56: Consider adding error handling documentation.Given that
NewServicenow panics on invalid invocations (as mentioned in the PR objectives), it would be helpful to add a section about error handling and validation, including:
- What constitutes an invalid invocation
- Examples of conditions that would cause panics
- Best practices for preventing such scenarios
Would you like me to propose content for an error handling section?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/src/content/docs/learn/services.mdx(4 hunks)v3/internal/generator/analyse.go(2 hunks)v3/internal/generator/errors.go(2 hunks)v3/internal/generator/testcases/complex_instantiations/funcs.go(1 hunks)v3/internal/generator/testcases/complex_instantiations/other/funcs.go(1 hunks)v3/pkg/application/services.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- v3/internal/generator/errors.go
🧰 Additional context used
📓 Learnings (1)
v3/pkg/application/services.go (1)
Learnt from: fbbdev
PR: wailsapp/wails#4024
File: v3/pkg/application/services.go:38-51
Timestamp: 2025-01-18T21:19:47.778Z
Learning: In the Wails v3 codebase, panic is used to catch invalid invocations that should never occur in production (programming errors), while error returns are reserved for recoverable error conditions (runtime errors). For example, the `NewService` function in `v3/pkg/application/services.go` uses panic to handle the case where more than one `ServiceOptions` instance is provided, as this is considered an unrecoverable programming error.
🪛 LanguageTool
docs/src/content/docs/learn/services.mdx
[grammar] ~88-~88: It seems that a verb or a comma is missing.
Context: ...the name of the service. By default, it will the struct name of the Service but can ...
(MD_DT_JJ)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (6)
v3/pkg/application/services.go (2)
8-14: LGTM! Well-documented struct definition.The Service struct is well-documented with clear comments about validity and construction. The zero value invalidation pattern is a good practice.
67-82: LGTM! Comprehensive documentation for ServiceStartup interface.The documentation clearly explains:
- Optional nature of the interface
- Context lifecycle
- Error handling behavior
- Shutdown notification guarantees
v3/internal/generator/analyse.go (1)
13-19: LGTM! Clear signature validation map.The map provides a clean way to validate different service creation functions. The structure makes it easy to add new signatures in the future.
v3/internal/generator/testcases/complex_instantiations/funcs.go (1)
5-7: LGTM! Simplified service initializer signature.The function signature has been correctly updated to match the new API which no longer uses ServiceOptions.
v3/internal/generator/testcases/complex_instantiations/other/funcs.go (1)
9-11: LGTM! Simplified service initializer signature.The function signature has been correctly updated to match the new API which no longer uses ServiceOptions.
docs/src/content/docs/learn/services.mdx (1)
124-124: LGTM! Clear and effective example code.The examples effectively demonstrate the usage of
NewServiceWithOptionswith proper configuration ofServiceOptions.Also applies to: 167-171
| func NewService[T any](instance *T) Service { | ||
| return NewServiceWithOptions(instance, DefaultServiceOptions) | ||
| } | ||
|
|
||
| // NewServiceWithOptions returns a Service value wrapping the given pointer | ||
| // and specifying the given service options. | ||
| // If T is not a concrete named type, the returned value is invalid. | ||
| func NewServiceWithOptions[T any](instance *T, options ServiceOptions) Service { | ||
| service := NewService[T](instance) | ||
| service.options = options | ||
| return service | ||
| } |
There was a problem hiding this comment.
Potential infinite recursion in NewServiceWithOptions.
There appears to be a circular dependency where NewServiceWithOptions calls NewService which in turn calls NewServiceWithOptions. This could lead to infinite recursion.
Consider this fix:
func NewServiceWithOptions[T any](instance *T, options ServiceOptions) Service {
- service := NewService[T](instance)
- service.options = options
- return service
+ return Service{
+ instance: instance,
+ options: options,
+ }
}
func NewService[T any](instance *T) Service {
return NewServiceWithOptions(instance, DefaultServiceOptions)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewService[T any](instance *T) Service { | |
| return NewServiceWithOptions(instance, DefaultServiceOptions) | |
| } | |
| // NewServiceWithOptions returns a Service value wrapping the given pointer | |
| // and specifying the given service options. | |
| // If T is not a concrete named type, the returned value is invalid. | |
| func NewServiceWithOptions[T any](instance *T, options ServiceOptions) Service { | |
| service := NewService[T](instance) | |
| service.options = options | |
| return service | |
| } | |
| func NewService[T any](instance *T) Service { | |
| return NewServiceWithOptions(instance, DefaultServiceOptions) | |
| } | |
| // NewServiceWithOptions returns a Service value wrapping the given pointer | |
| // and specifying the given service options. | |
| // If T is not a concrete named type, the returned value is invalid. | |
| func NewServiceWithOptions[T any](instance *T, options ServiceOptions) Service { | |
| return Service{ | |
| instance: instance, | |
| options: options, | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/internal/generator/testcases/complex_instantiations/main.go (1)
23-23: LGTM! Consider adding test case documentation.The new Service13 type and its instantiation with custom options effectively demonstrate the enhanced service API capabilities. Consider adding a comment explaining the purpose of this test case to help future maintainers understand the various instantiation patterns being tested.
+// Service13 demonstrates service instantiation with custom options type Service13 struct{}Also applies to: 40-40
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v3/internal/commands/bindings.go(1 hunks)v3/internal/generator/testcases/complex_instantiations/bound_types.json(1 hunks)v3/internal/generator/testcases/complex_instantiations/main.go(2 hunks)v3/internal/generator/testcases/complex_instantiations/other/local.go(1 hunks)v3/pkg/application/services.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/pkg/application/services.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (3)
v3/internal/commands/bindings.go (1)
80-80: LGTM! Good security practice.The explicit format string
"%s"prevents potential format string vulnerabilities if theresultMessagecontains format specifiers. This is a good defensive programming practice.v3/internal/generator/testcases/complex_instantiations/other/local.go (1)
5-7: LGTM! Verify service type consistency.The service type change and instantiation look correct. However, let's verify that this change is consistently reflected across all test cases.
✅ Verification successful
Services are correctly defined in their respective packages
The verification shows that Service13 and Service14 are distinct services meant to coexist:
- Service13 is defined in the main package
- Service14 is defined in the 'other' package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to Service13 in the test cases directory rg "Service13" "v3/internal/generator/testcases"Length of output: 388
Script:
#!/bin/bash # Check for all references to Service14 to understand the scope rg "Service14" "v3/internal/generator/testcases"Length of output: 358
v3/internal/generator/testcases/complex_instantiations/bound_types.json (1)
14-15:⚠️ Potential issueFix inconsistent path format.
The path format for Service14 uses an absolute path (
/other.Service14) while all other services, including Service13, use relative paths with dot prefix (.ServiceX). This inconsistency should be addressed to maintain uniformity in type binding.".Service12", ".Service13", - "/other.Service14" + ".Service14"Likely invalid or redundant comment.
…eanup' into fork/fbbdev/v3-alpha-refactor/service-api-cleanup
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/examples/services/main.go (1)
Line range hint
46-50: Consider standardizing service initialization patterns.While the new
NewServiceWithOptionsusage for the fileserver is correct, there's an inconsistency in how services are initialized throughout the example. Other services still useNewServicewithout options.Consider standardizing the initialization pattern by using
NewServiceWithOptionsfor all services where applicable, making the configuration more explicit. For example:- application.NewService(hashes.New()), + application.NewServiceWithOptions(hashes.New(), application.ServiceOptions{}),This would make the code more consistent and self-documenting, showing that options are available even when not needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/examples/services/main.go(1 hunks)
⏰ 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)
v3/examples/services/main.go (1)
Line range hint
46-50: Verify route handling setup.The fileserver is now configured with a "/files" route. Ensure that:
- The route doesn't conflict with other services
- The frontend code is updated to use this route
- The route is documented in relevant API documentation
Let's check for potential route conflicts:
| // the canonical path for the standard context package | ||
| // did not match any actual package. | ||
| var ErrNoContextPackage = errors.New("standard context package not found at canonical import path ('context'): is the Wails v3 module properly installed?") | ||
| var ErrNoContextPackage = errors.New("standard context package not found at canonical import path ('context'): is the Wails v3 module properly installed? ") |
There was a problem hiding this comment.
This is just to make sure that the final space here and on the next two lines has been added on purpose.
* Gather and document service API * Update changelog * Add NewServiceWithOptions * Revert static analyser change * Remove infinite loop in NewService[WithOptions] * Fix compiler warning in bindings command * Add test for NewServiceWithOptions * Update changelog * Fix service example --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
Description
This patch proposes a regrouping of the service API in a single file
v3/pkg/application/services.go, following the layout of the more recent single instance API.It adds comprehensive doc comments to the service API for a better IDE experience.
It changes the
application.NewServicefunction to take exactly one parameter and adds a newapplication.NewServiceWithOptionsto pass additional options at creation time.Discussed on Discord at A couple API refactorings and Two potential bugs in service API.
Type of change
How Has This Been Tested?
Ran the go test suite and binding, service examples.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Documentation
Breaking Changes
Name→ServiceNameOnStartup→ServiceStartupOnShutdown→ServiceShutdownServiceandServiceOptionsstructsServicestruct with enhanced configuration optionsNewServicemethod to no longer accept optionsNew Features
NewServiceWithOptionsfor service initialization