Skip to content

Commit a91fb94

Browse files
fix(types): relax variant behavior on incompatible props (#97)
* fix(types): relax variant behavior on incompatible props `variant` blocking on "incompatible props" use-case is bad for DX it only protects against bad spreads, which are antipattern anyway * Support more edge-cases * Update type tests
1 parent 56e8e0d commit a91fb94

File tree

2 files changed

+126
-13
lines changed

2 files changed

+126
-13
lines changed

public-types/reflect.d.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,22 @@ export function list<
184184

185185
// variant types
186186

187+
/**
188+
* Computes final props type based on Props of the view component and Bind object for variant operator specifically
189+
*
190+
* Difference is important since in variant case Props is a union
191+
*
192+
* Props that are "taken" by Bind object are made **optional** in the final type,
193+
* so it is possible to overwrite them in the component usage anyway
194+
*/
195+
type FinalPropsVariant<Props, Bind extends BindFromProps<Props>> = Show<
196+
Props extends any
197+
? Omit<Props, keyof Bind> & {
198+
[K in Extract<keyof Bind, keyof Props>]?: Props[K];
199+
}
200+
: never
201+
>;
202+
187203
/**
188204
* Operator to conditionally render a component based on the reactive `source` store value.
189205
*
@@ -206,8 +222,9 @@ export function list<
206222
* ```
207223
*/
208224
export function variant<
209-
Props,
210225
CaseType extends string,
226+
Cases extends Record<CaseType, ComponentType<any>>,
227+
Props extends ComponentProps<Cases[CaseType]>,
211228
// It is ok here - it fixed bunch of type inference issues, when `bind` is not provided
212229
// but it is not clear why it works this way - Record<string, never> or any option other than `{}` doesn't work
213230
// eslint-disable-next-line @typescript-eslint/ban-types
@@ -216,7 +233,7 @@ export function variant<
216233
config:
217234
| {
218235
source: Store<CaseType>;
219-
cases: Partial<Record<CaseType, ComponentType<Props>>>;
236+
cases: Partial<Cases>;
220237
default?: ComponentType<Props>;
221238
bind?: Bind;
222239
hooks?: Hooks<Props>;
@@ -236,7 +253,7 @@ export function variant<
236253
*/
237254
useUnitConfig?: UseUnitConfig;
238255
},
239-
): FC<FinalProps<Props, Bind>>;
256+
): FC<FinalPropsVariant<Props, Bind>>;
240257

