-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[release/8.0-rc2] [Blazor] Make auto components prefer the existing render mode #50851
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c505759
Improved auto mode decision making
MackinnonBuck 551ad19
Simplify things a bit
MackinnonBuck bf18121
PR feedback
MackinnonBuck 5559739
Account for WebAssembly activation latency
MackinnonBuck 95c0d8f
Fix test failures
MackinnonBuck File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
when does getRootComponentCount gets populated? Can we be in a situation where we are starting both runtimes and end up having a similar situation? (Some components get assigned server, others webassembly, based on how quickly the runtimes start.
Also, what happens on situations where we start the server runtime, then navigate and kill it, and render new components? Do we switch to wasm at that point?
Uh oh!
There was an error while loading. Please reload this page.
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.
Good thing to check! It looks like the server sends
'JS.AttachComponent'
with a descriptor, which triggersattachRootComponentToLogicalElement
and thencethis.rootComponentIds.add(componentId)
. So the server renderer will only have components after the circuit is fully established, and not while we're still starting it.So it does seem possible to have this sequence:
server
, so it starts establishing a circuit, sending the descriptor for the auto componentwasm
That would be much more of an edge case than before (where any enhanced nav after starting the circuit could resolve auto as wasm), but seems still possible.
There's already a
hasStartedServer
function that is used to make sure we don't start a 2nd circuit while the 1st one is still in process of starting, so maybe ifhasStartedServer
is true butcircuit.isDisposedOrDisposing
is false, we should resolve auto as server, regardless of whether the server renderer has any components, i.e., the priority order would be:Is this correct, @MackinnonBuck? You know this area better and maybe counting
server.rootComponentIds
is already equivalent.Uh oh!
There was an error while loading. Please reload this page.
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.
Good catch, and your analysis seems correct to me. I think the right thing to do here would be to go with your suggestion and resolve Auto to use Blazor Server if the circuit is active, even if there aren't Server components on the page.
Uh oh!
There was an error while loading. Please reload this page.
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.
I actually realized that this might lead to less desirable behavior if a page gets loaded with Auto, WebAssembly, and Server components all present. The logic above would resolve the Auto component to use Server because the circuit starts before the first WebAssembly component gets activated (considering the time it takes for the WebAssembly runtime to start). This actually broke an E2E test.
I'm proposing we use the following logic:
The "or SSR components on the page that would get activated using WebAssembly/Server" means:
^ This reuses part of the same logic we already use for determining whether the circuit can be closed.
How does this sound?