-
Notifications
You must be signed in to change notification settings - Fork 1
Add configurable optimization bailouts for hooks #11
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduce a configurable disabled optimizations feature allowing selective disablement of advanced optimizations for specific hooks and properties, alongside a customizable bailout property mechanism for JSX attribute analysis. Function context tracking enables hook-based disablement to force basic transforms, and a new conditional code path bypasses memoization when optimizations are disabled. Changes
Sequence DiagramsequenceDiagram
participant User
participant Transformer
participant BlockAnalyzer
participant JSXTransformer
User->>Transformer: transformCode(source, options)
Transformer->>Transformer: resolveDisabledOptimizations(options)
rect rgb(200, 220, 240)
Note over Transformer: Setup & Scanning Phase
Transformer->>Transformer: Create functionStack
loop for each function node
Transformer->>Transformer: push function to functionStack
Transformer->>BlockAnalyzer: new BlockAnalyzer(..., customBailoutProps)
BlockAnalyzer->>BlockAnalyzer: merge bailoutPropNames
end
end
rect rgb(240, 200, 220)
Note over Transformer: Hook Disablement Check
loop for each hook call in function
Transformer->>Transformer: Check if hook in disabledOptimizations
alt Hook is disabled
Transformer->>Transformer: Mark function in forceBasicTransformFunctions
end
end
end
rect rgb(220, 240, 200)
Note over Transformer: JSX Transformation Phase
Transformer->>JSXTransformer: transformJsxElement(element, context)
alt shouldUseBasicTransform(context) = true
JSXTransformer->>JSXTransformer: Return basic optimized element
else Advanced optimization path
JSXTransformer->>BlockAnalyzer: analyzeJsxElement(element)
BlockAnalyzer->>BlockAnalyzer: Use bailoutPropNames for analysis
JSXTransformer->>JSXTransformer: Apply memoization & fine-patch
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
transformer/src/analyzer.ts(3 hunks)transformer/src/index.ts(6 hunks)transformer/src/transformer.ts(6 hunks)transformer/src/types.ts(2 hunks)transformer/test/transformer.integration.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
transformer/src/analyzer.ts (1)
transformer/src/roblox-bridge.ts (1)
robloxStaticDetector(632-632)
transformer/test/transformer.integration.test.ts (1)
transformer/src/index.ts (1)
DecillionTransformerOptions(17-26)
transformer/src/transformer.ts (2)
transformer/src/types.ts (5)
DisabledOptimizationOptions(4-7)ResolvedDisabledOptimizationOptions(9-12)isResolvedDisabledOptimizations(41-50)resolveDisabledOptimizations(14-39)OptimizationContext(133-154)transformer/src/roblox-bridge.ts (1)
robloxStaticDetector(632-632)
transformer/src/index.ts (3)
transformer/src/types.ts (2)
DisabledOptimizationOptions(4-7)resolveDisabledOptimizations(14-39)transformer/src/analyzer.ts (1)
BlockAnalyzer(23-719)transformer/src/transformer.ts (1)
getFunctionName(1046-1072)
| if (ts.isCallExpression(node) && resolvedDisabledOptimizations.hooks.size > 0) { | ||
| const hookName = getCallExpressionIdentifier(node); | ||
| if (hookName && resolvedDisabledOptimizations.hooks.has(hookName)) { | ||
| const activeFunction = getActiveFunctionName(functionStack); | ||
| if (activeFunction) { | ||
| optimizationContext.forceBasicTransformFunctions.add(activeFunction); | ||
| if (debug) { | ||
| console.log( | ||
| `Disabling advanced optimizations for ${activeFunction} due to hook ${hookName}`, | ||
| ); | ||
| } | ||
| } | ||
| } |
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.
Hook bailouts miss anonymous components
We only ever push function names onto functionStack, so getActiveFunctionName returns undefined for anonymous functions (e.g. export default () => { useContext(...) }). That means the new hook-based bailout never trips for those components, and advanced optimizations still run even though useContext is in disabledOptimizations.hooks. This breaks the main feature for a very common pattern. Please track the actual function nodes (or another unique identifier) instead of just names when pushing/popping, and store those identifiers in forceBasicTransformFunctions so anonymous/default exports are covered.
Summary
Testing
cd transformer && npm testhttps://chatgpt.com/codex/tasks/task_e_68f88b0fd9d4832982aeecd49429117b
Summary by CodeRabbit
Release Notes
New Features
disabledOptimizationsoption to transformer configuration, enabling users to disable advanced optimizations for specific hooks, which will fall back to basic transforms.Refactor