241258
// fromTag types
242259
/**

type-tests/types-variant.tsx

Lines changed: 106 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,10 @@ import { expectType } from 'tsd';
3131
},
3232
});
3333

34-
expectType<React.FC>(VariableInput);
34+
<VariableInput />;
3535
}
3636

37-
// variant catches incompatible props between cases
37+
// variant allows to pass incompatible props between cases - resulting component will have union of all props from all cases
3838
{
3939
const Input: React.FC<{
4040
value: string;
@@ -56,12 +56,38 @@ import { expectType } from 'tsd';
5656
},
5757
cases: {
5858
input: Input,
59-
// @ts-expect-error
6059
datetime: DateTime,
6160
},
6261
});
6362

64-
expectType<React.FC>(VariableInput);
63+
<VariableInput />;
64+
<VariableInput value="test" />;
65+
<VariableInput
66+
value="test"
67+
onChange={(event: { target: { value: string } }) => {
68+
event.target.value;
69+
}}
70+
/>;
71+
<VariableInput
72+
value="test"
73+
onChange={() => {
74+
// ok
75+
}}
76+
/>;
77+
<VariableInput
78+
value="test"
79+
onChange={(event: string) => {
80+
event;
81+
}}
82+
/>;
83+
<VariableInput
84+
// @ts-expect-error
85+
value={42}
86+
// @ts-expect-error
87+
onChange={(event: number) => {
88+
event;
89+
}}
90+
/>;
6591
}
6692

6793
// variant allows not to set every possble case
@@ -89,7 +115,10 @@ import { expectType } from 'tsd';
89115
default: NotFoundPage,
90116
});
91117

92-
expectType<React.FC>(CurrentPage);
118+
<CurrentPage />;
119+
<CurrentPage context={{ route: 'home' }} />;
120+
// @ts-expect-error
121+
<CurrentPage context="kek" />;
93122
}
94123

95124
// variant warns about wrong cases
@@ -117,7 +146,10 @@ import { expectType } from 'tsd';
117146
default: NotFoundPage,
118147
});
119148

120-
expectType<React.FC>(CurrentPage);
149+
<CurrentPage />;
150+
<CurrentPage context={{ route: 'home' }} />;
151+
// @ts-expect-error
152+
<CurrentPage context="kek" />;
121153
}
122154

123155
// overload for boolean source
@@ -140,14 +172,21 @@ import { expectType } from 'tsd';
140172
else: FallbackPage,
141173
bind: { context: $ctx },
142174
});
143-
expectType<React.FC>(CurrentPageThenElse);
175+
176+
<CurrentPageThenElse />;
177+
<CurrentPageThenElse context={{ route: 'home' }} />;
178+
// @ts-expect-error
179+
<CurrentPageThenElse context="kek" />;
144180

145181
const CurrentPageOnlyThen = variant({
146182
if: $enabled,
147183
then: HomePage,
148184
bind: { context: $ctx },
149185
});
150-
expectType<React.FC>(CurrentPageOnlyThen);
186+
<CurrentPageOnlyThen />;
187+
<CurrentPageOnlyThen context={{ route: 'home' }} />;
188+
// @ts-expect-error
189+
<CurrentPageOnlyThen context="kek" />;
151190
}
152191

153192
// supports nesting
@@ -169,6 +208,8 @@ import { expectType } from 'tsd';
169208
}),
170209
},
171210
});
211+
212+
<NestedVariant />;
172213
}
173214

174215
// allows variants of compatible types
@@ -188,6 +229,10 @@ import { expectType } from 'tsd';
188229
}),
189230
else: Loader,
190231
});
232+
233+
<View test="test" />;
234+
// @ts-expect-error
235+
<View test={42} />;
191236
}
192237

193238
// Issue #81 reproduce 1
@@ -264,7 +309,6 @@ import { expectType } from 'tsd';
264309
},
265310
cases: {
266311
button: Button<'button'>,
267-
// @ts-expect-error
268312
a: Button<'a'>,
269313
},
270314
});
@@ -277,15 +321,67 @@ import { expectType } from 'tsd';
277321
},
278322
cases: {
279323
button: Button<'button'>,
280-
// @ts-expect-error
281324
a: Button<'a'>,
282325
},
283326
});
284327

328+
<ReflectedVariantBad />;
329+
<ReflectedVariantBad size="xl" />;
330+
// @ts-expect-error
331+
<ReflectedVariantBad size={52} />;
332+
285333
const IfElseVariant = variant({
286334
if: createStore(true),
287335
then: Button<'button'>,
288336
// @ts-expect-error
289337
else: Button<'a'>,
290338
});
339+
340+
<IfElseVariant />;
341+
<IfElseVariant size="xl" />;
342+
// @ts-expect-error
343+
<IfElseVariant size={52} />;
344+
}
345+
346+
// variant should allow not-to pass required props - as they can be added later in react
347+
{
348+
const Input: React.FC<{
349+
value: string;
350+
onChange: (newValue: string) => void;
351+
color: 'red';
352+
}> = () => null;
353+
const $variants = createStore<'input' | 'fallback'>('input');
354+
const Fallback: React.FC<{ kek?: string }> = () => null;
355+
const $value = createStore<string>('');
356+
const changed = createEvent<string>();
357+
358+
const InputBase = reflect({
359+
view: Input,
360+
bind: {
361+
value: $value,
362+
onChange: changed,
363+
},
364+
});
365+
366+
const ReflectedInput = variant({
367+
source: $variants,
368+
cases: {
369+
input: InputBase,
370+
fallback: Fallback,
371+
},
372+
});
373+
374+
const App: React.FC = () => {
375+
// missing prop must still be required in react
376+
// but in this case it is not required, as props are conditional union
377+
return <ReflectedInput />;
378+
};
379+
380+
<ReflectedInput kek="kek" />;
381+
382+
const AppFixed: React.FC = () => {
383+
return <ReflectedInput color="red" />;
384+
};
385+
expectType<React.FC>(App);
386+
expectType<React.FC>(AppFixed);
291387
}

0 commit comments

Comments
 (0)