|
| 1 | +--- |
| 2 | +issue: "#208" |
| 3 | +date: 2026-05-17 |
| 4 | +slug: 208-constructor-injection |
| 5 | +spec: docs/specs/2026-05-17-208-constructor-injection.md |
| 6 | +--- |
| 7 | + |
| 8 | +# Plan: Decompose `Ctrl` god object via constructor injection (#208) |
| 9 | + |
| 10 | +## Approach |
| 11 | + |
| 12 | +Remove `public ctrl: JournalController` from each action/UI constructor and replace it with the specific sub-interfaces the class actually uses. `Ctrl.initServices(logger: ILogger)` becomes the single composition step — it creates all action objects in dependency order and passes narrow sub-interfaces. `Startup.registerLoggingChannel` triggers it by creating `ConsoleLogger` first, then passing it to `initServices`. |
| 13 | + |
| 14 | +**Amendment vs spec:** The spec said `initServices(channel: vscode.OutputChannel)`; this plan changes the signature to `initServices(logger: ILogger)`. Reason: `Ctrl` should not know about `vscode.OutputChannel`; pushing `ConsoleLogger` construction into `Startup` is better SRP. Tests can pass `TestLogger` directly. Behaviour is identical. |
| 15 | + |
| 16 | +Main trade-off: 14 test files all do `ctrl.logger = new TestLogger(false)` — they must be updated to `ctrl.initServices(new TestLogger(false))` instead. The change is mechanical but touches many files. |
| 17 | + |
| 18 | +## Steps |
| 19 | + |
| 20 | +### Step 1 — `ConsoleLogger`: remove `ctrl`, accept `config: IConfiguration` |
| 21 | + |
| 22 | +`src/util/logger.ts` constructor change: |
| 23 | +```typescript |
| 24 | +// Before: constructor(public ctrl: JournalController, public channel: vscode.OutputChannel) |
| 25 | +// After: constructor(private config: IConfiguration, public channel: vscode.OutputChannel) |
| 26 | +``` |
| 27 | +Remove the `ctrl` field entirely; the constructor already copies `ctrl.config.isDevelopmentModeEnabled()` into `this.devMode` — copy from `config` directly now. No other body changes needed. |
| 28 | + |
| 29 | +### Step 2 — `Inject`: remove `ctrl`, accept narrow params |
| 30 | + |
| 31 | +`src/actions/inject.ts`: |
| 32 | +- Constructor: `constructor(private config: IConfiguration, private logger: ILogger)` |
| 33 | +- Remove `public ctrl` field |
| 34 | +- Replace all `this.ctrl.config.` → `this.config.` |
| 35 | +- Replace all `this.ctrl.logger.` → `this.logger.` |
| 36 | + |
| 37 | +### Step 3 — `Parser`: remove `ctrl`, accept narrow params |
| 38 | + |
| 39 | +`src/actions/parser.ts`: |
| 40 | +- Constructor: `constructor(private config: IConfiguration, private logger: ILogger)` |
| 41 | +- Replace all `this.ctrl.config.` → `this.config.` |
| 42 | +- Replace all `this.ctrl.logger.` → `this.logger.` |
| 43 | + |
| 44 | +### Step 4 — `ScanEntries`: remove `ctrl`, accept narrow params |
| 45 | + |
| 46 | +`src/provider/features/scan-entries.ts`: |
| 47 | +- Constructor: `constructor(private config: IConfiguration, private logger: ILogger)` |
| 48 | +- Replace all `this.ctrl.config.` → `this.config.` |
| 49 | +- Replace all `this.ctrl.logger.` → `this.logger.` |
| 50 | + |
| 51 | +### Step 5 — `Writer`: remove `ctrl`, accept narrow params |
| 52 | + |
| 53 | +`src/actions/writer.ts`: |
| 54 | +- Constructor: `constructor(private config: IConfiguration, private logger: ILogger, private inject: IInject)` |
| 55 | +- Replace all `this.ctrl.config.` → `this.config.` |
| 56 | +- Replace all `this.ctrl.logger.` → `this.logger.` |
| 57 | +- Replace all `this.ctrl.inject.` → `this.inject.` |
| 58 | + |
| 59 | +### Step 6 — `Dialogues`: remove `ctrl`, accept narrow params |
| 60 | + |
| 61 | +`src/ext/dialogues.ts`: |
| 62 | +- Constructor: `constructor(private config: IConfiguration, private logger: ILogger, private parser: IParser)` |
| 63 | +- The scanner field is created internally: change `new ScanEntries(this.ctrl)` → `new ScanEntries(config, logger)` (Step 4 makes this possible) |
| 64 | +- `getScanner()` return type and body unchanged |
| 65 | +- Replace all `this.ctrl.config.` → `this.config.` |
| 66 | +- Replace all `this.ctrl.logger.` → `this.logger.` |
| 67 | +- Replace all `this.ctrl.parser.` → `this.parser.` |
| 68 | + |
| 69 | +### Step 7 — `Reader`: remove `ctrl`, accept narrow params |
| 70 | + |
| 71 | +`src/actions/reader.ts`: |
| 72 | +- Constructor: `constructor(private config: IConfiguration, private logger: ILogger, private writer: IWriter, private ui: IDialogues)` |
| 73 | +- Replace all `this.ctrl.config.` → `this.config.` |
| 74 | +- Replace all `this.ctrl.logger.` → `this.logger.` |
| 75 | +- Replace all `this.ctrl.writer.` → `this.writer.` |
| 76 | +- Replace all `this.ctrl.ui.` → `this.ui.` |
| 77 | + |
| 78 | +### Step 8 — `Ctrl`: add `initServices`, remove construction from constructor |
| 79 | + |
| 80 | +`src/util/controller.ts`: |
| 81 | +- **Constructor**: remove the five `new J.Actions.*` and `new J.Extension.Dialogues` lines; keep only `this._config = new Configuration(vscodeConfig)` |
| 82 | +- **Add** `initServices(logger: ILogger): void` that creates all services in dependency order: |
| 83 | + ``` |
| 84 | + 1. this._logger = logger |
| 85 | + 2. this._inject = new Inject(this._config, logger) |
| 86 | + 3. this._writer = new Writer(this._config, logger, this._inject) |
| 87 | + 4. this._parser = new Parser(this._config, logger) |
| 88 | + 5. this._ui = new Dialogues(this._config, logger, this._parser) |
| 89 | + 6. this._reader = new Reader(this._config, logger, this._writer, this._ui) |
| 90 | + ``` |
| 91 | +- **Import** the concrete classes directly: `import { Parser } from '../actions/parser'`, `import { Writer } from '../actions/writer'`, etc. (replace `J.Actions.*` and `J.Extension.*` references inside `initServices`) |
| 92 | +- **Remove** the `logger` setter (setter injection replaced by `initServices`) |
| 93 | +- **Keep** all getters unchanged |
| 94 | +- `_logger` field type stays `Logger | undefined`; getter guard unchanged |
| 95 | + |
| 96 | +Note: `controller.ts` still has `import * as J from '../.'` (barrel) for the field types in the class body (#201 work). This is acceptable for now; the barrel import is only for type references. |
| 97 | + |
| 98 | +### Step 9 — `Startup`: trigger `initServices` from `registerLoggingChannel` |
| 99 | + |
| 100 | +`src/ext/startup.ts`, `registerLoggingChannel` method only: |
| 101 | +```typescript |
| 102 | +// Before: |
| 103 | +ctrl.logger = new J.Util.ConsoleLogger(ctrl, channel); |
| 104 | +ctrl.logger.debug("..."); |
| 105 | +return ctrl; |
| 106 | + |
| 107 | +// After: |
| 108 | +const logger = new J.Util.ConsoleLogger(ctrl.config, channel); |
| 109 | +ctrl.initServices(logger); |
| 110 | +ctrl.logger.debug("..."); |
| 111 | +return ctrl; |
| 112 | +``` |
| 113 | +No other changes to `Startup`. |
| 114 | + |
| 115 | +### Step 10 — Tests: replace two-line pattern with `initServices` |
| 116 | + |
| 117 | +Every test file that does: |
| 118 | +```typescript |
| 119 | +const ctrl = new J.Util.Ctrl(config); |
| 120 | +ctrl.logger = logger; |
| 121 | +``` |
| 122 | +changes to: |
| 123 | +```typescript |
| 124 | +const ctrl = new J.Util.Ctrl(config); |
| 125 | +ctrl.initServices(logger); |
| 126 | +``` |
| 127 | + |
| 128 | +Files affected (14): `commands-entry.test.ts`, `commands-inject.test.ts`, `commands-note.test.ts`, `commands-prev-next.test.ts`, `commands-weekly.test.ts`, `input.test.ts`, `issue-168-entry-granularity.test.ts`, `issue-185-weekly-sync.test.ts`, `issue-51-remote-create.test.ts`, `notes-sync.test.ts`, `phase1-regression.test.ts`, `read-templates.test.ts`, `scan-entries-cache.test.ts`, `week-input.test.ts`. |
| 129 | + |
| 130 | +`scan-entries-cache.test.ts` additionally creates `new ScanEntries(ctrl)` — change to `new ScanEntries(ctrl.config, ctrl.logger)`. |
| 131 | + |
| 132 | +### Step 11 — Verify |
| 133 | + |
| 134 | +```bash |
| 135 | +npm run compile # zero errors |
| 136 | +npm run lint # zero new warnings |
| 137 | +npm test # 126 tests pass |
| 138 | +grep -rn "this\.ctrl" src/actions/ src/ext/dialogues.ts src/util/logger.ts src/provider/features/scan-entries.ts |
| 139 | +# must return no output |
| 140 | +``` |
| 141 | + |
| 142 | +## Test scenarios |
| 143 | + |
| 144 | +**T1 — Compile validates all parameter types (unit: TypeScript)** |
| 145 | +After each step, run `npm run compile` to confirm no type errors. TypeScript rejects a `config: IConfiguration` where `ILogger` is expected, so mismatches surface immediately. |
| 146 | + |
| 147 | +**T2 — `Ctrl.initServices` correctly wires services (integration)** |
| 148 | +`npm test` passes all 126 tests. Tests create `new Ctrl(config); ctrl.initServices(testLogger)` — identical runtime path to production, just with `TestLogger` instead of `ConsoleLogger`. |
| 149 | + |
| 150 | +**T3 — Barrel-free constructor params (static)** |
| 151 | +`grep -rn "this\.ctrl" src/actions/ src/ext/dialogues.ts src/util/logger.ts src/provider/features/scan-entries.ts` → no output. |
| 152 | + |
| 153 | +**T4 — `logger` getter guard still active (unit)** |
| 154 | +Before `initServices` is called, `ctrl.logger` throws `"Tried to access undefined logger"`. Confirmed by compile check: getter body is unchanged. |
| 155 | + |
| 156 | +**T5 — `Startup` initialisation path unchanged (integration)** |
| 157 | +Extension activate path: `Ctrl(config)` → `initialize()` → `registerLoggingChannel()` → `initServices(logger)` → all getters populated → commands registered. Covered by T2 (full suite exercises this path through Extension Host). |
| 158 | + |
| 159 | +## Dependencies |
| 160 | + |
| 161 | +No other PRs or branches must land first. #201 (barrel removal in `controller.ts`) is a parallel track and can land in either order. |
| 162 | + |
| 163 | +## Risk |
| 164 | + |
| 165 | +| Risk | Coverage | |
| 166 | +|---|---| |
| 167 | +| Missing constructor arg → tsc error | T1 (compile after each step) | |
| 168 | +| Test file misses `initServices` call → `undefined` service at runtime | T2 (test failures) | |
| 169 | +| `Dialogues` creates wrong `ScanEntries` args → runtime crash | T2 | |
| 170 | +| `logger` getter called before `initServices` → error thrown | T4 | |
| 171 | +| `Startup` wiring broken → extension won't activate | T5 | |
| 172 | + |
| 173 | +## Rollback |
| 174 | + |
| 175 | +`git revert <commit>` — pure structural refactor, no data migrations. |
| 176 | + |
| 177 | +## Reference spec |
| 178 | + |
| 179 | +[docs/specs/2026-05-17-208-constructor-injection.md](../specs/2026-05-17-208-constructor-injection.md) |
0 commit comments