Allow JS root components to reinitialize on circuit restart#20
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request aims to enable JS root components to reinitialize when a Blazor circuit restarts, addressing scenarios where users navigate away and return to a page with dynamically registered JS root components.
Changes:
- Modified the TypeScript logic to accept reinitialization from the same renderer type during circuit restarts
- Simplified test infrastructure by removing conditional component registration
- Consolidated test coverage by moving tests to the existing StatePersistenceTest class
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| RazorComponentEndpointsStartup.cs | Removed conditional registration logic; component is now always registered for JS root components |
| Program.cs | Removed separate test host configuration that was specific to JS root component testing |
| StatePersistenceTest.cs | Added test to verify persistent state works in dynamic JS roots |
| StatePersistanceJSRootTest.cs | Deleted entire test class; functionality moved to StatePersistenceTest (also fixes spelling error in filename) |
| WebRendererInteropMethods.ts | Added rendererId parameter to enableJSRootComponents call |
| JSRootComponents.ts | Added logic to track rendererId and allow reinitialization, with flag to prevent duplicate initializer calls |
Comments suppressed due to low confidence (1)
src/Components/test/E2ETest/Tests/StatePersistanceJSRootTest.cs:1
- The deleted file name contained a spelling error: 'StatePersistance' should be 'StatePersistence'. This has been corrected by moving the test to the correctly-named StatePersistenceTest.cs file.
| [Theory] | ||
| [InlineData("ServerNonPrerendered")] | ||
| [InlineData("WebAssemblyNonPrerendered")] | ||
| public void PersistentStateIsSupportedInDynamicJSRoots(string renderMode) | ||
| { | ||
| Navigate($"subdir/WasmMinimal/dynamic-js-root.html?renderMode={renderMode}"); | ||
|
|
||
| Browser.Equal("Counter", () => Browser.Exists(By.TagName("h1")).Text); | ||
| Browser.Equal("Current count: 0", () => Browser.Exists(By.CssSelector("p[role='status']")).Text); | ||
|
|
||
| Browser.Click(By.CssSelector("button.btn-primary")); | ||
| Browser.Equal("Current count: 1", () => Browser.Exists(By.CssSelector("p[role='status']")).Text); | ||
| } |
There was a problem hiding this comment.
The test PersistentStateIsSupportedInDynamicJSRoots verifies that dynamic JS root components work with persistent state, but it doesn't actually test the circuit restart scenario mentioned in the PR title. The test should navigate away to destroy the circuit, then navigate back to verify that the component can reinitialize after circuit restart. Consider adding assertions that verify the component continues to work after a circuit restart, similar to the StateIsProvidedEveryTimeACircuitGetsCreated test pattern.
| // 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.
The variable jsComponentParametersByIdentifier is assigned but never updated when enableJSRootComponents is called during a circuit restart. This means that when a new circuit starts with potentially different component parameters, the old parameters from the initial circuit will still be used by functions like RootComponentsFunctions.add(). This could lead to incorrect parameter metadata being used for dynamically added components after a circuit restart.
| manager = managerInstance; | |
| manager = managerInstance; | |
| jsComponentParametersByIdentifier = jsComponentParameters; |
| if (manager && currentRendererId === rendererId) { | ||
| // 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.'); | ||
| } |
There was a problem hiding this comment.
The condition checks if manager exists AND currentRendererId equals the new rendererId, which means it throws an error when the SAME renderer is being re-initialized (e.g., circuit restart). However, the comment states this is for detecting "A different renderer type" scenario. The logic is inverted - it should check if manager exists AND currentRendererId is DIFFERENT from rendererId to detect multi-host scenarios. As written, it prevents the intended circuit restart scenario and allows different renderers.
| for (const componentIdentifier of componentIdentifiers) | ||
| initializerFunc(componentIdentifier, jsComponentParameters[componentIdentifier]); |
There was a problem hiding this comment.
The for loop is written without braces as a single-line statement. While syntactically valid, this deviates from the established pattern in the codebase where multi-line control flow statements use braces. The previous version of this code used braces, and removing them reduces code consistency and makes the code more error-prone to future modifications.
| for (const componentIdentifier of componentIdentifiers) | |
| initializerFunc(componentIdentifier, jsComponentParameters[componentIdentifier]); | |
| for (const componentIdentifier of componentIdentifiers) | |
| { | |
| initializerFunc(componentIdentifier, jsComponentParameters[componentIdentifier]); | |
| } |
Benchmark PR from qodo-benchmark#90