-
Notifications
You must be signed in to change notification settings - Fork 0
Code Review Bench PR #64536 - Allow JS root components to reinitialize on circuit restart #7
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: base_pr_64536_20260125_2237
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,7 +10,9 @@ let nextPendingDynamicRootComponentIdentifier = 0; | |||||
| type ComponentParameters = object | null | undefined; | ||||||
|
|
||||||
| let manager: DotNet.DotNetObject | undefined; | ||||||
| let currentRendererId: number | undefined; | ||||||
| let jsComponentParametersByIdentifier: JSComponentParametersByIdentifier; | ||||||
| let hasInitializedJsComponents = false; | ||||||
|
|
||||||
| // These are the public APIs at Blazor.rootComponents.* | ||||||
| export const RootComponentsFunctions = { | ||||||
|
|
@@ -116,28 +118,35 @@ class DynamicRootComponent { | |||||
|
|
||||||
| // Called by the framework | ||||||
| export function enableJSRootComponents( | ||||||
| rendererId: number, | ||||||
| managerInstance: DotNet.DotNetObject, | ||||||
| jsComponentParameters: JSComponentParametersByIdentifier, | ||||||
| jsComponentInitializers: JSComponentIdentifiersByInitializer | ||||||
| ): void { | ||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚨 Bug: Guard condition is inverted: blocks same renderer, allows differentThe condition on line 126 ( Current behavior:
Intended behavior (per PR description and comments):
The comment on lines 127-128 is also inconsistent with the condition — it says "A different renderer type is trying to enable JS root components" but the condition matches when renderer IDs are the same. Fix: Change if (manager && currentRendererId !== rendererId) {This would correctly block different renderers while allowing same-renderer reinitialization on circuit restart. Was this helpful? React with 👍 / 👎
Suggested change
|
||||||
| // A different renderer type (e.g., Server vs WebAssembly) is trying to enable JS root components. | ||||||
| // This is a multi-host scenario which is not supported for dynamic root components. | ||||||
| throw new Error('Dynamic root components have already been enabled.'); | ||||||
| } | ||||||
|
|
||||||
| // When the same renderer type re-enables (e.g., circuit restart or new circuit on same page), | ||||||
| // accept the new manager. The old manager's DotNetObjectReference is no longer valid anyway | ||||||
| // 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; | ||||||
| jsComponentParametersByIdentifier = jsComponentParameters; | ||||||
|
|
||||||
| // Call the registered initializers. This is an arbitrary subset of the JS component types that are registered | ||||||
| // on the .NET side - just those of them that require some JS-side initialization (e.g., to register them | ||||||
| // as custom elements). | ||||||
| for (const [initializerIdentifier, componentIdentifiers] of Object.entries(jsComponentInitializers)) { | ||||||
| const initializerFunc = DotNet.findJSFunction(initializerIdentifier, 0) as JSComponentInitializerCallback; | ||||||
| for (const componentIdentifier of componentIdentifiers) { | ||||||
| const parameters = jsComponentParameters[componentIdentifier]; | ||||||
| initializerFunc(componentIdentifier, parameters); | ||||||
|
|
||||||
| if (!hasInitializedJsComponents) { | ||||||
| // Call the registered initializers. This is an arbitrary subset of the JS component types that are registered | ||||||
| // on the .NET side - just those of them that require some JS-side initialization (e.g., to register them | ||||||
| // as custom elements). | ||||||
| for (const [initializerIdentifier, componentIdentifiers] of Object.entries(jsComponentInitializers)) { | ||||||
| const initializerFunc = DotNet.findJSFunction(initializerIdentifier, 0) as JSComponentInitializerCallback; | ||||||
| for (const componentIdentifier of componentIdentifiers) | ||||||
| initializerFunc(componentIdentifier, jsComponentParameters[componentIdentifier]); | ||||||
| } | ||||||
|
|
||||||
| hasInitializedJsComponents = true; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
This file was deleted.
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.
🚨 Bug:
jsComponentParametersByIdentifieris never assigned, breakingadd()The PR removed the line
jsComponentParametersByIdentifier = jsComponentParameters;fromenableJSRootComponents()(visible in the diff as a deleted line). However, the module-scoped variablejsComponentParametersByIdentifieris still used on line 30:Since
jsComponentParametersByIdentifieris declared on line 14 but never assigned a value after this change, it will beundefined. Any call toRootComponentsFunctions.add()(the public API for dynamically adding root components) will crash with aTypeError: Cannot read properties of undefined.The variable needs to be updated each time
enableJSRootComponentsis called so that newly added dynamic root components can look up their parameter definitions. This is especially important on circuit restart, where a fresh set of parameters may be provided.Fix: Restore the assignment of
jsComponentParametersByIdentifierin the function body (outside thehasInitializedJsComponentsguard, so it's updated on every call):Was this helpful? React with 👍 / 👎