Skip to content

Commit a584418

Browse files
mathpiratemathpirate
authored
fix ct-1035 by correctly transforming map inside ifelse (commontoolsinc#2063)
* fix ct-1035 by correctly transforming map inside ifelse
1 parent 6944710 commit a584418

File tree

4 files changed

+245
-8
lines changed

4 files changed

+245
-8
lines changed

‎packages/ts-transformers/src/closures/transformer.ts‎

Lines changed: 57 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -853,6 +853,28 @@ function findClosestJsxExpression(
853853
return undefined;
854854
}
855855

856+
/**
857+
* Helper to check if a node is contained within a specific argument of a call expression.
858+
*/
859+
function isWithinArgument(
860+
node: ts.Node,
861+
callExpr: ts.CallExpression,
862+
argIndex: number,
863+
): boolean {
864+
let current: ts.Node | undefined = node;
865+
while (current && current !== callExpr) {
866+
if (
867+
ts.isCallExpression(current.parent) &&
868+
current.parent === callExpr &&
869+
current.parent.arguments[argIndex] === current
870+
) {
871+
return true;
872+
}
873+
current = current.parent;
874+
}
875+
return false;
876+
}
877+
856878
/**
857879
* Check if a map call will be wrapped in a derive by the JSX transformer.
858880
* Returns true if wrapping will occur (meaning we should NOT transform the map).
@@ -874,6 +896,22 @@ function willBeWrappedByJsx(
874896
// Only check THIS expression for derive wrapping
875897
if (closestJsxExpression.expression !== mapCall) {
876898
const analysis = analyze(closestJsxExpression.expression);
899+
900+
// Special case: ifElse calls only wrap the predicate (first argument),
901+
// not the branches (second and third arguments)
902+
if (
903+
analysis.rewriteHint?.kind === "call-if-else" &&
904+
ts.isCallExpression(closestJsxExpression.expression)
905+
) {
906+
// If map is in the predicate (arg 0), it will be wrapped
907+
// If map is in the branches (args 1 or 2), it won't be wrapped
908+
return isWithinArgument(
909+
mapCall,
910+
closestJsxExpression.expression,
911+
0,
912+
);
913+
}
914+
877915
// Check if this will be wrapped in a derive (not just transformed in some other way)
878916
// Array-map calls have skip-call-rewrite hint, so they won't be wrapped in derive
879917
const willBeWrappedInDerive = analysis.requiresRewrite &&
@@ -889,12 +927,25 @@ function willBeWrappedByJsx(
889927
while (node) {
890928
if (ts.isJsxExpression(node) && node.expression) {
891929
const analysis = analyze(node.expression);
892-
const willBeWrappedInDerive = analysis.requiresRewrite &&
893-
!(analysis.rewriteHint?.kind === "skip-call-rewrite" &&
894-
analysis.rewriteHint.reason === "array-map");
895-
if (willBeWrappedInDerive) {
896-
// An ancestor JSX expression will wrap this in a derive
897-
return true;
930+
931+
// Special case: ifElse calls only wrap the predicate
932+
if (
933+
analysis.rewriteHint?.kind === "call-if-else" &&
934+
ts.isCallExpression(node.expression)
935+
) {
936+
// If map is in the predicate, it will be wrapped
937+
if (isWithinArgument(mapCall, node.expression, 0)) {
938+
return true;
939+
}
940+
// If map is in the branches, continue checking ancestors
941+
} else {
942+
const willBeWrappedInDerive = analysis.requiresRewrite &&
943+
!(analysis.rewriteHint?.kind === "skip-call-rewrite" &&
944+
analysis.rewriteHint.reason === "array-map");
945+
if (willBeWrappedInDerive) {
946+
// An ancestor JSX expression will wrap this in a derive
947+
return true;
948+
}
898949
}
899950
}
900951
node = node.parent;

‎packages/ts-transformers/test/fixtures/closures/map-inside-ifelse-with-handler.expected.tsx‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
import * as __ctHelpers from "commontools";
2+
import { Cell, handler, ifElse, recipe, UI } from "commontools";
3+
interface Item {
4+
id: number;
5+
name: string;
6+
}
7+
// Handler that closes over both items array and individual item
8+
const removeItem = handler(true as const satisfies __ctHelpers.JSONSchema, {
9+
type: "object",
10+
properties: {
11+
items: {
12+
type: "array",
13+
items: {
14+
$ref: "#/$defs/Item",
15+
asCell: true
16+
},
17+
asCell: true
18+
},
19+
item: {
20+
$ref: "#/$defs/Item",
21+
asCell: true
22+
}
23+
},
24+
required: ["items", "item"],
25+
$defs: {
26+
Item: {
27+
type: "object",
28+
properties: {
29+
id: {
30+
type: "number"
31+
},
32+
name: {
33+
type: "string"
34+
}
35+
},
36+
required: ["id", "name"]
37+
}
38+
}
39+
} as const satisfies __ctHelpers.JSONSchema, (_event, { items, item }) => {
40+
const currentItems = items.get();
41+
const index = currentItems.findIndex((el) => el.equals(item));
42+
if (index >= 0) {
43+
items.set(currentItems.toSpliced(index, 1));
44+
}
45+
});
46+
export default recipe({
47+
type: "object",
48+
properties: {
49+
items: {
50+
type: "array",
51+
items: {
52+
$ref: "#/$defs/Item"
53+
}
54+
},
55+
hasItems: {
56+
type: "boolean"
57+
}
58+
},
59+
required: ["items", "hasItems"],
60+
$defs: {
61+
Item: {
62+
type: "object",
63+
properties: {
64+
id: {
65+
type: "number"
66+
},
67+
name: {
68+
type: "string"
69+
}
70+
},
71+
required: ["id", "name"]
72+
}
73+
}
74+
} as const satisfies __ctHelpers.JSONSchema, ({ items, hasItems }) => {
75+
// CT-1035: Map inside ifElse branches should transform to mapWithPattern
76+
// The handler closure should work correctly with the map iterator variable
77+
return {
78+
[UI]: (<div>
79+
{ifElse(hasItems, <div>
80+
{items.mapWithPattern(__ctHelpers.recipe({
81+
type: "object",
82+
properties: {
83+
element: {
84+
$ref: "#/$defs/Item"
85+
},
86+
params: {
87+
type: "object",
88+
properties: {
89+
items: {
90+
type: "array",
91+
items: {
92+
$ref: "#/$defs/Item"
93+
},
94+
asOpaque: true
95+
}
96+
},
97+
required: ["items"]
98+
}
99+
},
100+
required: ["element", "params"],
101+
$defs: {
102+
Item: {
103+
type: "object",
104+
properties: {
105+
id: {
106+
type: "number"
107+
},
108+
name: {
109+
type: "string"
110+
}
111+
},
112+
required: ["id", "name"]
113+
}
114+
}
115+
} as const satisfies __ctHelpers.JSONSchema, ({ element: item, params: { items } }) => (<div>
116+
<span>{item.name}</span>
117+
<button type="button" onClick={removeItem({ items, item })}>Remove</button>
118+
</div>)), {
119+
items: items
120+
})}
121+
</div>, <div>No items</div>)}
122+
</div>),
123+
};
124+
});
125+
// @ts-ignore: Internals
126+
function h(...args: any[]) { return __ctHelpers.h.apply(null, args); }
127+
// @ts-ignore: Internals
128+
h.fragment = __ctHelpers.h.fragment;

‎packages/ts-transformers/test/fixtures/closures/map-inside-ifelse-with-handler.input.tsx‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/// <cts-enable />
2+
import { Cell, handler, ifElse, recipe, UI } from "commontools";
3+
4+
interface Item {
5+
id: number;
6+
name: string;
7+
}
8+
9+
// Handler that closes over both items array and individual item
10+
const removeItem = handler<
11+
unknown,
12+
{ items: Cell<Array<Cell<Item>>>; item: Cell<Item> }
13+
>((_event, { items, item }) => {
14+
const currentItems = items.get();
15+
const index = currentItems.findIndex((el) => el.equals(item));
16+
if (index >= 0) {
17+
items.set(currentItems.toSpliced(index, 1));
18+
}
19+
});
20+
21+
export default recipe<{ items: Item[]; hasItems: boolean }>(
22+
"CT-1035: Map with handler inside ifElse",
23+
({ items, hasItems }) => {
24+
// CT-1035: Map inside ifElse branches should transform to mapWithPattern
25+
// The handler closure should work correctly with the map iterator variable
26+
return {
27+
[UI]: (
28+
<div>
29+
{ifElse(
30+
hasItems,
31+
<div>
32+
{items.map((item) => (
33+
<div>
34+
<span>{item.name}</span>
35+
<button type="button" onClick={removeItem({ items, item })}>Remove</button>
36+
</div>
37+
))}
38+
</div>,
39+
<div>No items</div>
40+
)}
41+
</div>
42+
),
43+
};
44+
},
45+
);

‎packages/ts-transformers/test/fixtures/jsx-expressions/opaque-ref-cell-map.expected.tsx‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,25 @@ export default recipe("Charms Launcher", () => {
101101
[UI]: (<div>
102102
<h3>Stored Charms:</h3>
103103
{ifElse(__ctHelpers.derive({ typedCellRef: typedCellRef }, ({ typedCellRef }) => !typedCellRef?.length), <div>No charms created yet</div>, <ul>
104-
{typedCellRef.map((charm: any, index: number) => (<li>
104+
{typedCellRef.mapWithPattern(__ctHelpers.recipe({
105+
type: "object",
106+
properties: {
107+
element: true,
108+
index: {
109+
type: "number"
110+
},
111+
params: {
112+
type: "object",
113+
properties: {}
114+
}
115+
},
116+
required: ["element", "params"]
117+
} as const satisfies __ctHelpers.JSONSchema, ({ element: charm, index: index, params: {} }) => (<li>
105118
<ct-button onClick={goToCharm({ charm })}>
106119
Go to Charm {__ctHelpers.derive({ index: index }, ({ index }) => index + 1)}
107120
</ct-button>
108121
<span>Charm {__ctHelpers.derive({ index: index }, ({ index }) => index + 1)}: {__ctHelpers.derive({ charm: charm }, ({ charm }) => charm[NAME] || "Unnamed")}</span>
109-
</li>))}
122+
</li>)), {})}
110123
</ul>)}
111124

112125
<ct-button onClick={createSimpleRecipe({ cellRef })}>

0 commit comments

Comments
 (0)