torhovland / blazor-redux

Connecting a Redux state store with Blazor.
Other
481 stars 48 forks source link

First stab at middleware #9

Closed chanan closed 4 years ago

chanan commented 6 years ago

This is a first stab at middleware.

You can see I created a class called Logger - it does get the state and action but doesn't print them out yet Might need to use reflection to print them not sure. Also added to inline logger around the Logger class you can see that in program.cs

So basically just like middleware in aspnetcore (indeed I lifted the source code from aspnet) you can define middle in two ways:

Either inline in program.cs (PS - I had to do something ugly there because it needs IServiceProvider and not IServiceCollection, so I had to list out the store from the configure lambda, or you can define a class with either an Invoke (or InvokeAsync) method.

Still to do:

  1. Clean up the code, cleanup UseMiddlewateExtensions is esp. messy still.
  2. Allow usage of IMiddleware
  3. Allow extra params in the Middleware class in ctor and Invoke
  4. Move Location handling to Middleware?

Please give me your thoughts on what I have so far.

chanan commented 6 years ago

@torhovland I am a bit stuck.

This wont work right:


builder.Use(async (state, action, next) =>
{
  Console.WriteLine("Inline logger old state: {0}", JsonUtil.Serialize(state));
  Console.WriteLine("Inline logger: {0}", JsonUtil.Serialize(action));
  await next();
  Console.WriteLine("Inline logger new state: {0}", JsonUtil.Serialize(state));
});

In the last line the "new state" will print out the same state as the first line. The reason why this work is aspnet core middleware and not in here is when you for example you mdoe something like this in aspnet middleware:

app.Use( context, next => 
{
  context.Request.Headers.add("myHeader", "myValue");
  next();
});  

And then you can see the change in a later middleware is that context in aspnet doesn't change objects, while in blazor-redux it does (in the roor reducer, return new MyState ( ... );. I tried to pass it by ref but since lambda's dont work with ref that doesnt work.

To see the problem, run the code in the PR and you will get this output:

WASM: Inline logger old state: {"Location":null,"Count":0,"Forecasts":null}
MonoPlatform.ts:191 WASM: Inline logger: {"Location":"http://localhost:57926/"}
MonoPlatform.ts:191 WASM: State before Store InvokeAsync: {"Location":null,"Count":0,"Forecasts":null}
MonoPlatform.ts:191 WASM: State has changed inside InvokeAsync: {"Location":"http://localhost:57926/","Count":0,"Forecasts":null}
MonoPlatform.ts:191 WASM: State after InvokeAsync should change to match 'State has changed inside InvokeAsync' above: {"Location":"http://localhost:57926/","Count":0,"Forecasts":null}
MonoPlatform.ts:191 WASM: Inline logger new state: {"Location":null,"Count":0,"Forecasts":null}

The last in this out should match the two above it, but doesnt because state cant be passed as ref in Program.cs. This won't work with "Typed" middleware either because the code in UseMiddlewareExtension uses a lambda as well.

I am not sure what to do to fix this. Any thoughts?

torhovland commented 6 years ago

Hi, @chanan !

I believe your delegate needs to return the new state:

public delegate Task<TState> StoreEventDelegate<TState, TAction>(TState state, TAction action);

Then you can access the old and new state like this:

        public async Task InvokeAsync(TState state, TAction action)
        {
            Console.WriteLine("State before action: {0}", _serialize(state));
            Console.WriteLine("Action: {0}", _serialize(action));
            var newState = await _next(state, action);
            Console.WriteLine("State after action: {0}", _serialize(newState));
        }

Having said that, I don't think we actually want to have async middleware, so it would instead be like this:

        public void InvokeAsync(TState state, TAction action)
        {
            Console.WriteLine("State before action: {0}", _serialize(state));
            Console.WriteLine("Action: {0}", _serialize(action));
            var newState = _next(state, action);
            Console.WriteLine("State after action: {0}", _serialize(newState));
        }
galvesribeiro commented 6 years ago

Just my 2cents on that... I see lots of places where people is using Console.WriteLine() and that produces a log output unfriendly to the browsers debugger tool. If the intent is to write just a string, its is ok, but I believe that most of the case it would be useful to have the object logged so you can leverage the logger pad capabilities better so while Blazor team don't make the logger to work in a browser friendly way, here is a gist to get the same behavior:

https://gist.github.com/galvesribeiro/5baeb3662fbfaab27867d5f6428379e0

If you pass a string, it will log just as Console.WriteLine() but if you pass an object, you will be able to see the object structure on the logger.

To use it, just drop the two files in a blazorlib project at a directory that is mapped as embedded resource so blazor apps will load it at boot time.

I hope it helps.

chanan commented 6 years ago

@torhovland I tried that and it wasn't working, I gave up and forgot about it, but yes you are right that was the way to go, I will go back and take a second look.

@galvesribeiro Yep, I plan to use JSInterop for the log messages, just need to get everything working first. Thanks for the gist!

chanan commented 6 years ago

@torhovland Fixed middleware to return TState, removed the async calls.

Here is the example from Program.cs of an inline middleware and class middleware:

builder.Use((state, action, next) =>
{
  Console.WriteLine("Inline logger old state: {0}", JsonUtil.Serialize(state));
  Console.WriteLine("Inline logger action: {0}", JsonUtil.Serialize(action));
  var newState = next(state, action);
  Console.WriteLine("Inline logger new state: {0}", JsonUtil.Serialize(newState));
  return newState;
  });

Func<object, string> fn = obj => JsonUtil.Serialize(obj);
builder.UseMiddleware<Logger<MyState, IAction>, MyState, IAction>(serviceProvider, fn);

Note: defining fn and passing to Logger is for two reasons: 1. to avoid taking a dep on Blazor in the Logger class, and more importantly, 2. to test passing additional params to the middleware class.

Still lots to do, but I think its starting to look good. I will work next on moving Location to middleware, even though its not 100% needed, I think it will help finding more things that need work.

chanan commented 6 years ago

@torhovland Next problem :)

The problem is how to allow middlware to dispatch actions , if you look in the new project I added BlazorReduxLocation -> Location.cs, Line 31. You will see that it won't compile (ignore the ugly _store.Dispath syntax, dispatch function should be supplied to the middleware but haven't done that yet) - The issue is that even though NewLocationAction implements IAction, it can't be cast to TAction.

I think maybe if Store changes to Store<TState, IAction> or using where on TAction it would fix the issue.

What do you think?

drkmn commented 6 years ago

Nice effort @chanan ! Very interested in middleware extensibility being integrated in blazor-redux for this will allow me to get a redux-saga like pattern going for server calls. As you already mentioned, passing the dispatch method through from one middleware to the next will be key for this.

chanan commented 6 years ago

Thanks @drkmn ! Once middleare works fully, I plan to port redux-saga, so hang in there! :)