Odonno / ReduxSimple

Simple Stupid Redux Store using Reactive Extensions
http://www.nuget.org/packages/ReduxSimple/
MIT License
144 stars 20 forks source link

Improve README file for beginners #76

Open zanybaka opened 4 years ago

zanybaka commented 4 years ago

Hey guys,

I've just tried to use all the described features and here are some ideas how to improve the readme.

1. Create Reducer functions

It is not so obvious without looking into your code that it is required to specify using static ReduxSimple.Reducers to use On. It would be helpful to specify the full namespace ReduxSimple.Reducers.On in the examples.

public static class Reducers
{
    public static IEnumerable<On<RootState>> CreateReducers()
    {
        return new List<On<RootState>>
        {
            ReduxSimple.Reducers.On<NavigateAction, RootState>(

2. Reducers on action

It seems the reducer is wrong as CurrentPage setter is not called. Why don't you update it here?

You can define a list of On functions where at least one action can be triggered.

return new List<On<RootState>>
{
On<NavigateAction, RootState>(
(state, action) => state.With(new { Pages = state.Pages.Add(action.PageName) })
),

I had to replace it with

On<NavigateAction, RootState>(
                    (state, action) => state.With(new
                    {
                        CurrentPage = action.PageName,
                        Pages       = state.Pages.Add(action.PageName)
                    })),

3. Enable time travel

It's not so obvious how to track Redo action. It would be helpful to specify it directly in the readme file or even to implement ObserveRedoneAction.

// Undo
App.Store.ObserveUndoneAction().Subscribe(o =>
{
    ...
});

// Redo
App.Store.ObserveAction(ActionOriginFilter.Redone).Subscribe(o =>
{
    ...
});

4.1 Entity management (in preview)

Could you publish the preview version as a separate alpha/beta package? something like ReduxSimple.Entity.dll

4.2 Then use the EntityAdapter in reducers

On<CompleteTodoItemAction, TodoListState>(
    (state, action) =>
    {
        return state.With(new
        {
            Items = TodoItemAdapter.UpsertOne(new { action.Id, Completed = true }, state.Items)
        });
    }
)

It is a potential error prone code imho. a) the current UpsertOne impl requires the writable property Id in TodoItem class to be properly converted (would be nice to have just a field) b) if you make a mistake in the anonymous class (name the property as ID instead of Id) you will receive the unfriendly message Value cannot be null. Parameter name: key' from the UpsertMany method. It would be nice to raise some specific exceptions for such cases.


What you do think?

visionarylab commented 4 years ago

Reference to #78

Odonno commented 4 years ago

@visionarylab In order to make it clear, I created another issue for your problem. I will look into it asap.

@zanybaka It seems like a nice feedback you made. I will try to read and respond to each part separately.