feat(rfc): resolve ALL 16 known issues
Pass one complete. All 16 issues now have dispositions: Resolved (design decisions): - 1: Naming convention — CamelCase=primitive, UPPERCASE=sugar - 5: RegisterAction location → solved by Issue 16 split - 6: serviceRegistry → exported via Registry[T] (Section 20) - 8: NewRuntime → NOT legacy, it's GUI bridge. Update factory signature. - 9: CommandLifecycle → three-layer CLI (Cli/cli/go-process) - 10: Array[T] → guardrail primitive, keep - 11: ConfigVar[T] → promote to documented primitive - 12: Ipc → owns Action registry, reads from Registry[T] - 16: task.go → splits into ipc.go + action.go Resolved (mechanical fixes): - 2: MustServiceFor → keep, document startup-only usage - 3: Embed() → remove (dead code) - 4: Logging → document boundary (global=bootstrap, Core=runtime) - 13: Lock allocation → use Registry[T], cache Lock struct - 14: Startables/Stoppables → return []*Service directly - 15: Stale comment → fix to match *Core return Blocked (planned): - 7: c.Process() → spec'd in Section 17, needs go-process v0.7.0 Co-Authored-By: Virgil <virgil@lethean.io>
This commit is contained in:
Snider
parent
b130309c3d
commit
59dcbc2a31
1 changed files with 94 additions and 30 deletions
124
docs/RFC.md
124
docs/RFC.md
|
|
@ -1389,7 +1389,7 @@ c.IPC().Actions() // all registered action names
|
|||
|
||||
The UPPERCASE methods stay for backwards compatibility and convenience — a service that just wants to broadcast uses `c.ACTION(msg)`. A service that needs to inspect capabilities or invoke by name uses `c.Action("name")`.
|
||||
|
||||
### 2. MustServiceFor Uses Panic
|
||||
### 2. MustServiceFor Uses Panic (Resolved)
|
||||
|
||||
```go
|
||||
func MustServiceFor[T any](c *Core, name string) T {
|
||||
|
|
@ -1397,42 +1397,75 @@ func MustServiceFor[T any](c *Core, name string) T {
|
|||
}
|
||||
```
|
||||
|
||||
RFC-025 says "no hidden panics." `Must` prefix signals it, but the pattern contradicts the Result philosophy. Consider deprecating in favour of `ServiceFor` + `if !ok` pattern.
|
||||
**Resolution:** Keep but document clearly. `Must` prefix is a Go convention that signals "this panics." In the guardrail context (Issue 10/11), `MustServiceFor` is valid for startup-time code where a missing service means the app can't function. The alternative — `ServiceFor` + `if !ok` + manual error handling — adds LOC that contradicts the Result philosophy.
|
||||
|
||||
### 3. Embed() Legacy Accessor
|
||||
**Rule:** Use `MustServiceFor` only in `OnStartup` / init paths where failure is fatal. Never in request handlers or runtime code. Document this in RFC-025.
|
||||
|
||||
### 3. Embed() Legacy Accessor (Resolved)
|
||||
|
||||
```go
|
||||
func (c *Core) Embed() Result { return c.data.Get("app") }
|
||||
```
|
||||
|
||||
Dead accessor with "use Data()" comment. Should be removed — it's API surface clutter that confuses agents.
|
||||
**Resolution:** Remove. It's a shortcut to `c.Data().Get("app")` with a misleading name. `Embed` sounds like it embeds something — it actually reads. An agent seeing `c.Embed()` can't know it's reading from Data. Dead code, remove in next refactor.
|
||||
|
||||
### 4. Package-Level vs Core-Level Logging
|
||||
### 4. Package-Level vs Core-Level Logging (Resolved)
|
||||
|
||||
```go
|
||||
core.Info("msg") // global default logger
|
||||
core.Info("msg") // global default logger — no Core needed
|
||||
c.Log().Info("msg") // Core's logger instance
|
||||
```
|
||||
|
||||
Both work. Global functions exist for code without Core access (early init, proc.go helpers). Services with Core access should use `c.Log()`. Document the boundary.
|
||||
**Resolution:** Both stay. Document the boundary:
|
||||
|
||||
### 5. RegisterAction Lives in task.go
|
||||
| Context | Use | Why |
|
||||
|---------|-----|-----|
|
||||
| Has `*Core` (services, handlers) | `c.Log().Info()` | Logger may be configured per-Core |
|
||||
| No `*Core` (init, package-level, helpers) | `core.Info()` | Global logger, always available |
|
||||
| Test code | `core.Info()` | Tests may not have Core |
|
||||
|
||||
IPC registration (`RegisterAction`, `RegisterActions`, `RegisterTask`) is in `task.go` but the dispatch functions (`Action`, `Query`, `QueryAll`) are in `ipc.go`. All IPC should be in one file or the split should follow a clear boundary (dispatch vs registration).
|
||||
This is the same dual pattern as `process.Run()` (global) vs `c.Process().Run()` (Core). Package-level functions are the bootstrap path. Core methods are the runtime path.
|
||||
|
||||
### 6. serviceRegistry Is Unexported
|
||||
### 5. RegisterAction Lives in task.go (Resolved by Issue 16)
|
||||
|
||||
`serviceRegistry` is unexported, meaning consumers can't extend service management. Per the Lego Bricks philosophy, this should be exported so downstream packages can build on it.
|
||||
Resolved — task.go splits into ipc.go (registration) + action.go (execution). See Issue 16.
|
||||
|
||||
### 6. serviceRegistry Is Unexported (Resolved by Section 20)
|
||||
|
||||
Resolved — `serviceRegistry` becomes `ServiceRegistry` embedding `Registry[*Service]`. See Section 20.
|
||||
|
||||
### 7. No c.Process() Accessor
|
||||
|
||||
Process management (go-process) should be a Core subsystem accessor like `c.Fs()`, not a standalone service retrieved via `ServiceFor`. Planned for go-process v0.7.0 update.
|
||||
Spec'd in Section 17. Blocked on go-process v0.7.0 update.
|
||||
|
||||
### 8. NewRuntime / NewWithFactories — Legacy
|
||||
### 8. NewRuntime / NewWithFactories — GUI Bridge, Not Legacy (Resolved)
|
||||
|
||||
These pre-v0.7.0 functions take `app any` instead of `*Core`. `Runtime` is a separate struct from `Core` with its own `ServiceStartup`/`ServiceShutdown`. This was the original bootstrap pattern before `core.New()` + `WithService` replaced it.
|
||||
NOT dead code. `Runtime` is the **GUI binding container** — it bridges Core to frontend frameworks like Wails. `App.Runtime` holds the Wails app reference (`any`). This is the core-webview-bridge: CoreGO exported methods → Wails WebView2 → CoreTS fronts them.
|
||||
|
||||
**Question:** Is anything still using `NewRuntime`/`NewWithFactories`? If not, remove. If yes, migrate to `core.New()`.
|
||||
```go
|
||||
type Runtime struct {
|
||||
app any // Wails app or equivalent
|
||||
Core *Core // the Core instance
|
||||
}
|
||||
```
|
||||
|
||||
**Issue:** `NewWithFactories` uses the old factory pattern (`func() Result` instead of `func(*Core) Result`). Factories don't receive Core, so they can't use DI during construction.
|
||||
|
||||
**Resolution:** Update `NewWithFactories` to accept `func(*Core) Result` factories (same as `WithService`). The `Runtime` struct stays — it's the GUI bridge, not a replacement for Core. Consider adding `core.WithRuntime(app)` as a CoreOption:
|
||||
|
||||
```go
|
||||
// Current:
|
||||
r := core.NewWithFactories(wailsApp, factories)
|
||||
|
||||
// Target:
|
||||
c := core.New(
|
||||
core.WithRuntime(wailsApp),
|
||||
core.WithService(display.Register),
|
||||
core.WithService(gui.Register),
|
||||
)
|
||||
```
|
||||
|
||||
This unifies CLI and GUI bootstrap — same `core.New()`, just with `WithRuntime` added.
|
||||
|
||||
### 9. CommandLifecycle — The Three-Layer CLI Architecture
|
||||
|
||||
|
|
@ -1605,42 +1638,73 @@ This is the same pattern as:
|
|||
- **`c.Drive()`** owns connection config, **`c.API()`** opens streams using it
|
||||
- **`c.Data()`** owns mounts, **`c.Embed()`** was the legacy shortcut
|
||||
|
||||
### 13. Lock() Allocates on Every Call
|
||||
### 13. Lock() Allocates on Every Call (Resolved)
|
||||
|
||||
```go
|
||||
func (c *Core) Lock(name string) *Lock {
|
||||
// ... looks up or creates mutex ...
|
||||
return &Lock{Name: name, Mutex: m} // new struct every call
|
||||
}
|
||||
```
|
||||
|
||||
The mutex is cached and reused. The `Lock` wrapper struct is allocated fresh every call. `c.Lock("drain").Mutex.Lock()` allocates a throwaway struct just to access the mutex.
|
||||
The mutex is cached. The `Lock` wrapper is not.
|
||||
|
||||
**Resolution:** Cache the `Lock` struct alongside the mutex, or return `*sync.RWMutex` directly since the `Lock` struct's only purpose is carrying the `Name` field (which the caller already knows).
|
||||
**Resolution:** With `Registry[T]` (Section 20), Lock becomes a Registry:
|
||||
|
||||
### 14. Startables() / Stoppables() Return Result
|
||||
```go
|
||||
// Lock registry becomes:
|
||||
type LockRegistry struct {
|
||||
*Registry[*sync.RWMutex]
|
||||
}
|
||||
|
||||
// Usage stays the same but no allocation:
|
||||
c.Lock("drain") // returns cached *Lock, not new allocation
|
||||
```
|
||||
|
||||
The `Lock` struct can cache itself in the registry alongside the mutex. Or simpler: `c.Lock("drain")` returns `*sync.RWMutex` directly — the caller already knows the name.
|
||||
|
||||
### 14. Startables() / Stoppables() Return Result (Resolved)
|
||||
|
||||
```go
|
||||
func (c *Core) Startables() Result // Result{[]*Service, true}
|
||||
func (c *Core) Stoppables() Result // Result{[]*Service, true}
|
||||
```
|
||||
|
||||
These return `Result` wrapping `[]*Service`, requiring a type assertion. They're only called by `ServiceStartup`/`ServiceShutdown` internally. Since they always succeed and always return `[]*Service`, they should return `[]*Service` directly.
|
||||
|
||||
**Resolution:** Change signatures to `func (c *Core) Startables() []*Service`. Or if keeping Result for consistency, document that `r.Value.([]*Service)` is the expected assertion.
|
||||
|
||||
### 15. contract.go Comment Says New() Returns Result
|
||||
**Resolution:** With `Registry[T]` and `ServiceRegistry`, these become registry queries:
|
||||
|
||||
```go
|
||||
// r := core.New(...)
|
||||
// if !r.OK { log.Fatal(r.Value) }
|
||||
// c := r.Value.(*Core)
|
||||
// Current — type assertion required
|
||||
startables := c.Startables()
|
||||
for _, s := range startables.Value.([]*Service) { ... }
|
||||
|
||||
// Target — registry filter
|
||||
for _, s := range c.Registry("services").Each(func(name string, svc *Service) bool {
|
||||
return svc.OnStart != nil
|
||||
}) { ... }
|
||||
```
|
||||
|
||||
Or simpler: change return type to `[]*Service` directly. These are internal — only `ServiceStartup`/`ServiceShutdown` call them. No need for Result wrapping.
|
||||
|
||||
### 15. contract.go Comment Says New() Returns Result (Resolved)
|
||||
|
||||
```go
|
||||
// r := core.New(...) // WRONG — stale comment
|
||||
// if !r.OK { ... } // WRONG
|
||||
// c := r.Value.(*Core) // WRONG
|
||||
func New(opts ...CoreOption) *Core {
|
||||
```
|
||||
|
||||
The comment shows the old API where `New()` returned `Result`. The actual signature returns `*Core` directly. This was changed during the v0.4.0 restructure but the comment wasn't updated.
|
||||
**Resolution:** Fix the comment. Simple mechanical fix:
|
||||
|
||||
**Resolution:** Fix the comment to match the signature. `New()` returns `*Core` because Core is the one type that can't wrap its own creation in `Result` (it doesn't exist yet).
|
||||
```go
|
||||
// c := core.New(
|
||||
// core.WithOption("name", "myapp"),
|
||||
// core.WithService(mypackage.Register),
|
||||
// )
|
||||
// c.Run()
|
||||
func New(opts ...CoreOption) *Core {
|
||||
```
|
||||
|
||||
`New()` returns `*Core` directly — it's the one constructor that can't wrap its own creation error in Result.
|
||||
|
||||
### 16. task.go Mixes Concerns (Resolved)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue