Skip to content

Commit 4e16ddb

Browse files
mathpirateclaude
mathpirate
and
claude
committed
refactor(ts-transformers): optimize when/unless based on right-side cost
Changes the decision logic for when to use `when`/`unless` helpers: - Previously based on whether LEFT side was a simple opaque ref access - Now based on whether RIGHT side is "expensive" (JSX or needs derive) This better captures the intent: when/unless provide short-circuit optimization that's most valuable when the right side is expensive to construct (like JSX) or when both sides need derive (for taint tracking). Key changes: - Add isJsxExpression() helper to detect JSX elements/fragments - Use when/unless only when: rightIsJsx OR rightNeedsDerive - Simple refs passed directly to when/unless (no derive wrapper) - String/number literals on right side → plain derive, no short-circuit 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 76ce31e commit 4e16ddb

10 files changed

+132
-739
lines changed

‎packages/ts-transformers/src/transformers/opaque-ref/emitters/binary-expression.ts‎

Lines changed: 86 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,20 @@ import { createUnlessCall, createWhenCall } from "../../builtins/ifelse.ts";
1010
import { selectDataFlowsReferencedIn } from "../../../ast/mod.ts";
1111
import { isSimpleOpaqueRefAccess } from "../opaque-ref.ts";
1212

13+
/**
14+
* Check if an expression is JSX (element, fragment, or self-closing).
15+
* Also handles parenthesized JSX like `(<div>...</div>)`.
16+
*/
17+
function isJsxExpression(expr: ts.Expression): boolean {
18+
// Unwrap parentheses
19+
while (ts.isParenthesizedExpression(expr)) {
20+
expr = expr.expression;
21+
}
22+
return ts.isJsxElement(expr) ||
23+
ts.isJsxFragment(expr) ||
24+
ts.isJsxSelfClosingElement(expr);
25+
}
26+
1327
export const emitBinaryExpression: Emitter = ({
1428
expression,
1529
dataFlows,
@@ -21,37 +35,58 @@ export const emitBinaryExpression: Emitter = ({
2135
if (dataFlows.all.length === 0) return undefined;
2236

2337
// Optimize && operator: convert to when instead of wrapping entire expression in derive
24-
// Example: foo.length > 0 && <div>...</div>
25-
// Becomes: when(derive(foo, foo => foo.length > 0), <div>...</div>)
38+
// Example: showPanel && <Panel/>
39+
// Becomes: when(showPanel, <Panel/>) or when(derive(condition), <Panel/>)
40+
//
41+
// The when/unless optimization is beneficial when the right side (value) is expensive
42+
// to construct, like JSX. This allows short-circuit evaluation to skip constructing
43+
// the value when the condition is falsy.
44+
//
45+
// If the right side is simple (not JSX, no reactive deps), using when/unless is just
46+
// overhead - better to wrap the whole expression in derive.
2647
if (expression.operatorToken.kind === ts.SyntaxKind.AmpersandAmpersandToken) {
2748
const leftDataFlows = selectDataFlowsReferencedIn(
2849
dataFlows,
2950
expression.left,
3051
);
52+
const rightDataFlows = selectDataFlowsReferencedIn(
53+
dataFlows,
54+
expression.right,
55+
);
3156

32-
const shouldDeriveLeft = leftDataFlows.length > 0 &&
33-
!isSimpleOpaqueRefAccess(expression.left, context.checker);
57+
// Check if right side is "expensive" - JSX or has reactive dependencies that need derive
58+
const rightIsJsx = isJsxExpression(expression.right);
59+
const rightNeedsDerive = rightDataFlows.length > 0 &&
60+
!isSimpleOpaqueRefAccess(expression.right, context.checker);
61+
const rightIsExpensive = rightIsJsx || rightNeedsDerive;
3462

35-
// Only apply when optimization if left side has opaque refs (needs computed)
36-
// Right side is handled by rewriteChildren which processes any opaque refs appropriately
37-
if (shouldDeriveLeft) {
38-
const plan = createBindingPlan(leftDataFlows);
39-
const computedPredicate = createComputedCallForExpression(
40-
expression.left,
41-
plan,
42-
context,
43-
);
44-
// If we couldn't create a computed call, fall back to original expression
45-
const predicate = computedPredicate ?? expression.left;
63+
// Only use when optimization if right side is expensive
64+
if (rightIsExpensive) {
65+
// Process left side - derive if it has reactive deps, otherwise pass as-is
66+
let condition: ts.Expression = expression.left;
67+
if (leftDataFlows.length > 0) {
68+
if (!isSimpleOpaqueRefAccess(expression.left, context.checker)) {
69+
const plan = createBindingPlan(leftDataFlows);
70+
const computedCondition = createComputedCallForExpression(
71+
expression.left,
72+
plan,
73+
context,
74+
);
75+
if (computedCondition) {
76+
condition = computedCondition;
77+
}
78+
}
79+
// If it's a simple opaque ref, pass it directly (no derive needed)
80+
}
4681

47-
// Process right side - rewrite children but don't wrap whole thing in derive
82+
// Process right side - rewrite children to handle nested opaque refs
4883
const value = rewriteChildren(expression.right) || expression.right;
4984

50-
// Create when(predicate, value)
51-
// This is equivalent to: ifElse(predicate, value, predicate)
85+
// Create when(condition, value)
86+
// This is equivalent to: ifElse(condition, value, condition)
5287
// Preserves && semantics where falsy values are returned as-is
5388
return createWhenCall({
54-
condition: predicate,
89+
condition,
5590
value,
5691
factory: context.factory,
5792
ctHelpers: context.ctHelpers,
@@ -60,30 +95,46 @@ export const emitBinaryExpression: Emitter = ({
6095
}
6196

6297
// Optimize || operator: convert to unless instead of wrapping entire expression in derive
63-
// Example: fallbackValue || <div>...</div>
64-
// Becomes: unless(derive(fallbackValue, v => v), <div>...</div>)
98+
// Example: value || <Fallback/>
99+
// Becomes: unless(value, <Fallback/>) or unless(derive(condition), <Fallback/>)
100+
//
101+
// Same rationale as &&: only beneficial when right side is expensive.
65102
if (expression.operatorToken.kind === ts.SyntaxKind.BarBarToken) {
66103
const leftDataFlows = selectDataFlowsReferencedIn(
67104
dataFlows,
68105
expression.left,
69106
);
107+
const rightDataFlows = selectDataFlowsReferencedIn(
108+
dataFlows,
109+
expression.right,
110+
);
70111

71-
const shouldDeriveLeft = leftDataFlows.length > 0 &&
72-
!isSimpleOpaqueRefAccess(expression.left, context.checker);
112+
// Check if right side is "expensive" - JSX or has reactive dependencies that need derive
113+
const rightIsJsx = isJsxExpression(expression.right);
114+
const rightNeedsDerive = rightDataFlows.length > 0 &&
115+
!isSimpleOpaqueRefAccess(expression.right, context.checker);
116+
const rightIsExpensive = rightIsJsx || rightNeedsDerive;
73117

74-
// Only apply unless optimization if left side has opaque refs (needs computed)
75-
// Right side is handled by rewriteChildren which processes any opaque refs appropriately
76-
if (shouldDeriveLeft) {
77-
const plan = createBindingPlan(leftDataFlows);
78-
const computedCondition = createComputedCallForExpression(
79-
expression.left,
80-
plan,
81-
context,
82-
);
83-
// If we couldn't create a computed call, fall back to original expression
84-
const condition = computedCondition ?? expression.left;
118+
// Only use unless optimization if right side is expensive
119+
if (rightIsExpensive) {
120+
// Process left side - derive if it has reactive deps, otherwise pass as-is
121+
let condition: ts.Expression = expression.left;
122+
if (leftDataFlows.length > 0) {
123+
if (!isSimpleOpaqueRefAccess(expression.left, context.checker)) {
124+
const plan = createBindingPlan(leftDataFlows);
125+
const computedCondition = createComputedCallForExpression(
126+
expression.left,
127+
plan,
128+
context,
129+
);
130+
if (computedCondition) {
131+
condition = computedCondition;
132+
}
133+
}
134+
// If it's a simple opaque ref, pass it directly (no derive needed)
135+
}
85136

86-
// Process right side - rewrite children but don't wrap whole thing in derive
137+
// Process right side - rewrite children to handle nested opaque refs
87138
const value = rewriteChildren(expression.right) || expression.right;
88139

89140
// Create unless(condition, value)

‎packages/ts-transformers/test/fixtures/jsx-expressions/logical-and-simple-ref.expected.tsx‎

Lines changed: 3 additions & 215 deletions
Original file line numberDiff line numberDiff line change
@@ -115,223 +115,11 @@ export default recipe(false as const satisfies __ctHelpers.JSONSchema, {
115115
} as const satisfies __ctHelpers.JSONSchema);
116116
return {
117117
[UI]: (<div>
118-
{/* Simple opaque ref - should NOT use when, just derive the whole expression */}
119-
{__ctHelpers.derive({
120-
type: "object",
121-
properties: {
122-
showPanel: {
123-
type: "boolean",
124-
asCell: true
125-
}
126-
},
127-
required: ["showPanel"]
128-
} as const satisfies __ctHelpers.JSONSchema, {
129-
type: "object",
130-
properties: {
131-
type: {
132-
type: "string",
133-
"enum": ["vnode"]
134-
},
135-
name: {
136-
type: "string"
137-
},
138-
props: {
139-
$ref: "#/$defs/Props"
140-
},
141-
children: {
142-
$ref: "#/$defs/RenderNode"
143-
},
144-
$UI: {
145-
$ref: "#/$defs/VNode"
146-
}
147-
},
148-
required: ["type", "name", "props"],
149-
$defs: {
150-
VNode: {
151-
type: "object",
152-
properties: {
153-
type: {
154-
type: "string",
155-
"enum": ["vnode"]
156-
},
157-
name: {
158-
type: "string"
159-
},
160-
props: {
161-
$ref: "#/$defs/Props"
162-
},
163-
children: {
164-
$ref: "#/$defs/RenderNode"
165-
},
166-
$UI: {
167-
$ref: "#/$defs/VNode"
168-
}
169-
},
170-
required: ["type", "name", "props"]
171-
},
172-
RenderNode: {
173-
anyOf: [{
174-
type: "string"
175-
}, {
176-
type: "number"
177-
}, {
178-
type: "boolean",
179-
"enum": [false]
180-
}, {
181-
type: "boolean",
182-
"enum": [true]
183-
}, {
184-
$ref: "#/$defs/VNode"
185-
}, {
186-
type: "object",
187-
properties: {}
188-
}, {
189-
type: "array",
190-
items: {
191-
$ref: "#/$defs/RenderNode"
192-
}
193-
}]
194-
},
195-
Props: {
196-
type: "object",
197-
properties: {},
198-
additionalProperties: {
199-
anyOf: [{
200-
type: "string"
201-
}, {
202-
type: "number"
203-
}, {
204-
type: "boolean",
205-
"enum": [false]
206-
}, {
207-
type: "boolean",
208-
"enum": [true]
209-
}, {
210-
type: "object",
211-
additionalProperties: true
212-
}, {
213-
type: "array",
214-
items: true
215-
}, {
216-
asCell: true
217-
}, {
218-
asStream: true
219-
}, {
220-
type: "null"
221-
}]
222-
}
223-
}
224-
}
225-
} as const satisfies __ctHelpers.JSONSchema, { showPanel: showPanel }, ({ showPanel }) => showPanel && <div>Panel content</div>)}
118+
{/* Simple opaque ref with JSX on right - SHOULD use when for short-circuit optimization */}
119+
{__ctHelpers.when(showPanel, <div>Panel content</div>)}
226120

227121
{/* Another simple ref */}
228-
{__ctHelpers.derive({
229-
type: "object",
230-
properties: {
231-
userName: {
232-
type: "string",
233-
asCell: true
234-
}
235-
},
236-
required: ["userName"]
237-
} as const satisfies __ctHelpers.JSONSchema, {
238-
type: "object",
239-
properties: {
240-
type: {
241-
type: "string",
242-
"enum": ["vnode"]
243-
},
244-
name: {
245-
type: "string"
246-
},
247-
props: {
248-
$ref: "#/$defs/Props"
249-
},
250-
children: {
251-
$ref: "#/$defs/RenderNode"
252-
},
253-
$UI: {
254-
$ref: "#/$defs/VNode"
255-
}
256-
},
257-
required: ["type", "name", "props"],
258-
$defs: {
259-
VNode: {
260-
type: "object",
261-
properties: {
262-
type: {
263-
type: "string",
264-
"enum": ["vnode"]
265-
},
266-
name: {
267-
type: "string"
268-
},
269-
props: {
270-
$ref: "#/$defs/Props"
271-
},
272-
children: {
273-
$ref: "#/$defs/RenderNode"
274-
},
275-
$UI: {
276-
$ref: "#/$defs/VNode"
277-
}
278-
},
279-
required: ["type", "name", "props"]
280-
},
281-
RenderNode: {
282-
anyOf: [{
283-
type: "string"
284-
}, {
285-
type: "number"
286-
}, {
287-
type: "boolean",
288-
"enum": [false]
289-
}, {
290-
type: "boolean",
291-
"enum": [true]
292-
}, {
293-
$ref: "#/$defs/VNode"
294-
}, {
295-
type: "object",
296-
properties: {}
297-
}, {
298-
type: "array",
299-
items: {
300-
$ref: "#/$defs/RenderNode"
301-
}
302-
}]
303-
},
304-
Props: {
305-
type: "object",
306-
properties: {},
307-
additionalProperties: {
308-
anyOf: [{
309-
type: "string"
310-
}, {
311-
type: "number"
312-
}, {
313-
type: "boolean",
314-
"enum": [false]
315-
}, {
316-
type: "boolean",
317-
"enum": [true]
318-
}, {
319-
type: "object",
320-
additionalProperties: true
321-
}, {
322-
type: "array",
323-
items: true
324-
}, {
325-
asCell: true
326-
}, {
327-
asStream: true
328-
}, {
329-
type: "null"
330-
}]
331-
}
332-
}
333-
}
334-
} as const satisfies __ctHelpers.JSONSchema, { userName: userName }, ({ userName }) => userName && <span>Hello {userName}</span>)}
122+
{__ctHelpers.when(userName, <span>Hello {userName}</span>)}
335123
</div>),
336124
};
337125
});

‎packages/ts-transformers/test/fixtures/jsx-expressions/logical-and-simple-ref.input.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ export default recipe("LogicalAndSimpleRef", (_state) => {
88
return {
99
[UI]: (
1010
<div>
11-
{/* Simple opaque ref - should NOT use when, just derive the whole expression */}
11+
{/* Simple opaque ref with JSX on right - SHOULD use when for short-circuit optimization */}
1212
{showPanel && <div>Panel content</div>}
1313

1414
{/* Another simple ref */}

0 commit comments

Comments
 (0)