From 5ca6023a5a31c3be4278f07c48831d9c2c9376c9 Mon Sep 17 00:00:00 2001 From: Marc Mintel Date: Sun, 11 Jan 2026 16:44:08 +0100 Subject: [PATCH] docs --- plans/website-architecture-violations.md | 116 +++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/plans/website-architecture-violations.md b/plans/website-architecture-violations.md index 4d6aea743..21380349f 100644 --- a/plans/website-architecture-violations.md +++ b/plans/website-architecture-violations.md @@ -28,6 +28,93 @@ Hard boundaries to enforce: - No locale APIs (`Intl.*`, `toLocale*`) in any formatting path (guardrails: [`VIEW_DATA.md`](docs/architecture/website/VIEW_DATA.md:41), [`WEBSITE_GUARDRAILS.md`](docs/architecture/website/WEBSITE_GUARDRAILS.md:13)). - All writes enter via Server Actions (contract: [`FORM_SUBMISSION.md`](docs/architecture/website/FORM_SUBMISSION.md:18)). +### 0.1 Naming: stop calling everything “dto” (clarify the model taxonomy) + +This repo currently uses `dto` as a generic variable name, which collapses three different concepts. + +We will use the contract’s model names and enforce naming conventions to prevent category errors. + +Authoritative types: + +- **API Transport DTO**: returned by `apps/api` over HTTP. In this repo, often generated from OpenAPI. + - Canonical placement: [`apps/website/lib/types/**`](apps/website/lib/types/League.ts:1) per [`WEBSITE_CONTRACT.md`](docs/architecture/website/WEBSITE_CONTRACT.md:41). + - Naming rule: + - Type names end with `ApiDto` (preferred) or `DTO` (existing generated types). + - Variable names: `apiDto`, `apiResponse`. + +- **Page DTO**: website-owned server-to-client payload assembled by PageQueries. + - Canonical placement: `apps/website/lib/page-queries/**` per [`WEBSITE_CONTRACT.md`](docs/architecture/website/WEBSITE_CONTRACT.md:54). + - Naming rule: + - Type names end with `PageDto`. + - Variable names: `pageDto`. + +- **ViewData**: the only allowed input to Templates. + - Canonical placement: `apps/website/templates/**` per [`WEBSITE_CONTRACT.md`](docs/architecture/website/WEBSITE_CONTRACT.md:86). + - Naming rule: + - Type names end with `ViewData`. + - Variable names: `viewData`. + +Other model names we should keep explicit: + +- **ViewModel**: client-only class instance. + - Naming rule: `*ViewModel` types/classes, variables `viewModel`. + +- **Command DTO** (write intent): the payload sent to Server Actions / API for mutations. + - Naming rule: `*CommandDto`, variables `commandDto`. + +Non-negotiable rule: + +- Never use a bare `dto` variable name. Use `apiDto`, `pageDto`, `viewData`, or `commandDto`. + +This is a Clean Architecture guardrail: when names are precise, wrong-layer dependencies become obvious during review. + +### 0.2 Enforce the taxonomy with required abstractions (not lint) + +You asked for “proper abstractions that must be implemented” (interfaces and/or abstract classes), rather than naming-only or ESLint enforcement. + +The goal is that the *compiler* (and the module boundaries) makes it hard to mix: + +- API Transport DTO vs Page DTO vs ViewData +- server-only vs client-only +- orchestration vs presentation mapping + +Proposed required abstractions (website-owned): + +1. **API Client contracts** (transport boundary) + - Interface per API area, returns **ApiDto/DTO only**. + - Example: `LeaguesApiClient` already exists; enforce that it returns `*DTO` types only. + - Prohibit returning ViewModels. + +2. **Service contracts** (orchestration boundary, server-safe) + - Interface per feature: `*Service` returns **ApiDto** or **PageDto** only. + - Must be stateless. + - Must not import `lib/view-models/**`. + +3. **PageQuery contract** (server composition boundary) + - A generic interface: + - `execute(params) -> PageQueryResult` + - PageQueries must only depend on API clients + services (manual wiring). + - Must not import `lib/view-models/**`. + +4. **Presenter contract** (pure mapping boundary, client-only) + - Presenter is a pure mapper: + - `PageDto -> ViewData` OR `PageDto -> ViewModel` OR `ViewModel -> ViewData` + - No HTTP, no side effects. + +5. **ViewModel base** (client-only) + - Optional but useful to enforce non-serializability: + - e.g. `abstract class ViewModel { abstract toViewData(): ViewData }` + - No `toDTO()` method that resembles server DTOs (it tempts server usage). + +6. **DisplayObject base** (deterministic formatting) + - Abstract base or interface to enforce “no locale APIs” policy by convention and review. + - Encourages `new MoneyDisplay(amount).formatted()` style, but only used from Presenters/ViewModels. + +Implementation note: + +- These abstractions live in `apps/website/lib/contracts/**` so every feature must conform. +- This is complementary to guardrails: guardrails prevent forbidden imports; contracts ensure the correct “shape” of each layer. + --- ## 1) Remediation plan (explicit actions) @@ -58,6 +145,22 @@ Deliverable for this section: - A CI-failing rule that prevents future regressions. - A grep for `getContainer()` in server modules returns zero hits. +Rationale: why DI is banned on the server side + +This is a deliberate safety simplification. + +1. Next.js server execution is concurrent; DI containers make it easy to accidentally share state across requests. + - The contract explicitly warns about this risk ([`WEBSITE_CONTRACT.md`](docs/architecture/website/WEBSITE_CONTRACT.md:227), [`WEBSITE_DI_RULES.md`](docs/architecture/website/WEBSITE_DI_RULES.md:9)). + - The singleton container is explicitly called out as unsafe ([`ContainerManager`](apps/website/lib/di/container.ts:61), [`WEBSITE_DI_RULES.md`](docs/architecture/website/WEBSITE_DI_RULES.md:33)). + +2. The codebase already contains stateful “service” patterns (e.g. blockers stored as instance fields), which are harmless in a per-request graph but dangerous in a shared container. + - Example: [`LeagueService`](apps/website/lib/services/leagues/LeagueService.ts:95) stores [`submitBlocker`](apps/website/lib/services/leagues/LeagueService.ts:96). + +3. Clean Architecture intent: server modules are composition roots; manual wiring makes dependencies explicit and reviewable. + - This reduces hidden coupling and prevents accidental imports of client-only types like `lib/view-models/**`. + +Note: the strict contract technically allows request-scoped server DI ([`WEBSITE_DI_RULES.md`](docs/architecture/website/WEBSITE_DI_RULES.md:25)), but we choose the stronger rule (manual wiring only) to eliminate an entire class of cross-request bugs and enforcement ambiguity. + ### 1.2 Standardize PageQuery contract (one discriminated union) **Goal:** all PageQueries return the exact contract described in [`WEBSITE_PAGE_QUERIES.md`](docs/architecture/website/WEBSITE_PAGE_QUERIES.md:17). @@ -807,6 +910,19 @@ Non-negotiable target: - `lib/view-models/**` + Presenters: client-only; pure mapping to ViewData. - `templates/**`: dumb renderer; ViewData only. +Naming enforcement (applies across all refactors): + +- API response shapes are `ApiDto`/`DTO` and stored/handled as `apiDto`. +- PageQuery output is always named `pageDto` and typed `*PageDto`. +- Template props are always `*ViewData` and passed as `viewData`. + +Abstractions enforcement (applies across all refactors): + +- Every PageQuery implements a common `PageQuery` contract (in `lib/contracts/page-queries/**`). +- Every service implements a `Service` interface that returns `ApiDto`/`PageDto` only (in `lib/contracts/services/**`). +- Every Presenter implements `Presenter` with `Output` being `ViewData` or `ViewModel` (in `lib/contracts/presenters/**`). +- ViewModels extend a shared base (in `lib/contracts/view-models/**`) to discourage accidental server serialization patterns. + ### 9.1 Guardrails first (prevent regression) Add CI-failing checks (tests or ESLint rules). These come first to enforce Clean Architecture boundaries while refactors are in flight: