-
Notifications
You must be signed in to change notification settings - Fork 11
fix(transform): optimize && conditionals to use ifElse instead of derive #1966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 5 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/ts-transformers/src/transformers/opaque-ref/emitters/binary-expression.ts">
<violation number="1" location="packages/ts-transformers/src/transformers/opaque-ref/emitters/binary-expression.ts:64">
The synthesized `ifElse` false branch returns `null`, but `a && b` evaluates to the left operand when it is falsy. This changes results (e.g., `0 && <div/>` now yields `null` instead of `0`). Please return the original left value when the predicate is false to preserve semantics.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| context.factory.createToken(ts.SyntaxKind.QuestionToken), | ||
| whenTrue, | ||
| context.factory.createToken(ts.SyntaxKind.ColonToken), | ||
| context.factory.createNull(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The synthesized ifElse false branch returns null, but a && b evaluates to the left operand when it is falsy. This changes results (e.g., 0 && <div/> now yields null instead of 0). Please return the original left value when the predicate is false to preserve semantics.
Prompt for AI agents
Address the following comment on packages/ts-transformers/src/transformers/opaque-ref/emitters/binary-expression.ts at line 64:
<comment>The synthesized `ifElse` false branch returns `null`, but `a && b` evaluates to the left operand when it is falsy. This changes results (e.g., `0 && <div/>` now yields `null` instead of `0`). Please return the original left value when the predicate is false to preserve semantics.</comment>
<file context>
@@ -6,16 +6,74 @@ import {
+ context.factory.createToken(ts.SyntaxKind.QuestionToken),
+ whenTrue,
+ context.factory.createToken(ts.SyntaxKind.ColonToken),
+ context.factory.createNull(),
+ );
+
</file context>
✅ Addressed in 27e479f
|
Ah, what you could do is: ifElse is already a wrapper around the underlying built-in which takes a 3-tuple! So we can just add another that takes two parameters and calls the underlying ifElse factory with [a, b, a]. You could even make the || variant, although I'm not sure it's used in practice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 10 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed changes from recent commits (found 1 issue).
1 issue found across 12 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/ts-transformers/test/fixtures/jsx-expressions/opaque-ref-cell-map.expected.tsx">
<violation number="1" location="packages/ts-transformers/test/fixtures/jsx-expressions/opaque-ref-cell-map.expected.tsx:108">
The inner derive already yields an OpaqueRef, so wrapping it in an outer derive produces an OpaqueRef<OpaqueRef> instead of the expected string fallback; drop the outer derive so the JSX receives a single OpaqueRef result.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| Go to Charm {__ctHelpers.derive(index, index => index + 1)} | ||
| </ct-button> | ||
| <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => charm[NAME] || "Unnamed")}</span> | ||
| <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => __ctHelpers.unless(__ctHelpers.derive(charm, charm => charm[NAME]), "Unnamed"))}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inner derive already yields an OpaqueRef, so wrapping it in an outer derive produces an OpaqueRef instead of the expected string fallback; drop the outer derive so the JSX receives a single OpaqueRef result.
Prompt for AI agents
Address the following comment on packages/ts-transformers/test/fixtures/jsx-expressions/opaque-ref-cell-map.expected.tsx at line 108:
<comment>The inner derive already yields an OpaqueRef, so wrapping it in an outer derive produces an OpaqueRef<OpaqueRef> instead of the expected string fallback; drop the outer derive so the JSX receives a single OpaqueRef result.</comment>
<file context>
@@ -105,7 +105,7 @@ export default recipe("Charms Launcher", () => {
Go to Charm {__ctHelpers.derive(index, index => index + 1)}
</ct-button>
- <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => charm[NAME] || "Unnamed")}</span>
+ <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => __ctHelpers.unless(__ctHelpers.derive(charm, charm => charm[NAME]), "Unnamed"))}</span>
</li>))}
</ul>)}
</file context>
| <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => __ctHelpers.unless(__ctHelpers.derive(charm, charm => charm[NAME]), "Unnamed"))}</span> | |
| <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.unless(__ctHelpers.derive(charm, charm => charm[NAME]), "Unnamed")}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed changes from recent commits (found 4 issues).
4 issues found across 12 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="packages/ts-transformers/test/fixtures/jsx-expressions/logical-or-unless.input.tsx">
<violation number="1" location="packages/ts-transformers/test/fixtures/jsx-expressions/logical-or-unless.input.tsx:11">
Because `!items.length` becomes `true` when the list is empty, this `||` expression resolves to the boolean `true`, so the fallback never displays in the empty state and instead shows when items exist. Replace the left operand with `items.length` (or invert via ternary) so the fallback renders only when the list is empty.</violation>
</file>
<file name="packages/ts-transformers/test/fixtures/jsx-expressions/logical-and-non-jsx.expected.tsx">
<violation number="1" location="packages/ts-transformers/test/fixtures/jsx-expressions/logical-and-non-jsx.expected.tsx:11">
Passing the dynamic string directly to when() evaluates `user.name` eagerly, so the greeting never updates when the cell changes and can even read `undefined`. Wrap the consequent in a derive (or use ifElse) so the string recomputes reactively.</violation>
<violation number="2" location="packages/ts-transformers/test/fixtures/jsx-expressions/logical-and-non-jsx.expected.tsx:14">
The numeric branch reads `user.age` eagerly before the predicate is true, so the value never updates and can access the cell while null. Keep the consequent lazy/reactive (e.g., wrap it in derive alongside the predicate) to preserve correct semantics.</violation>
</file>
<file name="packages/ts-transformers/test/fixtures/jsx-expressions/opaque-ref-cell-map.expected.tsx">
<violation number="1" location="packages/ts-transformers/test/fixtures/jsx-expressions/opaque-ref-cell-map.expected.tsx:108">
The derive callback now invokes __ctHelpers.derive on the already-resolved charm object, so we pass a plain object where an OpaqueRef is expected. This will throw or mis-wire subscriptions at runtime; please derive the property directly instead.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| [UI]: ( | ||
| <div> | ||
| {/* Pattern: falsy check || fallback */} | ||
| {!items.length || <span>List is empty</span>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because !items.length becomes true when the list is empty, this || expression resolves to the boolean true, so the fallback never displays in the empty state and instead shows when items exist. Replace the left operand with items.length (or invert via ternary) so the fallback renders only when the list is empty.
Prompt for AI agents
Address the following comment on packages/ts-transformers/test/fixtures/jsx-expressions/logical-or-unless.input.tsx at line 11:
<comment>Because `!items.length` becomes `true` when the list is empty, this `||` expression resolves to the boolean `true`, so the fallback never displays in the empty state and instead shows when items exist. Replace the left operand with `items.length` (or invert via ternary) so the fallback renders only when the list is empty.</comment>
<file context>
@@ -0,0 +1,15 @@
+ [UI]: (
+ <div>
+ {/* Pattern: falsy check || fallback */}
+ {!items.length || <span>List is empty</span>}
+ </div>
+ ),
</file context>
✅ Addressed in acd174a
| return { | ||
| [UI]: (<div> | ||
| {/* Non-JSX right side: string template with complex expression */} | ||
| <p>{__ctHelpers.when(__ctHelpers.derive(user.name, _v1 => _v1.length > 0), `Hello, ${user.name}!`)}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the dynamic string directly to when() evaluates user.name eagerly, so the greeting never updates when the cell changes and can even read undefined. Wrap the consequent in a derive (or use ifElse) so the string recomputes reactively.
Prompt for AI agents
Address the following comment on packages/ts-transformers/test/fixtures/jsx-expressions/logical-and-non-jsx.expected.tsx at line 11:
<comment>Passing the dynamic string directly to when() evaluates `user.name` eagerly, so the greeting never updates when the cell changes and can even read `undefined`. Wrap the consequent in a derive (or use ifElse) so the string recomputes reactively.</comment>
<file context>
@@ -0,0 +1,21 @@
+ return {
+ [UI]: (<div>
+ {/* Non-JSX right side: string template with complex expression */}
+ <p>{__ctHelpers.when(__ctHelpers.derive(user.name, _v1 => _v1.length > 0), `Hello, ${user.name}!`)}</p>
+
+ {/* Non-JSX right side: number expression */}
</file context>
| <p>{__ctHelpers.when(__ctHelpers.derive(user.name, _v1 => _v1.length > 0), `Hello, ${user.name}!`)}</p> | ||
|
|
||
| {/* Non-JSX right side: number expression */} | ||
| <p>Age: {__ctHelpers.when(__ctHelpers.derive(user.age, _v1 => _v1 > 18), user.age)}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The numeric branch reads user.age eagerly before the predicate is true, so the value never updates and can access the cell while null. Keep the consequent lazy/reactive (e.g., wrap it in derive alongside the predicate) to preserve correct semantics.
Prompt for AI agents
Address the following comment on packages/ts-transformers/test/fixtures/jsx-expressions/logical-and-non-jsx.expected.tsx at line 14:
<comment>The numeric branch reads `user.age` eagerly before the predicate is true, so the value never updates and can access the cell while null. Keep the consequent lazy/reactive (e.g., wrap it in derive alongside the predicate) to preserve correct semantics.</comment>
<file context>
@@ -0,0 +1,21 @@
+ <p>{__ctHelpers.when(__ctHelpers.derive(user.name, _v1 => _v1.length > 0), `Hello, ${user.name}!`)}</p>
+
+ {/* Non-JSX right side: number expression */}
+ <p>Age: {__ctHelpers.when(__ctHelpers.derive(user.age, _v1 => _v1 > 18), user.age)}</p>
+ </div>),
+ };
</file context>
| Go to Charm {__ctHelpers.derive(index, index => index + 1)} | ||
| </ct-button> | ||
| <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => charm[NAME] || "Unnamed")}</span> | ||
| <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => __ctHelpers.unless(__ctHelpers.derive(charm, charm => charm[NAME]), "Unnamed"))}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The derive callback now invokes __ctHelpers.derive on the already-resolved charm object, so we pass a plain object where an OpaqueRef is expected. This will throw or mis-wire subscriptions at runtime; please derive the property directly instead.
Prompt for AI agents
Address the following comment on packages/ts-transformers/test/fixtures/jsx-expressions/opaque-ref-cell-map.expected.tsx at line 108:
<comment>The derive callback now invokes __ctHelpers.derive on the already-resolved charm object, so we pass a plain object where an OpaqueRef is expected. This will throw or mis-wire subscriptions at runtime; please derive the property directly instead.</comment>
<file context>
@@ -105,7 +105,7 @@ export default recipe("Charms Launcher", () => {
Go to Charm {__ctHelpers.derive(index, index => index + 1)}
</ct-button>
- <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => charm[NAME] || "Unnamed")}</span>
+ <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => __ctHelpers.unless(__ctHelpers.derive(charm, charm => charm[NAME]), "Unnamed"))}</span>
</li>))}
</ul>)}
</file context>
| <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => __ctHelpers.unless(__ctHelpers.derive(charm, charm => charm[NAME]), "Unnamed"))}</span> | |
| <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => __ctHelpers.unless(charm[NAME], "Unnamed"))}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files
acd174a to
f3d9ddd
Compare
seefeldb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me. i'm realizing reading this that we might need to add schema information to ifElse/when/unless. not 100% let's discuss. either way not a blocker to merge.
- Transform `condition && value` to `when(condition, value)` - Transform `condition || fallback` to `unless(condition, fallback)` - Add when() and unless() built-in functions to runner - Add comprehensive test fixtures for logical operator transformations - Add runtime unit tests for when/unless - Add integration pattern tests for when/unless operators 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
dcbc3e3 to
6857e2b
Compare
CT-1005: Conditional expressions using && operator now generate ifElse calls instead of wrapping entire expression in derive. This optimization improves performance and readability.
Before:
{list.length > 0 &&
// Became: derive(list, list => list.length > 0 &&
After:
{list.length > 0 &&
// Becomes: ifElse(derive(list, list => list.length > 0),
Benefits:
Updates binary-expression emitter to detect && operator and apply ifElse optimization when left side contains opaque refs. Includes test fixture updates to reflect new expected behavior.
🤖 Generated with Claude Code
Summary by cubic
Optimized logical && and || in JSX to use when/unless helpers instead of deriving the whole expression. This preserves JavaScript short-circuit behavior, keeps JSX out of derive, and addresses CT-1005.
Refactors
New Features
Written for commit 6857e2b. Summary will update automatically on new commits.