Skip to content

Commit 5a47ae6

Browse files
justin808claude
andcommitted
Add analysis documents for console replay parameter refactoring
These documents provide context and analysis for the parameter order refactoring work: - PARAMETER_ORDER_ANALYSIS.md: Detailed analysis of the parameter order issue and rationale for reverting to original order - REVIEW_CHANGES_SUMMARY.md: Summary of code review feedback that was addressed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 637e5e7 commit 5a47ae6

File tree

2 files changed

+400
-0
lines changed

2 files changed

+400
-0
lines changed

PARAMETER_ORDER_ANALYSIS.md

Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,240 @@
1+
# Parameter Order Analysis: consoleReplay() Refactoring
2+
3+
## Executive Summary
4+
5+
**RECOMMENDATION: REVERT THE PARAMETER ORDER CHANGE**
6+
7+
The parameter reordering from `(customConsoleHistory, numberOfMessagesToSkip)` to `(numberOfMessagesToSkip, customConsoleHistory)` was done to satisfy an ESLint rule, but it makes the API **significantly worse** for the vast majority of use cases.
8+
9+
## Original vs New Signatures
10+
11+
### OLD (before commit 328cbde1c):
12+
13+
```typescript
14+
function consoleReplay(
15+
customConsoleHistory: Console['history'] | undefined = undefined,
16+
numberOfMessagesToSkip: number = 0,
17+
): string;
18+
```
19+
20+
### NEW (current):
21+
22+
```typescript
23+
function consoleReplay(
24+
numberOfMessagesToSkip = 0,
25+
customConsoleHistory: Console['history'] | undefined = undefined,
26+
): string;
27+
```
28+
29+
## ALL Usage Patterns Found
30+
31+
### 1. Open Source Package: serverRenderReactComponent.ts
32+
33+
**OLD code** (3 call sites):
34+
35+
```typescript
36+
consoleReplay(consoleHistory); // 2 times - passing custom history
37+
consoleReplay(); // 1 time - using global history
38+
```
39+
40+
**NEW code** (3 call sites):
41+
42+
```typescript
43+
buildConsoleReplay(0, consoleHistory); // 2 times
44+
buildConsoleReplay(); // 1 time
45+
```
46+
47+
### 2. Pro Package: streamingUtils.ts
48+
49+
**OLD code** (1 call site):
50+
51+
```typescript
52+
buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
53+
```
54+
55+
**NEW code** (1 call site):
56+
57+
```typescript
58+
consoleReplay(previouslyReplayedConsoleMessages, consoleHistory);
59+
```
60+
61+
### 3. Client API: base/client.ts
62+
63+
**OLD code** (1 call site):
64+
65+
```typescript
66+
consoleReplay(); // Export for client-side use
67+
```
68+
69+
**NEW code** (1 call site):
70+
71+
```typescript
72+
consoleReplay(); // No change - still works
73+
```
74+
75+
### 4. Internal: buildConsoleReplay.ts
76+
77+
**OLD code**:
78+
79+
```typescript
80+
// buildConsoleReplay() wraps consoleReplay()
81+
const consoleReplayJS = consoleReplay(customConsoleHistory, numberOfMessagesToSkip);
82+
```
83+
84+
**NEW code**:
85+
86+
```typescript
87+
const consoleReplayJS = consoleReplay(numberOfMessagesToSkip, customConsoleHistory);
88+
```
89+
90+
## Analysis of Common Usage Patterns
91+
92+
### Pattern Frequency (OLD code):
93+
94+
1. **Pass only custom history** (most common): `consoleReplay(consoleHistory)`
95+
96+
- serverRenderReactComponent.ts: 2 times
97+
- Pro streamingUtils.ts: 1 time (via buildConsoleReplay)
98+
- **Total: 3 call sites**
99+
100+
2. **Use defaults** (second most common): `consoleReplay()`
101+
102+
- serverRenderReactComponent.ts: 1 time
103+
- base/client.ts: 1 time
104+
- **Total: 2 call sites**
105+
106+
3. **Pass both parameters** (rare): `buildConsoleReplay(consoleHistory, numberOfMessagesToSkip)`
107+
- Pro streamingUtils.ts: 1 time
108+
- **Total: 1 call site**
109+
110+
### Pattern Frequency (NEW code):
111+
112+
1. **Pass both parameters**: `buildConsoleReplay(0, consoleHistory)` or `consoleReplay(0, consoleHistory)`
113+
114+
- Open source: 2 times
115+
- Pro: 1 time
116+
- **Total: 3 call sites**
117+
118+
2. **Use defaults**: `buildConsoleReplay()` or `consoleReplay()`
119+
120+
- Open source: 1 time
121+
- Client API: 1 time
122+
- **Total: 2 call sites**
123+
124+
3. **Skip count only** (NEW capability): `consoleReplay(2)`
125+
- **Total: 0 call sites (not actually used!)**
126+
127+
## The Problem
128+
129+
### OLD API Benefits:
130+
131+
```typescript
132+
consoleReplay(consoleHistory); // ✅ Clean - most common use case
133+
consoleReplay(); // ✅ Clean - second most common
134+
consoleReplay(consoleHistory, 2); // ✅ Rare but readable
135+
```
136+
137+
### NEW API Problems:
138+
139+
```typescript
140+
consoleReplay(0, consoleHistory); // ❌ UGLY - requires passing 0 for most common case!
141+
consoleReplay(); // ✅ Still clean
142+
consoleReplay(2); // ✅ Clean BUT NEVER ACTUALLY USED
143+
```
144+
145+
## The ESLint Rule
146+
147+
The change was made to satisfy:
148+
149+
```
150+
ESLint: @typescript-eslint/default-param-last
151+
"Default parameters should be last"
152+
```
153+
154+
This rule exists because:
155+
156+
- It prevents confusion when calling functions with positional arguments
157+
- Default params after non-default params can create weird calling patterns
158+
159+
**BUT** the rule assumes you're frequently passing the first param and skipping the second. In our case:
160+
161+
- We **rarely** pass `numberOfMessagesToSkip` alone
162+
- We **frequently** pass `customConsoleHistory` alone
163+
- The old order was more natural for our usage patterns
164+
165+
## Impact on Code Quality
166+
167+
### Before (natural):
168+
169+
```typescript
170+
// Common case: pass custom history
171+
const script = consoleReplay(consoleHistory);
172+
173+
// Rare case: skip messages
174+
const script = consoleReplay(consoleHistory, 5);
175+
```
176+
177+
### After (forced 0's everywhere):
178+
179+
```typescript
180+
// Common case: NOW REQUIRES passing 0!
181+
const script = consoleReplay(0, consoleHistory); // ❌ UGLY!
182+
183+
// Rare case: slightly cleaner IF you're only skipping
184+
const script = consoleReplay(5); // ✅ But this never happens!
185+
```
186+
187+
## Recommendations
188+
189+
### Option 1: REVERT THE PARAMETER ORDER (RECOMMENDED)
190+
191+
```typescript
192+
// Go back to the original, more natural ordering
193+
function consoleReplay(customConsoleHistory?: Console['history'], numberOfMessagesToSkip = 0): string;
194+
```
195+
196+
Then disable the ESLint rule for this function:
197+
198+
```typescript
199+
// eslint-disable-next-line @typescript-eslint/default-param-last
200+
export function consoleReplay(
201+
```
202+
203+
**Why:** The original order matches actual usage patterns 100% better.
204+
205+
### Option 2: Use Options Object (BETTER LONG-TERM)
206+
207+
```typescript
208+
interface ConsoleReplayOptions {
209+
consoleHistory?: Console['history'];
210+
skip?: number;
211+
}
212+
213+
function consoleReplay(options: ConsoleReplayOptions = {}): string {
214+
const { consoleHistory = console.history, skip = 0 } = options;
215+
// ...
216+
}
217+
```
218+
219+
**Usage:**
220+
221+
```typescript
222+
consoleReplay({ consoleHistory }); // ✅ Clear
223+
consoleReplay({ skip: 2 }); // ✅ Clear
224+
consoleReplay({ consoleHistory, skip: 2 }); // ✅ Clear
225+
consoleReplay(); // ✅ Still works
226+
```
227+
228+
**Why:** Most flexible, self-documenting, future-proof. But requires updating all call sites.
229+
230+
### Option 3: Keep Current Order (NOT RECOMMENDED)
231+
232+
Only if you absolutely must satisfy ESLint with no exceptions.
233+
234+
**Cost:** Every common use case requires `consoleReplay(0, consoleHistory)` which is less readable.
235+
236+
## Conclusion
237+
238+
The parameter reordering was done for linting compliance, but it **degraded API ergonomics** for the majority of actual usage. The ESLint rule is valuable in general, but this is a case where the rule doesn't fit the domain.
239+
240+
**STRONG RECOMMENDATION:** Revert to the original parameter order and disable the ESLint rule for this function with a comment explaining why.

0 commit comments

Comments
 (0)