Skip to content
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

Possible fix for .NET 8 Blazor Web App breakage #481

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

WhitWaldo
Copy link

Adds .NET 8 as a valid target in addition to .NET 7 and the other versions.

Also moves state initialization out from OnAfterRenderAsync (which doesn't run on server anymore per the docs) to OnInitializedAsync which runs after existing logic in OnInitialized and after OnParametersSet (which in .NET 8 now runs before OnInitialized{Async}), so it's the last lifecycle method before OnAfterRender, assuming it were still being called.

I'm having problems getting Fluxor.Blazor.Web to build on my machine (likely missing one of your targets) so I haven't yet tested this, but as covered in the related issue,, when I set a breakpoint in the ActionDispatched method, I can see that the actions are accumulating in the Store and they don't dequeue because the store never sets HasActivatedStore to true, which only happens via this call to Store.InitializeAsync() which only happens in OnAfterRenderAsync which isn't called on the server anymore, so it intuitively feels like this might fix the problem.

I'll follow up if I'm able to validate it locally furhter.

@WhitWaldo
Copy link
Author

WhitWaldo commented Mar 8, 2024

There's unfortunately more that needs tackling here. I built a local version of Fluxor that only target .NET 8 to simplify this and it builds fine now, but while the Counter page loads without error when @rendermode InteractiveServer and I see the DequeueActions method trigger as expected, when I swap this out with @rendermode InteractiveAuto, now I'm getting a DI-related exception:

Error: One or more errors occurred. (Cannot provide a value for property 'State' on type 'Blazor8Test.Client.Pages.Counter'. There is no registered service of type 'Fluxor.IState`1[Shared.State.CounterUseCase.CounterState]'.)

This is odd since I'm registering all dependencies on the primary project so it should flow through to the client (per the guidance here) but this remains a work in progress.

@mrpmorris
Copy link
Owner

Fancy pairing on this one if the days?

I'm available from 5PM week days (UK time)

@WhitWaldo
Copy link
Author

WhitWaldo commented Mar 8, 2024

Sure - we can sync on that next week. I'm in the US, but if I did the math right, that puts me at 11 AM or later, so that's more than fine.

I've identified at least part of the problem.

In .NET 7, you have either a server or a WASM project. You register Fluxor via the dependency injection scheme and initialize the store by placing the <StoreInitializer> component at the top of what was then the App.razor file and the rest works like magic.

In .NET 8, you cannot quite do this because as the library author, you're not going to know what render mode the user is opting for as it can be done on a per-page/component basis instead of globally, so you've got to be a bit more flexible about how the service is initialized.

On the Server project, the <StoreInitializer> should be placed at the top of the Routes.razor file as App.razor is now what was the _Host.cshtml file. Both the server and the client need to perform the Fluxor DI registration, so this should be either put in a shared project or placed in the client app and called by both. Coupled these two with the change I made to initialize the store in OnInitializedAsync instead of in OnAfterRender to avoid conflicts with any pre-rendering.

However, on the client, we've got several limitations to consider:

  • There's no such thing as scoped DI registrations - all scoped registrations are just singleton registrations.
  • There's no "globally run" file like that of Routes.razor to run initialization off of.

As such, on the Client project, in addition to the local Fluxor DI registration mentioned above, I propose that because SetParametersAsync now runs before OnInitializedAsync in the component lifecycle, the FluxorComponent be updated to override OnParametersSetAsync with the following:

        /// <summary>
        /// Method invoked when the component has received parameters from its parent in
        /// the render tree, and the incoming values have been assigned to properties.
        /// </summary>
        /// <returns>A <see cref="T:System.Threading.Tasks.Task" /> representing any asynchronous operation.</returns>
        protected override async Task OnParametersSetAsync()
      {
	    await base.OnParametersSetAsync();

            //Attempt to initialize the store knowing that if it's already been initialized, this won't do anything.
            await Store.InitializeAsync();
        }

As the Store has a flag checking if it's already been initialized, if this runs and that flag is true, nothing will happen, but because all Fluxor components are expected to inherit a FluxorComponent this will ensure that the store is initialized and starts to dequeue pending actions as expected.

There's no need to change the FluxorOptions to prefer a Singleton over a Scoped registration (as the Client is going to treat is as a Singleton anyway). There's also no need to disable prerender to make this work. I've tested that it works on interactive auto whether prerender is enabled or not.

I've tested this several times and it now works precisely as expected on both the Server and, after waiting for it to load the WASM bundle, switching pages and switching back to validate it's disconnected from the server, successfully verified it works on the Client as well. I'll update the PR shortly with my proposed changes.

Remaining open work:

  • Might be worth rethinking what needs to happen in StoreInitializer as it's only running on the server and not the client project. I might suggest tentatively shoveling that into a singleton service with a HasInitializedFlag that's called from any component implementing FluxorComponent and then recommending that best practices be that all Fluxor-capable pages/components inherit from FluxorComponent instead of making it semi-optional as today so that the experience is identical on both server and client projects.
  • In the same vein, JS middlewares only register on the server since they're not initialized on the client.
  • Because the registration in server and client differ and there's no other way I've identified to share the state between the different contexts, I think this is going to require some sort of persistence/rehydration layer not unlike what redux-persist and redux-store achieve. I'm working on a proof of concept that at least demonstrates this idea working.

@WhitWaldo
Copy link
Author

I've added a single persistence layer to Fluxor that appears to resolve the cross-rendermode issue. As observed by others on another thread about this, it's one thing to get Fluxor up and running again, but a whole other thing to completely lose your state as soon as you jump render modes (e.g. server -> web assembly).

While I've taken a stab at implementing this in a lightweight manner, it wasn't until I was integrating the changes from my test project to this branch that I realized my solution is only compatible with .NET 6 and higher because of my use of JsonNode in System.Text.Json, so I surrounded the relevant registration and implementation bits with conditional compiler flags in the shorter term. Also, I cannot get Fluxor to build on my machine as it's targeting frameworks I simply don't have installed. If you've got .NET 8 installed, clone my repo below for a .NET 8-only version of this.

A .NET 8 version of this can be seen on the latest commit here, but I'll take a moment to walk through the idea.

I built off the idea of using an action to identify when the state has initialized and an effect that triggers from that. I've added some extensions to the registration method that allows the developer to opt into the persistence feature and provide an implementation of IPersistenceManager (I wrote such an implementation in my other project using session storage here) and to allow them to register any additional dependencies here that it might require. If IPersistenceManager is never registered, Fluxor runs as it did before this implementation and that's it (with a few minor tweaks).

If it is registered, when Fluxor is activating the store instance, it first marks the store as activated, dequeues any actions and then dispatches a new StoreRehydratingAction that kicks off a registered effect that reads any persisted state from the IPersistenceManager and uses a new method on the store to rehyrdrate the state on a per-feature basis (using that RestoreState method that explicitly says it shouldn't be used for this - whoops). This concludes with a StoreRehydratedAction in case anyone wants to subscribe to it for some reason.

On a location change event in the Fluxor component, it dispatches a PersistingStoreAction and this does the opposite. It builds a JSON object (each feature to a separate named key) and persists this via the IPersistenceManager and fires off a PersistedStoreAction.

This implementation avoids having the project take on any other package dependencies and leaves it to the developer to decide where and how they want to persist any data, whether that be in session state as my demo does or bind it to a session ID and save it somewhere on the server - their choice. Just implement the IPersistenceManager and register with the provided method on FluxorOptions.

Remaining work:

  • Again, this is only compatible with .NET 6, .NET 7 and .NET 8 because of the use of System.Text.Json for building the JsonNode during the serialization and deserialization steps. I imagine the same can be done with Newtonsoft.Json, if you want to take on another dependency.
  • I see some flicker in WebAssembly where it's initializing the store, then reading the state and updating it, so this could be improved on.
  • I thought persisting it on location change is often enough (wouldn't want to do it every single action) as I don't know that Blazor jumps render mode until you actually change the page. In all my testing, it always required a page change to swap modes. If that changes in a future release, perhaps they'll give us an event to help out. But if there's some notification of this or users want to save more frequently, this approach will need to be tweaked.
  • I save to the browser's session store in my demo so that when I relaunch the app, it doesn't just load the state from the previous session. I wasn't able to find any sort of common value (for non-authenticated users) that give me some sort of identifier indicating that WASM-session X matched Server-session X (so that if X didn't match, it would trigger a clear of the store). I added a method to IPersistenceManager so this is covered in a server-side store scenario, so perhaps this can be improved on.

@GordonBeeming
Copy link

@WhitWaldo thanks for the effort in getting this working ❤️

@mrpmorris

  1. Would this fix make it into 5.9.x or only 6.0?
  2. If not, do you know roughly when you are wanting 6.0 to reach stable?

@mrpmorris
Copy link
Owner

It'll be in 6.0

I will be releasing Beta versions, but not until they are stable so should be okay to use.

@GordonBeeming
Copy link

Great stuff, thanks @mrpmorris 🙂

@WhitWaldo
Copy link
Author

I've made a few changes to the PR and also updated my .NET 8 demonstration to reflect them. As before, I can't actually build this locally as I'm missing some of these older .NET SDKs, but I copy/pasted the changed pieces from my demo, so I'm fairly confident it should work.

  • As before, this is still dependent on .NET 6 and later because of the System.Text.Json dependency. My latest work doesn't change this at all.
  • The earlier version relied on the OnAfterRenderAsync method for triggering rehydration as the demonstration showed compatibility with IJSRuntime to persist the state to the user's browser via local or session state. I've since switched to use OnInitializedAsync so the rehyrdration occurs before it's rendered, eliminating the flicker. This does mean that this cannot be used with local/session storage any longer. My demo instead simulates a server-side Redis instance via a singleton service maintaining the value.
  • The earlier version also relied on NavigationManager to be the impetus triggering the state persistence. After some experimentation with PersistentComponentState, I (re-)discovered that the render mode occurs on a per-component basis and as those aren't pages, they wouldn't necessarily trigger a location change event on the NavigationManager. While I briefly looked at using PersistentComponentState to store the Store state, it is only able to write to the state whenever the event callback is being invoked indicating a render mode change and thus isn't suitable for more frequent writes. Rather, persistence now happens much more often - any time there's at least one action in the queue that's not one of the internal Fluxor system actions (e.g. rehydration, persistence, initialization), it will add an OnPersistingStateAction to the end of the queue ensuring that regardless of the component engaging with the State, all user-supplied actions are committed.
  • I couldn't find anything in Fluxor that specifically keeps track of whether or not the Feature states are actually updated as a result of any of the actions' downstream effects - if there's such a flag, that could be another useful gate to limit unnecessary state persistence operations so it only runs when the state was actually modified (which, given Reducers, will likely happen often anyway - so the value here outside of a demo might be negligible).
  • My demo is a bit more complex to reflect the real-world scenario that context running in web assembly will need to connect remotely to the server to get/set the current state whereas the server can simply inject the service it's using. Put simply, both the server and client implement an IStateService that's registered to a separate implementation for either project. On the server, this is a singleton that could just as easily be replaced with a Redis connection. On the client, this injects an IHttpClientFactory, creates the HttpClient and calls either a get or set method on a controller exposed by the server that itself injects its local IStateService (that singleton previously discussed). All of this will need to be added by the user (but would be anyway, because of the nature of the InteractiveAuto render mode) and isn't part of the Fluxor project. Rather, Fluxor, as before, simply requires that if Persistence is enabled, something that implements IPersistenceManager must be provided during the DI registration. Here, this injects an IStateService meaning that it gets either of the above implementation depending on the render mode it's using to perform that basic get/set operation, which again, happens at OnInitializedAsync as part of the state initialization, so entirely pre-empting any flicker on the page.

At this point, this is working well enough for my purposes that I don't know any other active work is really necessary unless you really want to replace the System.Text.Json dependency for something compatible with the broader set of targets you're looking to support.

I don't see a branch that specifically identifies as the 6.0 release, but if you can point me in the right direction, I'd be happy to apply my changes to that branch as well in a separate PR.

@@ -15,9 +15,11 @@
</ItemGroup>

<ItemGroup>
<PackageReference Condition="'$(TargetFramework)' == 'net8.0'" Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="8.0.0" />
<PackageReference Condition="'$(TargetFramework)' == 'net7.0'" Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="7.0.0" />
<PackageReference Condition="'$(TargetFramework)' == 'net6.0'" Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="6.0.0" />
<PackageReference Condition="'$(TargetFramework)' == 'net5.0'" Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="5.0.0" />
<PackageReference Condition="'$(TargetFramework)' != 'net7.0' AND '$(TargetFramework)' != 'net6.0' AND '$(TargetFramework)' != 'net5.0'" Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="3.1.0" />
Copy link
Contributor

@jafin jafin Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Microsoft.Extensions.DependencyInjection.Abstractions" Version="3.1.0 does this need to be removed or a condition added for != 'net8.0 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather assumed that whatever was there was compatible up to the current supported build prior to my adding .NET 8 to the list - presumably that includes .NET Core 3.1 as-is if that's still a supported target, so I wouldn't think anything else is necessary.

@@ -14,6 +21,10 @@ public class Store : IStore, IActionSubscriber, IDisposable
/// <see cref="IStore.Initialized"/>
public Task Initialized => InitializedCompletionSource.Task;

#if NET6_0_OR_GREATER
private readonly IPersistenceManager? _persistenceManager;
Copy link
Contributor

@jafin jafin Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider following naming convention of project.? Uppercase vs leading underscore for privates.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer to the maintainer's preference here.

jafin pushed a commit to jafin/Fluxor that referenced this pull request Apr 16, 2024
@mikeppcom
Copy link

mikeppcom commented Apr 23, 2024

Will this PR be in beta#3/V6? We make heavy use of Fluxor and are moving to a .NET8 hybrid Blazor server/client render model and are facing the issue that actions are no longer being dispatched.

@wave9d
Copy link

wave9d commented May 29, 2024

Will this PR be in beta#3/V6? We make heavy use of Fluxor and are moving to a .NET8 hybrid Blazor server/client render model and are facing the issue that actions are no longer being dispatched.

Any news on this PR being available for a beta release ? We're in the same situation as @mikeppcom

@mrpmorris
Copy link
Owner

try specifying the @rendermode in Routes.razor or on the <StoreInitializer compoonent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants