Skip to content

Commit 7280182

Browse files
fix: address mapProps review points (#1-#4, B1, B2)
Follow-up to PR #102 (mapProps feature). Addresses all review points: #1 — Runtime tests added for variant, list, createReflect (no-ssr + ssr). Previously only reflect had mapProps coverage. #2 — Unknown mapProps keys now error at the key site itself, not on fn's return. MapPropsFromSources resolves unknown-key entries to never, producing a TS2322 at the key line. Replaces the old 'K extends keyof Props ? Props[K] : never' fn-return approach that had a silent-failure path (never-returning fn compiled cleanly). #3 — Documented the Store<any> variable-source widening limitation. Type tests (3a positive, 3b widening) pin the current behavior. #4 — fn is skipped when its key is overridden by an external prop. 'if (key in props) continue' in src/core/reflect.ts. Spy assertions in reflect and createReflect tests verify fn is not called. B1 — bind + mapProps key collision is now a type error. MapPropsFromSources takes Bind as a type parameter; a key in both bind and mapProps resolves to never. Runtime skip ('if (key in storeProps) continue') is a defense-in-depth for JS/type-bypass scenarios (covers stores; events/data/functions are covered by the type fix). B2 — mapItem + mapProps key collision in list is a type error. MapItem's mapped type now omits keyof Sources alongside keyof Bind. Partial: bypassable with explicit item-parameter annotation (known TS limitation with mapped-type extends constraints), documented in the type-test comment. All four operators (reflect, createReflect, list, variant) are covered by the type fixes and runtime tests. Docs updated in docs/pages/docs/reflect.mdx: - mapProps keys must be props of the view; unknown keys error at the key - a key must not appear in both bind and mapProps - in list, a key must not appear in both mapItem and mapProps - source should be an inline literal for best inference - fn is not invoked when its key is overridden 80 runtime tests + type tests pass.
1 parent cdcbe32 commit 7280182

12 files changed

Lines changed: 463 additions & 32 deletions

File tree

docs/pages/docs/reflect.mdx

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,8 @@ Each entry is `{ source, fn }`:
9595
- an **array** of stores — `value` is the resolved tuple (`[Store<A>, Store<B>]``[A, B]`).
9696
- `fn``(value, props) => derivedProp`, where `value` is the resolved `source` value (its type is inferred — no annotation needed) and `props` are the component's own props.
9797

98+
> Each key in `mapProps` must be a prop of the `view` — a typo'd or unknown key is a type error at the key itself. A key must not appear in both `bind` and `mapProps` — this is a type error. For best type inference, pass `source` as an inline literal; a variable typed as `Store<any>` will widen `fn`'s `value` argument to `any`.
99+
98100
```tsx
99101
import { reflect } from '@effector/reflect';
100102
import { createStore } from 'effector';
@@ -143,9 +145,10 @@ const Hello = reflect({
143145

144146
The component re-renders only when the `source` changes. A prop computed via `mapProps`
145147
is made **optional** in the resulting component's type and can still be overridden explicitly at
146-
the usage site (an explicitly passed prop wins over the derived value).
148+
the usage site (an explicitly passed prop wins over the derived value — in that case `fn` is
149+
not invoked for the overridden key).
147150

148-
> Note: like `bind`, the `mapProps` field is supported by all Reflect operators — `reflect`, `createReflect`, `variant` and `list`.
151+
> Note: like `bind`, the `mapProps` field is supported by all Reflect operators — `reflect`, `createReflect`, `variant` and `list`. In `list`, a key must not appear in both `mapItem` and `mapProps``mapItem` automatically omits keys that are derived via `mapProps`.
149152
150153
### Fork API auto-compatibility
151154

public-types/reflect.d.ts

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -75,16 +75,23 @@ type SourceValue<S> = S extends Store<infer V>
7575
* `Sources` is a separate generic that captures the `source` of every entry. Because the
7676
* sources are inferred independently of the `fn`s, `fn`'s `value` argument is inferred as the
7777
* resolved source value (no manual annotation needed), and `props` is the view's `Props`.
78-
* `fn` must return the prop type - a key that is not a prop of the view resolves to `never`.
78+
* Each key in `mapProps` must be a prop of the `view` - a typo'd or unknown key resolves its
79+
* entry to `never`, so the object literal assigned to it is a type error at the key site.
80+
* A key that is also present in `bind` resolves to `never` as well — a prop must not be
81+
* both bound and derived.
7982
*/
80-
type MapPropsFromSources<Props, Sources extends Record<string, SourceShape>> = {
81-
[K in keyof Sources]: {
82-
source: Sources[K];
83-
fn: (
84-
value: SourceValue<Sources[K]>,
85-
props: Props,
86-
) => K extends keyof Props ? Props[K] : never;
87-
};
83+
type MapPropsFromSources<Props, Bind, Sources extends Record<string, SourceShape>> = {
84+
[K in keyof Sources]: K extends keyof Props
85+
? K extends keyof Bind
86+
? never
87+
: {
88+
source: Sources[K];
89+
fn: (
90+
value: SourceValue<Sources[K]>,
91+
props: Props,
92+
) => Props[K & keyof Props];
93+
}
94+
: never;
8895
};
8996

9097
/**
@@ -144,7 +151,7 @@ export function reflect<
144151
/**
145152
* Derives props for the `view` from a store value combined with the component's props.
146153
*/
147-
mapProps?: MapPropsFromSources<Props, Sources>;
154+
mapProps?: MapPropsFromSources<Props, Bind, Sources>;
148155
hooks?: Hooks<Props>;
149156
/**
150157
* This configuration is passed directly to `useUnit`'s hook second argument.
@@ -187,7 +194,7 @@ export function createReflect<
187194
/**
188195
* Derives props for the `view` from a store value combined with the component's props.
189196
*/
190-
mapProps?: MapPropsFromSources<Props, Sources>;
197+
mapProps?: MapPropsFromSources<Props, Bind, Sources>;
191198
hooks?: Hooks<Props>;
192199
/**
193200
* This configuration is passed directly to `useUnit`'s hook second argument.
@@ -226,7 +233,10 @@ export function list<
226233
Props extends ComponentProps<View>,
227234
Item,
228235
MapItem extends {
229-
[M in keyof Omit<Props, keyof Bind>]: (item: Item, index: number) => Props[M];
236+
[M in keyof Omit<Props, keyof Bind | keyof Sources>]: (
237+
item: Item,
238+
index: number,
239+
) => Props[M];
230240
},
231241
Bind extends BindFromProps<Props> = object,
232242
// eslint-disable-next-line @typescript-eslint/ban-types
@@ -238,7 +248,7 @@ export function list<
238248
view: View;
239249
bind?: Bind;
240250
mapItem?: MapItem;
241-
mapProps?: MapPropsFromSources<Props, Sources>;
251+
mapProps?: MapPropsFromSources<Props, Bind, Sources>;
242252
getKey?: (item: Item) => React.Key;
243253
hooks?: Hooks<Props>;
244254
/**
@@ -251,7 +261,7 @@ export function list<
251261
view: View;
252262
bind?: Bind;
253263
mapItem: MapItem;
254-
mapProps?: MapPropsFromSources<Props, Sources>;
264+
mapProps?: MapPropsFromSources<Props, Bind, Sources>;
255265
getKey?: (item: Item) => React.Key;
256266
hooks?: Hooks<Props>;
257267
/**
@@ -321,7 +331,7 @@ export function variant<
321331
cases: Partial<Cases>;
322332
default?: ComponentType<Props>;
323333
bind?: Bind;
324-
mapProps?: MapPropsFromSources<Props, Sources>;
334+
mapProps?: MapPropsFromSources<Props, Bind, Sources>;
325335
hooks?: Hooks<Props>;
326336
/**
327337
* This configuration is passed directly to `useUnit`'s hook second argument.
@@ -333,7 +343,7 @@ export function variant<
333343
then: ComponentType<Props>;
334344
else?: ComponentType<Props>;
335345
bind?: Bind;
336-
mapProps?: MapPropsFromSources<Props, Sources>;
346+
mapProps?: MapPropsFromSources<Props, Bind, Sources>;
337347
hooks?: Hooks<Props>;
338348
/**
339349
* This configuration is passed directly to `useUnit`'s hook second argument.

src/core/reflect.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,9 @@ export function reflectFactory(context: Context) {
7979
);
8080

8181
for (const key of mapPropsKeys) {
82+
// an explicitly passed prop or a bound store wins over the derived one — skip fn
83+
if (key in props) continue;
84+
if (key in storeProps) continue;
8285
mappedProps[key] = (mapProps as any)[key].fn(
8386
(mapPropsValues as any)[key],
8487
propsForFn,

src/no-ssr/create-reflect.test.tsx

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,3 +229,53 @@ describe('useUnitConfig', () => {
229229
);
230230
});
231231
});
232+
233+
describe('mapProps', () => {
234+
const Greeting: FC<{ testId: string; label: string }> = (props) => {
235+
return <span data-testid={props.testId}>{props.label}</span>;
236+
};
237+
const greetingReflect = createReflect(Greeting);
238+
239+
test('derives a prop in createReflect via mapProps', async () => {
240+
const setName = createEvent<string>();
241+
const $name = restore(setName, 'Bob');
242+
243+
const Hello = greetingReflect(
244+
{},
245+
{
246+
mapProps: {
247+
label: {
248+
source: $name,
249+
fn: (name, props: { greeting: string }) => `${props.greeting}, ${name}!`,
250+
},
251+
},
252+
},
253+
);
254+
255+
const container = render(<Hello testId="hello" greeting="Hi" />);
256+
expect(container.getByTestId('hello').textContent).toBe('Hi, Bob!');
257+
258+
await act(async () => {
259+
setName('Alice');
260+
});
261+
expect(container.getByTestId('hello').textContent).toBe('Hi, Alice!');
262+
});
263+
264+
test('explicitly passed prop wins over the derived one and fn is skipped', () => {
265+
const $name = createStore('Bob');
266+
const fn = vi.fn((name: string) => `Hello, ${name}!`);
267+
268+
const Hello = greetingReflect(
269+
{},
270+
{
271+
mapProps: {
272+
label: { source: $name, fn },
273+
},
274+
},
275+
);
276+
277+
const container = render(<Hello testId="hello" label="overridden" />);
278+
expect(container.getByTestId('hello').textContent).toBe('overridden');
279+
expect(fn).not.toHaveBeenCalled();
280+
});
281+
});

src/no-ssr/list.test.tsx

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,13 @@
11
import { list } from '@effector/reflect';
22
import { render } from '@testing-library/react';
3-
import { allSettled, createEffect, createEvent, createStore, fork } from 'effector';
3+
import {
4+
allSettled,
5+
createEffect,
6+
createEvent,
7+
createStore,
8+
fork,
9+
restore,
10+
} from 'effector';
411
import { Provider, useStore } from 'effector-react';
512
import React, { FC, memo } from 'react';
613
import { act } from 'react-dom/test-utils';
@@ -565,3 +572,44 @@ describe('useUnitConfig', () => {
565572
);
566573
});
567574
});
575+
576+
describe('mapProps', () => {
577+
const Item: FC<{ testId: string; label?: string }> = (props) => {
578+
return <li data-testid={props.testId}>{props.label}</li>;
579+
};
580+
581+
test('derives a prop in list via mapProps using mapItem output', async () => {
582+
const setName = createEvent<string>();
583+
const $name = restore(setName, 'Bob');
584+
const $items = createStore([{ key: 'a' }, { key: 'b' }]);
585+
586+
const Items = list({
587+
source: $items,
588+
view: Item,
589+
bind: {},
590+
mapItem: {
591+
testId: (item) => item.key,
592+
},
593+
mapProps: {
594+
label: {
595+
source: $name,
596+
fn: (name, props: { testId: string }) => `${props.testId}-${name}`,
597+
},
598+
},
599+
});
600+
601+
const container = render(
602+
<ul>
603+
<Items />
604+
</ul>,
605+
);
606+
expect(container.getByTestId('a').textContent).toBe('a-Bob');
607+
expect(container.getByTestId('b').textContent).toBe('b-Bob');
608+
609+
await act(async () => {
610+
setName('Alice');
611+
});
612+
expect(container.getByTestId('a').textContent).toBe('a-Alice');
613+
expect(container.getByTestId('b').textContent).toBe('b-Alice');
614+
});
615+
});

src/no-ssr/reflect.test.tsx

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -630,22 +630,42 @@ describe('mapProps', () => {
630630
expect(container.getByTestId('hello').textContent).toBe('Hi, Alice!');
631631
});
632632

633-
test('explicitly passed prop wins over the derived one', () => {
633+
test('explicitly passed prop wins over the derived one and fn is skipped', () => {
634634
const $name = createStore('Bob');
635+
const fn = vi.fn((name: string) => `Hello, ${name}!`);
635636

636637
const Hello = reflect({
637638
view: Greeting,
638639
bind: {},
639640
mapProps: {
640-
label: {
641-
source: $name,
642-
fn: (name) => `Hello, ${name}!`,
643-
},
641+
label: { source: $name, fn },
644642
},
645643
});
646644

647645
const container = render(<Hello testId="hello" label="overridden" />);
648646
expect(container.getByTestId('hello').textContent).toBe('overridden');
647+
expect(fn).not.toHaveBeenCalled();
648+
});
649+
650+
test('bound store wins over mapProps and fn is skipped', () => {
651+
const $a = createStore('from-bind');
652+
const $b = createStore('from-mapProps');
653+
const fn = vi.fn((b: string) => b);
654+
655+
// The type-level fix (B1) makes this a type error — a key can't be in
656+
// both bind and mapProps. We bypass it here to test the runtime skip,
657+
// which is a defense-in-depth for JS users and type-bypass scenarios.
658+
const Hello = reflect({
659+
view: Greeting,
660+
bind: { label: $a },
661+
mapProps: {
662+
label: { source: $b, fn },
663+
},
664+
} as any);
665+
666+
const container = render(<Hello testId="hello" />);
667+
expect(container.getByTestId('hello').textContent).toBe('from-bind');
668+
expect(fn).not.toHaveBeenCalled();
649669
});
650670

651671
test('combines an object of stores as source', async () => {

src/no-ssr/variant.test.tsx

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { variant } from '@effector/reflect';
22
import { act, render } from '@testing-library/react';
33
import userEvent from '@testing-library/user-event';
44
import { createEvent, createStore, restore } from 'effector';
5-
import React from 'react';
5+
import React, { FC } from 'react';
66

77
test('matches first', async () => {
88
const changeValue = createEvent<string>();
@@ -323,3 +323,52 @@ describe('useUnitConfig', () => {
323323
);
324324
});
325325
});
326+
327+
describe('mapProps', () => {
328+
const Greeting: FC<{ testId: string; label: string }> = (props) => {
329+
return <span data-testid={props.testId}>{props.label}</span>;
330+
};
331+
332+
test('derives a prop in variant via mapProps', async () => {
333+
const setName = createEvent<string>();
334+
const $name = restore(setName, 'Bob');
335+
const $type = createStore<'a' | 'b'>('a');
336+
337+
const Input = variant({
338+
source: $type,
339+
bind: {},
340+
cases: { a: Greeting, b: Greeting },
341+
mapProps: {
342+
label: {
343+
source: $name,
344+
fn: (name, props: { greeting: string }) => `${props.greeting}, ${name}!`,
345+
},
346+
},
347+
});
348+
349+
const container = render(<Input testId="hello" greeting="Hi" />);
350+
expect(container.getByTestId('hello').textContent).toBe('Hi, Bob!');
351+
352+
await act(async () => {
353+
setName('Alice');
354+
});
355+
expect(container.getByTestId('hello').textContent).toBe('Hi, Alice!');
356+
});
357+
358+
test('explicitly passed prop wins over the derived one', () => {
359+
const $name = createStore('Bob');
360+
const $type = createStore<'a' | 'b'>('a');
361+
362+
const Input = variant({
363+
source: $type,
364+
bind: {},
365+
cases: { a: Greeting, b: Greeting },
366+
mapProps: {
367+
label: { source: $name, fn: (name) => `Hello, ${name}!` },
368+
},
369+
});
370+
371+
const container = render(<Input testId="hello" label="overridden" />);
372+
expect(container.getByTestId('hello').textContent).toBe('overridden');
373+
});
374+
});

src/ssr/create-reflect.test.tsx

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,3 +154,39 @@ test('with ssr for client', async () => {
154154
const inputName = container.getByTestId('name') as HTMLInputElement;
155155
expect(inputName.value).toBe('Bob');
156156
});
157+
158+
test('mapProps derives a prop in createReflect from a scoped store', async () => {
159+
const app = createDomain();
160+
161+
const setName = app.createEvent<string>();
162+
const $name = restore(setName, 'Bob');
163+
164+
const Greeting: FC<{ testId: string; label: string }> = (props) => {
165+
return <span data-testid={props.testId}>{props.label}</span>;
166+
};
167+
const greetingReflect = createReflect(Greeting);
168+
169+
const Hello = greetingReflect(
170+
{},
171+
{
172+
mapProps: {
173+
label: {
174+
source: $name,
175+
fn: (name, props: { greeting: string }) => `${props.greeting}, ${name}!`,
176+
},
177+
},
178+
},
179+
);
180+
181+
const scope = fork(app, { values: [[$name, 'Alice']] });
182+
183+
const container = render(
184+
<Provider value={scope}>
185+
<Hello testId="hello" greeting="Hi" />
186+
</Provider>,
187+
);
188+
189+
expect(container.getByTestId('hello').textContent).toBe('Hi, Alice!');
190+
// global store is untouched
191+
expect($name.getState()).toBe('Bob');
192+
});

0 commit comments

Comments
 (0)