Persist Prerendered State #50373

surayya-MS · 2023-08-28T15:30:38Z

Persist Prerendered State

Description

https://learn.microsoft.com/en-us/aspnet/core/mvc/views/tag-helpers/built-in/persist-component-state?view=aspnetcore-7.0

With this PR the usage example stays exactly the same as in the link above.

Fixes #49733

surayya-MS · 2023-08-29T14:54:14Z

src/Components/Components/src/PersistentComponentState.cs

-        _registeredCallbacks.Add(callback);
+        if (callback.Target is not IComponent)
+        {
+            throw new InvalidOperationException("Cannot infer serialization mode for non component. Provide a serialization mode.");


When the provided callback.Target in RegisterOnPersisting is not a component throw an error saying to use the overloaded method.

What happens when using a pure Blazor Server or Blazor WebAssembly application? Shouldn't it work to have a callback defined outside of a component, because we know the serialization mode will always be "Server" or "WebAssembly"?

Good point, thanks for noting!

I changed the code by moving the check for callback target being a component into the EndpointHtmlRenderer.GetCallbackTargetSerializationMode.

I also implemented ServerSerializationModeHandler and WebAssemblySerializationModeHandler for pure Blazor Server and Blazor WebAssembly.

I wasn't sure where to add the ServerSerializationModeHandler. Seems like this is the right place https://github.com/dotnet/aspnetcore/pull/50373/files#diff-c1d00e1ea2b0628c9c2d2a32c1e59e625dd3c972e9938b4b0f33d2709e9ec5bfR212

surayya-MS · 2023-08-29T14:55:47Z

src/Components/Components/src/PersistedStateSerializationMode.cs

@@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.Components;
 public enum PersistedStateSerializationMode


Decided to use the existing enum PersistedStateSerializationMode instead of creating the new one. Moved it to Components dll from the Endpoints which is fine because the namespace remains the same.

surayya-MS · 2023-08-29T14:57:42Z

src/Components/Components/src/PersistentComponentState.cs

+        if (SerializationMode == PersistedStateSerializationMode.Infer)
+        {
+            var component = (IComponent)callback.Target;
+            serializationMode = _serializationModeHandler.GetComponentSerializationMode(component);


The serialization mode of the component is calculated by IComponentSerializationModeHadler. EndpointHtmlRenderer implements the new interface.

surayya-MS · 2023-08-29T15:02:36Z

src/Components/Components/src/PersistentComponentState.cs

+            throw new InvalidOperationException("Cannot infer serialization mode for non component. Provide a serialization mode.");
+        }
+
+        var serializationMode = SerializationMode;


When the code is executed on the circuit (or webassembly runtime) there is no way to get the render mode of the component. For this reason I changed ComponentStatePersistenceManager.RestoreStateAsync to provide the serialization mode.

Hm, it's not super intuitive to me that we use RestoreStateAsync to define the SerializationMode for future reference in RegisterOnPersisting. It's giving RestoreStateAsync an additional responsibility that it doesn't look like it should have, and it relies on the fact that RestoreStateAsync always gets called before RegisterOnPersisting.

I'm guessing the reason you chose to do it this way is because previously, the serialization mode was only specified in PrerenderPersistedStateAsync, but this obviously gets called long after any calls to RegisterOnPersisting occur, making it impossible to infer the serialization mode up front. Is that correct?

Rather than attempting to infer the serialization mode as soon as RegisterOnPersisting gets called, could we do it at the time of persistence? That would allow us to remove the extra argument in RestoreStateAsync.

We might be able to use an approach like this:

Go back to a single _registeredCallbacks field, and change its type to something like List<OnPersistingCallback>, where OnPersistingCallback is a struct containing:

The Func<Task> callback itself

The PersistedStateSerializationMode passed to RegisterOnPersisting (or Infer if the overload without that argument is used).

Remove EndpointHtmlRenderer.Prerender{Server|WebAssembly}PersistedStateAsync() and instead call PrerenderPersistedStateAsync() with the Infer serialization mode from RazorComponentEndpointInvoker.

This will require changing some of the logic in EndpointHtmlRenderer.PrerenderingState.cs, but I think this may have been expected anyway, especially considering the existing "TODO: This will all have to change when we support multiple render modes in the same response" comment.

Resolve the serialization mode for each callback during ComponentStatePersistenceManager.PersistStateAsync:

If the serialization mode argument is Infer:

Resolve each callback's serialization mode to its mode in the OnPersistingCallback struct.

If a callback's serialization mode is also Infer, run your existing logic that infers based on the target component's render mode

If the serialization mode argument is Server or WebAssembly:

Then a OnPersistingCallback.SerializationMode of Infer resolves to the value of the serialization mode argument

But if the OnPersistingCallback.SerializationMode is not Infer, and it doesn't match the specified serialization mode, then throw

What do you think? Note that I haven't tried this out, and I'm sure you know this area better than I do at this point, so it's totally possible there's a detail I'm missing that makes this approach less desirable 🙂

Thanks for the suggestion! I tried it out and it won't work.
The reason is that ComponentStatePersistenceManager.PersistStateAsync(store, serializationMode, dispatcher) persists the state for the provided serializationMode (Server or WebAssembly) into the provided store. We want to store server state and webassembly state into differrent stores. Currently we store the state in ProtectedPrerenderComponentApplicationStore for Blazor Server and PrerenderComponentApplicationStore for Blazor WebAssembly.

That is why it seemed like a good idea at the time to change ComponentStatePersistenceManager.RestoreStateAsync(store, serializationMode) by adding serializationMode argument. We persist the state for the given serialization mode in the given store and similarly restoring the the state for serialization mode from the given store.
But I agree that in the implementation RestoreStateAsync doesn't do what you'd expect and indeed ,as you said, it relies on the fact that RestoreStateAsync always gets called before RegisterOnPersisting.

I found a way to fix it. I added GlobalSerializationMode in ISerializationModeHandler. I change the GlobalSerializationMode before I call RestoreStateAsync.
GlobalSerializationMode is also useful in the case when the code is running on the circuit(or webassembly runtime) and you try to register a callback on webassembly(server). The callback should not be fired. By checking the GlobalSerializationMode I throw the exception.

Thanks for the suggestion! I tried it out and it won't work.
The reason is that ComponentStatePersistenceManager.PersistStateAsync(store, serializationMode, dispatcher) persists the state for the provided serializationMode (Server or WebAssembly) into the provided store. We want to store server state and webassembly state into differrent stores.

Yeah, that makes sense - it looks like I didn't cover that detail in my initial comment here. I sketched out some of the ideas from my first comment as a commit, because code is probably a lot easier to parse than a list of bullet points 😄

What do you think? The goal with the ideas I'm proposing is to reduce the amount of code dedicated to tracking Server/WebAssembly-specific state. Feel free to borrow some of those ideas if you agree with them (or ignore them if you prefer what you have).

Your approach using a GlobalSerializationMode property is also valid - my only concern with that is it's more difficult to reason about how configuration of global state impacts the behavior of the program. Ideally, I think it would be nice to have most of the logic be agnostic to the specific serialization mode.

surayya-MS · 2023-08-29T15:07:28Z

src/Components/Components/src/Infrastructure/ComponentStatePersistenceManager.cs

-    public Task PersistStateAsync(IPersistentComponentStateStore store, Dispatcher dispatcher)
+    public Task PersistStateAsync(
+        IPersistentComponentStateStore store,
+        PersistedStateSerializationMode serializationMode,


I needed to add serialization mode as an argument here because we don't want to persist everything into one store. We want to persist server state to the protected store and webassembly state to the un-protected one.

If we take the approach mentioned in my other comment and went back to single _stateIsPersisted and _pauseCallbacks fields, then I think we should be able to get away without changing the API. One approach could be to use two temporary dictionaries in PersistStateAsync (one for Server, one for WebAssembly). Whatever dictionary is relevant to the current callback could get accessed by PersistentComponentState via a new internal property, similar to how the existing PersistingState property works.

If we want callbacks to run in parallel, we could replace the property with an AsyncLocal containing the dictionary for the current callback, although I'm not sure if the perf implications of that would be enough to dismiss that idea. An alternative is to group each callback by serialization mode and run callbacks with the same serialization mode in parallel.

Explained in this comment

I sketched out some of my ideas in f7a8552 to show how this could be achieved 🙂

surayya-MS · 2023-08-29T15:14:50Z

src/Components/Components/src/Infrastructure/ComponentStatePersistenceManager.cs

            {
                try
                {
-                    await task;
+                    await callback();


I made the execution of the callbacks sequential here. The reason is that PersistAsJson is usually provided in the callback. In order to persist the key, value in the correct dictionary PersistAsJson uses the SerializaionState property which can be set only here before foreach block. If I'd leave the execution of the callbacks parallel the SerializaionState can be set incorrectly.

I think running callbacks in parallel should still be possible, as long as we await the completion of all callbacks before mutating SerializationMode again.

If I'd leave the execution of the callbacks parallel the SerializaionState can be set incorrectly.

Do you remember why exactly this was happening?

You are right. Thanks! Changed the code to await the completion of all callbacks.

surayya-MS · 2023-08-29T15:19:32Z

src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.PrerenderingState.cs

            }
+
+            var commentPrefix = _serializationMode switch


Now there are 2 types of the comments for the persisted state.

surayya-MS · 2023-08-29T15:20:48Z

src/Components/Web.JS/src/Services/ComponentDescriptorDiscovery.ts

@@ -12,12 +12,21 @@ export function discoverComponents(root: Node, type: 'webassembly' | 'server' |
  }
 }

-const blazorStateCommentRegularExpression = /^\s*Blazor-Component-State:(?<state>[a-zA-Z0-9+/=]+)$/;
+const blazorServerStateCommentRegularExpression = /^\s*Blazor-Server-Component-State:(?<state>[a-zA-Z0-9+/=]+)$/;


We need to discover server and webassembly states.

surayya-MS · 2023-08-29T15:23:01Z

src/Components/Web.JS/src/Services/WebRootComponentManager.ts

@@ -67,6 +67,7 @@ export class WebRootComponentManager implements DescriptorHandler, NavigationEnh

  // Implements NavigationEnhancementCallbacks.
  public documentUpdated() {
+    this.updateApplicationState();


Updating the persisted state after the document is updated by calling new method WebRenderer.UpdateApplicationState

MackinnonBuck

Looks like a great start!

MackinnonBuck · 2023-08-29T18:24:28Z

src/Components/Components/src/PersistentComponentState.cs

-        _registeredCallbacks.Add(callback);
+        if (callback.Target is not IComponent)
+        {
+            throw new InvalidOperationException("Cannot infer serialization mode for non component. Provide a serialization mode.");


What happens when using a pure Blazor Server or Blazor WebAssembly application? Shouldn't it work to have a callback defined outside of a component, because we know the serialization mode will always be "Server" or "WebAssembly"?

MackinnonBuck · 2023-08-29T18:59:38Z

src/Components/Components/src/PersistentComponentState.cs

+            throw new InvalidOperationException("Cannot infer serialization mode for non component. Provide a serialization mode.");
+        }
+
+        var serializationMode = SerializationMode;


Hm, it's not super intuitive to me that we use RestoreStateAsync to define the SerializationMode for future reference in RegisterOnPersisting. It's giving RestoreStateAsync an additional responsibility that it doesn't look like it should have, and it relies on the fact that RestoreStateAsync always gets called before RegisterOnPersisting.

I'm guessing the reason you chose to do it this way is because previously, the serialization mode was only specified in PrerenderPersistedStateAsync, but this obviously gets called long after any calls to RegisterOnPersisting occur, making it impossible to infer the serialization mode up front. Is that correct?

Rather than attempting to infer the serialization mode as soon as RegisterOnPersisting gets called, could we do it at the time of persistence? That would allow us to remove the extra argument in RestoreStateAsync.

We might be able to use an approach like this:

Go back to a single _registeredCallbacks field, and change its type to something like List<OnPersistingCallback>, where OnPersistingCallback is a struct containing:

The Func<Task> callback itself

The PersistedStateSerializationMode passed to RegisterOnPersisting (or Infer if the overload without that argument is used).

Remove EndpointHtmlRenderer.Prerender{Server|WebAssembly}PersistedStateAsync() and instead call PrerenderPersistedStateAsync() with the Infer serialization mode from RazorComponentEndpointInvoker.

This will require changing some of the logic in EndpointHtmlRenderer.PrerenderingState.cs, but I think this may have been expected anyway, especially considering the existing "TODO: This will all have to change when we support multiple render modes in the same response" comment.

Resolve the serialization mode for each callback during ComponentStatePersistenceManager.PersistStateAsync:

If the serialization mode argument is Infer:

Resolve each callback's serialization mode to its mode in the OnPersistingCallback struct.

If a callback's serialization mode is also Infer, run your existing logic that infers based on the target component's render mode

If the serialization mode argument is Server or WebAssembly:

Then a OnPersistingCallback.SerializationMode of Infer resolves to the value of the serialization mode argument

But if the OnPersistingCallback.SerializationMode is not Infer, and it doesn't match the specified serialization mode, then throw

What do you think? Note that I haven't tried this out, and I'm sure you know this area better than I do at this point, so it's totally possible there's a detail I'm missing that makes this approach less desirable 🙂

MackinnonBuck · 2023-08-29T20:29:39Z

src/Components/Components/src/Infrastructure/ComponentStatePersistenceManager.cs

-    public Task PersistStateAsync(IPersistentComponentStateStore store, Dispatcher dispatcher)
+    public Task PersistStateAsync(
+        IPersistentComponentStateStore store,
+        PersistedStateSerializationMode serializationMode,


If we take the approach mentioned in my other comment and went back to single _stateIsPersisted and _pauseCallbacks fields, then I think we should be able to get away without changing the API. One approach could be to use two temporary dictionaries in PersistStateAsync (one for Server, one for WebAssembly). Whatever dictionary is relevant to the current callback could get accessed by PersistentComponentState via a new internal property, similar to how the existing PersistingState property works.

If we want callbacks to run in parallel, we could replace the property with an AsyncLocal containing the dictionary for the current callback, although I'm not sure if the perf implications of that would be enough to dismiss that idea. An alternative is to group each callback by serialization mode and run callbacks with the same serialization mode in parallel.

MackinnonBuck · 2023-08-29T21:03:24Z

src/Components/Components/src/Infrastructure/ComponentStatePersistenceManager.cs

            {
                try
                {
-                    await task;
+                    await callback();


I think running callbacks in parallel should still be possible, as long as we await the completion of all callbacks before mutating SerializationMode again.

If I'd leave the execution of the callbacks parallel the SerializaionState can be set incorrectly.

Do you remember why exactly this was happening?

MackinnonBuck · 2023-08-29T21:05:54Z

src/Components/Components/src/PersistentComponentState.cs

@@ -46,9 +55,43 @@ public PersistingComponentStateSubscription RegisterOnPersisting(Func<Task> call
    {
        ArgumentNullException.ThrowIfNull(callback);

-        _registeredCallbacks.Add(callback);
+        if (callback.Target is not IComponent)


Really cool idea!!

@javiercn's idea

src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Prerendering.cs

MackinnonBuck · 2023-08-29T21:27:44Z

src/Components/Web.JS/src/Services/WebRootComponentManager.ts

+    private updateApplicationState() {
+      if (isRendererAttached(WebRendererId.Server)) {
+        const serverAppState = discoverServerPersistedState(document);
+        if (serverAppState) {
+            updateApplicationState(WebRendererId.Server, serverAppState);
+        }
+      }   
+      if (isRendererAttached(WebRendererId.WebAssembly)) {
+        const webAssemblyAppState = discoverWebAssemblyPersistedState(document);
+        if (webAssemblyAppState) {
+            updateApplicationState(WebRendererId.WebAssembly, webAssemblyAppState);
+        }
+      }  
+    }


I wonder if there are things that we can do to avoid scanning the entire DOM each time the document receives an SSR update.

With component comments, we scan the incoming DOM before it gets merged with the document. That allows us to only scan the subset of the document that gets changed. Could we consider a similar approach here?

Agree. Working on it

javiercn

This is a step in the good direction. However, I am not sure this design correctly aligns with the discussions that we previously had.

It's unclear to me how this design works when there are server and Webassembly components running on the same app.

There is a lack of E2E tests on the PR that make it hard to understand/validate what scenarios work and how.

It's unclear to me why we need an ISerializationModeHandler abstraction, when this was something that we were able to handle completely internally in the past.

I am not grocking the GlobalSerializationMode. In my head there is no such concept, as the serialization mode is local to the component render mode or service.

Server render mode component -> Server
Webassembly render mode component -> Webassembly.
Auto render mode component -> Server and webassembly.
DI service when only webassembly is enabled -> Webassembly
DI service when only server is enabled -> Server
DI service when both server and webassembly is enabled -> Server.

Explicit serialization options:

ServerOnly
WebAssemblyOnly
All

Overall, I think that the serialization/deserialization should be kept internal to the EndpointHtmlRenderer.

The only thing that ComponentState needs to know is the render mode (if any) for a given callback, which is something that the renderer can provide.

If it doesn't know the render mode, it can throw or it can fallback to something safe (like Server), and we can provide an option to change the default (we can even do this on a per-type basis).

javiercn · 2023-09-05T11:05:24Z

src/Components/Components/src/ISerializationModeHandler.cs

+/// <summary>
+/// A service that can infer <see cref="IComponent"/>'s <see cref="PersistedStateSerializationMode"/>.
+/// </summary>
+public interface ISerializationModeHandler


Why do we need this interface? Can this be done in a different way?

The reason for creating this interface is that it was impossible to reference EndpointHtmlRenderer in PersistentComponentState. Alternative solution was to add some render mode specific absctract methods to the Renderer and then override them in EndpointHtmlRenderer which didn't feel right.

The reason for creating this interface is that it was impossible to reference EndpointHtmlRenderer in PersistentComponentState.

Have you checked-out Steve's spike? It doesn't require those.

Alternative solution was to add some render mode specific absctract methods to the Renderer and then override them in EndpointHtmlRenderer which didn't feel right.

That sounds desirable over a separate interface that needs to be implemented in many places.

ISerializationModeHandler also solves the issue when the code start to run on the circuit or webassembly runtime. When this happens render mode for the components is not available. The reason is that circuit/webassembly runtime uses it own scope and the Renderer's dictionary for component states is not populated. More about this in this comment

javiercn · 2023-09-05T11:06:04Z

src/Components/Components/src/PersistentComponentState.cs

@@ -13,26 +13,36 @@ namespace Microsoft.AspNetCore.Components;
 public class PersistentComponentState
 {
    private IDictionary<string, byte[]>? _existingState;


Why do we need _existingState anymore?

We need _existingState for restoring the application state after emitting the comment containing the application state into html page.

javiercn · 2023-09-05T11:06:47Z

src/Components/Components/src/PersistentComponentState.cs

    }

    internal bool PersistingState { get; set; }

+    internal PersistedStateSerializationMode CurrentSerializationMode { get; set; } = PersistedStateSerializationMode.Infer;


Why is the default "infer"?

It doesn't need to be. I'll remove this. Thanks for noting!

javiercn · 2023-09-05T11:08:10Z

src/Components/Components/src/PersistentComponentState.cs

+        ArgumentNullException.ThrowIfNull(callback);
+
+        if (_serializationModeHandler.GlobalSerializationMode != PersistedStateSerializationMode.Infer &&
+            serializationMode != _serializationModeHandler.GlobalSerializationMode)


Can you enumerate the possible combinations for the different use cases? It's not clear how this works E2E

This means "Unless the GlobalSerializationMode is not infer you cannot persist state for serializationMode different that the GlobalSerializationMode".
I'll provide a clarifying comment, since it raised a question.

javiercn · 2023-09-05T11:10:10Z

src/Components/Components/src/PersistentComponentState.cs

+        {
+            PersistedStateSerializationMode.Server => _currentServerState,
+            PersistedStateSerializationMode.WebAssembly => _currentWebAssemblyState,
+            _ => throw new InvalidOperationException("Invalid persistence mode.")


How does this work with AutoMode? One of the key points of the design was that calls to PersistAsJson still work without specifying the "serialization mode" when the callback comes from a component.

How does that work in this scenario? It's throwing isn't it?

Haven't tried this scenario yet.

javiercn · 2023-09-05T11:10:44Z

src/Components/Components/src/PersistentComponentState.cs

+            PersistedStateSerializationMode.WebAssembly => _currentWebAssemblyState,
+            _ => throw new InvalidOperationException("Invalid persistence mode.")
+        };
+
        ArgumentNullException.ThrowIfNull(key);


This should happen before any other check

javiercn · 2023-09-05T11:12:20Z

src/Components/WebAssembly/WebAssembly/src/Services/WebAssemblySerializationModeHandler.cs

+    }
+
+    public PersistedStateSerializationMode GetCallbackTargetSerializationMode(object? callbackTarget)
+        => PersistedStateSerializationMode.WebAssembly;


I'm failing to understand what this is doing if it's just returning a constant instead of looking at the callback target.

Well, short answer to your question is because for webassembly the callback target can only be PersistedStateSerializationMode.WebAssembly.

I guess the confusing part is why there are 3 ISerializationModeHandlers: EndpointHtmlRenderer, ServerSerializationModeHandler, WebAssemblySerializationModeHandler (actually now listing them, I realised it might be better to add EndpointSerializationModeHandler and use EndpointHtmlRenderer inside of it).

EndpointHtmlRenderer
GetCallbackTargetSerializationMode checks if the callback target is a component and then gets the component's render mode.
However this doesn't work on the circuit because the component's render mode is not available on the circuit. The reason is that the circuit doesn't use the same instance of PersistentComponentState and ISerializationModeHandlers, it has its own scope. The circuit uses same exact kinds of services listed in AddRazorComponents though.
Hence, there is a need to be able to change the GlobalSerializationMode to Server when creating the circuit host here.

WebAssemblySerializationModeHandler
The webassembly host uses the services provided here.
Thant is why I created WebAssemblySerializationModeHandler that will just return constant PersistedStateSerializationMode.WebAssembly and throws when trying to set GlobalSerializationMode.

ServerSerializationModeHandler
This one is for pure Blazor Server apps. This is useful for cases when calling PersistentComponentState.RegisterOnPersisting for non-components. You wouldn't have to provide Server serialization mode every time with this.
I baked it here before AddRazorComponents so it would override EndpointHtmlRenderer. This seemed like a right place after following AddRazorPages.

Hope this answers your question.

javiercn · 2023-09-05T11:27:06Z

src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.PrerenderingState.cs

@@ -15,6 +15,16 @@ internal partial class EndpointHtmlRenderer
 {
    private static readonly object InvokedRenderModesKey = new object();

+    public async ValueTask<IHtmlContent> PrerenderServerPersistedStateAsync()


Why do we need to go through IHtmlContent for this? Seems like unnecessary overhead.

I don't think I understand your question, why do you think this might be unnecessary overhead?
IHtmlContent is then used to emit a comment containing state in [RazorComponentEndpointInvoker]
(https://github.com/dotnet/aspnetcore/pull/50373/files#diff-b891c4aa1da07f8709ffc51ce6c26c489d9432866aee1cdac187484377dbfefeR124-R129).
This is similar to the existing ValueTask IHtmlContent PrerenderPersistedStateAsync(HttpContext httpContext, PersistedStateSerializationMode serializationMode)

Couldn't we just write directly to the response?

SteveSandersonMS · 2023-09-05T13:16:21Z

Great to see this, @surayya-MS!

I know you might have an excess of review feedback by now since @MackinnonBuck and @javiercn have already given comments, so I'll try to keep this super focused!

My main feedback is that I would recommend taking an approach like @MackinnonBuck described here, though maybe with a few extra tweaks to minimize new public API. That, is:

Have RegisterOnPersisting take an extra optional IComponentRenderMode and store this inside PersistentComponentState's _registeredCallbacks
Also have ComponentStatePersistenceManager.PersistStateAsync take an optional extra "filter" parameter, e.g., of type public delegate bool PersistentStateCallbackFilter(object? target, IComponentRenderMode? renderMode);
- It can get the IComponentRenderMode from _registeredCallbacks, and target from Callback.Target
Then EndpointHtmlRenderer's PrerenderPersistedStateAsync can have two overloads:
- One that still takes a PersistedStateSerializationMode and has the same logic as before. The "filter" parameter can just return true for everything, since PersistedStateSerializationMode controls everything. This continues to be used for back-compat by the <persist-component-state> tag helper.
- A new one that doesn't take PrerenderPersistedStateAsync, but instead serialized both server and webassembly state by using two different stores, and "filter" callbacks that do something like:
  - If there's a renderMode on the registration, match based on that
  - If not, but if target is a component, look up the effective rendermode for target and match based on that
  - Else just omit the value completely (or, equally good, if we're serializing server state, include nonmatched items by default)
  ... then you'll have populated two stores, one for server and one for WebAssembly, and can return an IHtmlContent that emits tags for either/both based on which ones have content.

This is almost the same as Mackinnon's suggestion except it leaves the renderer in control of how to interpret IComponentRenderMode and avoids baking in the concept of "server vs WebAssembly" at the M.A.Components level which is meant to be agnostic to those concepts. This also eliminates the need for a "global serialization mode" concept.

I did mock up this approach and it appeared to work (let me know if it's useful to see that), but I know you've dug deeper into this area already so apologies if I'm missing some details.

SteveSandersonMS · 2023-09-05T13:20:19Z

src/Components/Components/src/PersistedStateSerializationMode.cs

@@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.Components;
 public enum PersistedStateSerializationMode
 {
    /// <summary>
-    /// Indicates that the serialization mode should be inferred from the current request context.
+    /// Indicates that the serialization mode should be inferred.
    /// </summary>
    Infer = 1,



I think we can and should avoid baking in the "server vs webassembly" distinction at this level - see #50373 (comment)

2. Fixed PrerenderPersistedStateAsync 3. Moved the check _stateIsPersisted from ComponentStatePersistenceManager to the store 4. Made possible to change ISerializationModeHandler to Server one for circuit

surayya-MS · 2023-09-07T18:53:14Z

@MackinnonBuck, thanks for taking time to code your ideas! It is much easier. I really like that the PersistentComponentState and ComponentStatePersistenceManager look much cleaner! Also, cool idea to add SupportsSerializationMode to IPersistentComponentStateStore! I included your changes. Thanks a lot!

However, I had to change a few things.

When code runs on the circuit there is no serialization mode available. Same problem as before with adding extra serializationMode to RestoreAsync or changing GlobalSerializationMode. This time I added ComponentStatePersistenceManager.SetSerializationModeHandler. I change the ISerializationMode to ServerSerializationMode when creating the circuit host here.
Also I'm concerned about the case when the code runs on circuit(or webassembly) and someone registers a callback with webassembly(or server) serialization mode. With the current design we don't throw exeption. The callback is just never fired. Are we okay with this @SteveSandersonMS , @javiercn ? With the GlobalSerializationMode design it was possible. Bringing it back also doesn't make sense since it will bring back the complexity of the serialization mode back to PersistenComponentState. I'm fine with the current code and to throw an exception. We should just mention this in the docs.
Inferring the serialization mode from InvokedRenderModes doesn't work. It always InvokedRenderModes.Mode.None. For that reason I added PersistedStateSerializationMode.ServerAndWebAssembly.
This change makes the code shorter in case when someone wants to register the callback for both server and webassembly. Also had to throw exception when serialization mode is infer.
The exception is thrown saying that the state is already persisted. This happens when ComponentStatePersistenceManager.PersistStateAsync is called for the second time. For this reason I moved this check inside the PrerenderComponentApplicationStore.PersistStateAsync

javiercn

Thanks for pushing through this @surayya-MS. I think we still need to iron out some of the details in the design/implementation. Specifically:

We shouldn't need to add an ISerializationModeHandler public API bit to deal with inferring the persistence. ComponentStatePersistenceManager already has access to the renderer and can use that (though an internal method) to get the render mode for the component.
UpdateApplicationState shouldn't need to be public. We've always restored the state for the renderer in the past in a private manner, and I don't see a reason why that needs to change.

Lastly, it's really hard to further evaluate the PR without the E2E scenarios that showcase the different scenarios and how they work, so I would strongly encourage you to add E2E scenarios.

javiercn · 2023-09-08T13:07:00Z

src/Components/Components/src/ISerializationModeHandler.cs

+/// <summary>
+/// A service that can infer <see cref="IComponent"/>'s <see cref="PersistedStateSerializationMode"/>.
+/// </summary>
+public interface ISerializationModeHandler


The reason for creating this interface is that it was impossible to reference EndpointHtmlRenderer in PersistentComponentState.

Have you checked-out Steve's spike? It doesn't require those.

Alternative solution was to add some render mode specific absctract methods to the Renderer and then override them in EndpointHtmlRenderer which didn't feel right.

That sounds desirable over a separate interface that needs to be implemented in many places.

javiercn · 2023-09-08T13:12:40Z

src/Components/Components/src/Infrastructure/ComponentStatePersistenceManager.cs

+            State.PersistenceContext = new(currentState);
+            await PauseAsync(store);
+            State.PersistenceContext = default;


We can avoid all this if we do things a bit differently.

Collect or infer the mode inside the call to register for each callback.

Persist components with Server mode in a batch.

Persist components with Webassembly mode afterwards.

javiercn · 2023-09-08T13:15:48Z

src/Components/Components/src/PersistenceCallback.cs

+
+namespace Microsoft.AspNetCore.Components;
+
+internal readonly record struct PersistenceCallback(Func<Task> Callback, PersistedStateSerializationMode SerializationMode);


This is only used inside PersistentComponentState, it's better to nest it inside that type. Having it as a top-level type adds unnecessary noise to the Microsoft.AspNetCore.Components namespace.

javiercn · 2023-09-08T13:17:53Z

src/Components/Components/src/PersistenceContext.cs

+
+namespace Microsoft.AspNetCore.Components;
+
+internal readonly record struct PersistenceContext(IDictionary<string, byte[]> State);


Same comment as above, these types that are used just for structured data should be inside the class that uses them. You can also dispense creating a separate type when all you are holding is a single piece of data.

javiercn · 2023-09-08T13:19:43Z

src/Components/Components/src/PersistentComponentState.cs

+    /// <param name="callback">The callback to invoke when the application is being paused.</param>
+    /// <param name="serializationMode">The <see cref="PersistedStateSerializationMode"/> to register the callback.</param>
+    /// <returns>A subscription that can be used to unregister the callback when disposed.</returns>
+    public PersistingComponentStateSubscription RegisterOnPersisting(Func<Task> callback, PersistedStateSerializationMode serializationMode)


This should probably be IComponentRenderMode instead of PersistedStateSerializationMode since that's what we are telling users to use.

javiercn · 2023-09-08T13:26:48Z

src/Components/Endpoints/src/DependencyInjection/RazorComponentsServiceCollectionExtensions.cs

@@ -54,6 +54,7 @@ public static IRazorComponentsBuilder AddRazorComponents(this IServiceCollection
        services.TryAddSingleton<WebAssemblyComponentSerializer>();
        services.TryAddScoped<EndpointHtmlRenderer>();
        services.TryAddScoped<IComponentPrerenderer>(services => services.GetRequiredService<EndpointHtmlRenderer>());
+        services.TryAddScoped<ISerializationModeHandler>(services => services.GetRequiredService<EndpointHtmlRenderer>());


This is where the issue is. There is a hidden circular dependency here.

Renderer -> PersistentComponentStateManager -> Renderer(ISerializationModeHandler).

The thing that initiates the persistence process (the renderer) calls the ComponentPersistenceManager and has access to the renderer, which in turn can use that to determine the correct render mode

aspnetcore/src/Components/Components/src/Infrastructure/ComponentStatePersistenceManager.cs

Line 50 in 8eaf4b5

public Task PersistStateAsync(IPersistentComponentStateStore store, Renderer renderer)

javiercn · 2023-09-08T13:29:35Z

src/Components/Web/src/PublicAPI.Unshipped.txt

@@ -121,6 +121,7 @@ static Microsoft.AspNetCore.Components.Web.RenderMode.WebAssembly.get -> Microso
 virtual Microsoft.AspNetCore.Components.HtmlRendering.Infrastructure.StaticHtmlRenderer.RenderChildComponent(System.IO.TextWriter! output, ref Microsoft.AspNetCore.Components.RenderTree.RenderTreeFrame componentFrame) -> void
 virtual Microsoft.AspNetCore.Components.HtmlRendering.Infrastructure.StaticHtmlRenderer.WriteComponentHtml(int componentId, System.IO.TextWriter! output) -> void
 virtual Microsoft.AspNetCore.Components.RenderTree.WebRenderer.GetWebRendererId() -> int
+virtual Microsoft.AspNetCore.Components.RenderTree.WebRenderer.UpdateApplicationState(string! applicationState) -> System.Threading.Tasks.Task!


Why is this needed at all? Previous versions didn't require it, and I don't see why this would be something that is required now.

With the new blazor web we prerender every page. This allows to update the application state. We discussed it with Mackinnon and Steve on the email thread

Can't find the email thread, can you point me to it?

forwarded the email to you

surayya-MS · 2023-09-08T15:00:04Z

UpdateApplicationState shouldn't need to be public. We've always restored the state for the renderer in the past in a private manner, and I don't see a reason why that needs to change.

@javiercn, we restored the state in the past in a private manner when creating circuit host/wasm host. When the host already exists there needs to be a way to update the application state from the js side. UpdateApplication state solves that

javiercn · 2023-09-08T15:03:03Z

@javiercn, we restored the state in the past in a private manner when creating circuit host/wasm host. When the host already exists there needs to be a way to update the application state from the js side. UpdateApplication state solves that

That doesn't mean there needs to be a public API to do it.

surayya-MS added 5 commits August 28, 2023 17:22

do not clear store when circuit is initialized

1c416bb

provide a way to persist data for Blazor web

576529b

small fix

fb8ef7e

fix tests

1d6a1e7

update application state when document is updated

a84bd39

ghost added area-blazor Includes: Blazor, Razor Components labels Aug 28, 2023

surayya-MS added 4 commits August 28, 2023 17:45

provide summaries

8d10aff

merge from main

4ac7ec3

small fixes

ecbad34

fix updating persisted state when document is updated

8d36369

surayya-MS commented Aug 29, 2023

View reviewed changes

surayya-MS requested review from SteveSandersonMS, javiercn and MackinnonBuck August 29, 2023 15:24

MackinnonBuck reviewed Aug 29, 2023

View reviewed changes

surayya-MS added 6 commits September 1, 2023 09:34

fire callbacks in parallel

b84048e

chek for a component in the interface method

f002b24

fix tests

5af5309

changed ISerializationModeHandler

e48cd1c

implemented only server and only webassembly ISerializationModeHandler

8071ac2

fire the callbacks in parallel

4b71007

surayya-MS requested a review from MackinnonBuck September 1, 2023 15:35

surayya-MS added 3 commits September 1, 2023 17:59

merge from main

ddcabf0

small fixes

f13b535

fix tests

7bbfde0

javiercn reviewed Sep 5, 2023

View reviewed changes

SteveSandersonMS reviewed Sep 5, 2023

View reviewed changes

MackinnonBuck and others added 4 commits September 7, 2023 16:11

Some ideas

414b461

fix PublicAPI

87fee27

1. Add ServerAndWebAssembly PersistedStateSerializationMode

1e5de57

2. Fixed PrerenderPersistedStateAsync 3. Moved the check _stateIsPersisted from ComponentStatePersistenceManager to the store 4. Made possible to change ISerializationModeHandler to Server one for circuit

fix

bbf34b0

javiercn reviewed Sep 8, 2023

View reviewed changes

surayya-MS closed this Sep 12, 2023

surayya-MS deleted the applicationState branch November 27, 2023 16:02

		@@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.Components;
		public enum PersistedStateSerializationMode


		namespace Microsoft.AspNetCore.Components;

		internal readonly record struct PersistenceCallback(Func<Task> Callback, PersistedStateSerializationMode SerializationMode);

Persist Prerendered State #50373

Persist Prerendered State #50373

Uh oh!

Conversation

surayya-MS commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Persist Prerendered State

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surayya-MS Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surayya-MS Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MackinnonBuck Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surayya-MS Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MackinnonBuck Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MackinnonBuck Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surayya-MS Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MackinnonBuck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MackinnonBuck Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MackinnonBuck Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MackinnonBuck Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surayya-MS commented Aug 28, 2023 •

edited

Loading

surayya-MS Sep 1, 2023 •

edited

Loading

surayya-MS Aug 29, 2023 •

edited

Loading

MackinnonBuck Aug 29, 2023 •

edited

Loading

surayya-MS Aug 29, 2023 •

edited

Loading

MackinnonBuck Aug 29, 2023 •

edited

Loading

MackinnonBuck Aug 29, 2023 •

edited

Loading

surayya-MS Sep 1, 2023 •

edited

Loading

MackinnonBuck Aug 29, 2023 •

edited

Loading

MackinnonBuck Aug 29, 2023 •

edited

Loading

MackinnonBuck Aug 29, 2023 •

edited

Loading

surayya-MS Sep 8, 2023 •

edited

Loading