Allow JS root components to reinitialize on circuit restart#17
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (manager) { | ||
| // This will only happen in very nonstandard cases where someone has multiple hosts. | ||
| // It's up to the developer to ensure that only one of them enables dynamic root components. | ||
| if (manager && currentRendererId === rendererId) { |
There was a problem hiding this comment.
🔴 Inverted condition prevents circuit restart but allows multi-host scenario
The condition logic in enableJSRootComponents is inverted from the intended behavior.
Click to expand
Actual vs Expected Behavior
Original code (correct):
if (manager && currentRendererId !== rendererId) {
throw new Error('Dynamic root components have already been enabled.');
}New code (incorrect):
if (manager && currentRendererId === rendererId) {
throw new Error('Dynamic root components have already been enabled.');
}The original code throws an error when a different renderer tries to enable JS root components (multi-host scenario not supported), but allows the same renderer to re-enable (circuit restart scenario).
The new code does the opposite: it throws an error when the same renderer tries to re-enable (breaking circuit restart), but allows different renderers to enable (which was supposed to be blocked).
Impact
- Circuit restart scenarios (the PR's stated goal) will fail with "Dynamic root components have already been enabled" error
- Multi-host scenarios (Server + WebAssembly) that should be blocked will now be allowed, potentially causing undefined behavior
The comment on lines 127-128 says "A different renderer type (e.g., Server vs WebAssembly) is trying to enable JS root components" but the condition currentRendererId === rendererId checks for the same renderer, contradicting the comment.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // because the old circuit is gone. We don't dispose the old manager - doing so would cause | ||
| // JSDisconnectedException because the circuit that created it no longer exists. | ||
| currentRendererId = rendererId; | ||
| manager = managerInstance; |
There was a problem hiding this comment.
🔴 Missing assignment of jsComponentParametersByIdentifier causes undefined access
The assignment jsComponentParametersByIdentifier = jsComponentParameters; was removed from enableJSRootComponents, leaving the module-level variable uninitialized.
Click to expand
How the Bug Gets Triggered
The variable jsComponentParametersByIdentifier is declared at line 14 but never assigned a value in the new code. When RootComponentsFunctions.add() is called at line 30:
const component = new DynamicRootComponent(componentId, jsComponentParametersByIdentifier[componentIdentifier]);This will access undefined[componentIdentifier], resulting in undefined being passed to the DynamicRootComponent constructor.
Impact
In the DynamicRootComponent constructor at lines 77-84:
constructor(componentId: number, parameters: JSComponentParameter[]) {
this._componentId = componentId;
for (const parameter of parameters) { // <-- iterating over undefined
if (parameter.type === 'eventcallback') {
this._jsEventCallbackWrappers.set(parameter.name.toLowerCase(), new EventCallbackWrapper());
}
}
}Iterating over undefined will throw a TypeError at runtime, breaking all dynamic root component creation.
Recommendation: Add jsComponentParametersByIdentifier = jsComponentParameters; after line 137 (after manager = managerInstance;)
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Benchmark PR from qodo-benchmark#90