ctrlplusb / easy-peasy

Vegetarian friendly state for React
https://easy-peasy.dev
MIT License
5.03k stars 190 forks source link

How to have actions or effects mutate disconnected parts of the state tree? #58

Closed christianchown closed 5 years ago

christianchown commented 5 years ago

Let's say I have got a model slice todos. I then add a new model slice toast for toast notifications. I want the toast state to get mutated (and perhaps do async stuff) as a result of todo actions.

In vanilla Redux, for sync mutations, I'd add extra switch cases in my toast reducer, and typically use one of two approaches for async

Is there a different, easy-peasy, way for sync or async mutations to model slices based on actions (or effects) evoked on other parts of the state tree? A reducer at the toast level seems a good fit for sync, but both that and the toastMiddleware/saga route would seem to need an idiomatic way to get the correct action.type string.

(These also might need a typing helper to get the shape of the vanilla Redux action in Typescript... πŸ€”)

Anyone run into this and found a pattern that works?

ctrlplusb commented 5 years ago

Hey @christianchown;

My general approach I have taken to this is to switch the originating action into an effect, thereby gaining the ability to dispatch multiple actions. Effects need not necessarily fire asynchronous actions.

createStore({
  toasts: {
    ...
  },
  todos: {
    ...,
    add: effect((dispatch, payload) => {
      dispatch.todos.save(payload);
      dispatch.toasts.success('Added todo');
    })
  },
});

This does make the relationship more explicit in my opinion, although it does feel like a double action against the todos model. Perhaps it also starts to skew the meaning of "effect".

Definitely open to thinking this one through a bit more.

ctrlplusb commented 5 years ago

We could consider renaming effect to thunk (keeping it aliased to avoid introduction a breaking change). I feel like thunk would allow the type of thinking that it is a fn that can dispatch multiple actions, rather than being explicitly related to containing side effects.

christianchown commented 5 years ago

Hi @ctrlplusb the problem with the above approach is that it requres todos to know about (and be tightly coupled to) toast, when it's the opposite that I really want to achieve.

import { todos } from 'shared-framework';

createStore({
  todos,
  toasts: {
    ...
    toastReducer: reducer((s, a) => {
      // handle when a.type is a todo action?
    }),
    // and how to handle async?
  },
});

The most common use-case for this pattern is an AUTH_LOGOUT action.

I've historically had many websites and apps use shared authentication logic, but each website & app then perform custom additional steps when logging out - removing site-specific sensitive data, transmitting custom logs etc. In this case, I can't have the shared auth model slice know about these things: it's for the consumer of the auth reducer/model slice to decide on the appropriate activity.

It's not a problem with vanilla Redux as each reducer gets every action, not just its own (and ditto for middleware/saga for async). Haven't yet come up with an easy(-peasy) way of doing the same.

ctrlplusb commented 5 years ago

Totally see your point.

What about something conceptually like this:

import { createStore, listener } from 'easy-peasy';

const store = createStore({
    todos: {
        add: () => ...,
    },
    user: {
        logout: () => ...,
    },
    // You wrap the model with the "listner" helper, indicating that you want
    // it to be able to listen and respond to other actions.
    //      πŸ‘‡
    toast: listener(
        // The first argument is the "model", as normal:
        {
            notification: '',
            clear: (state) => {
                state.notification = '';
            },
            set: (state, payload) => {
                state.notification = payload;
            }
        }, 
        // The second argument is a function that receives the actions and then
        // must return an object, where the keys are the action to listen to,
        // and the right is an "effect" to execute when the subsequent action
        // gets fired
        actions => ({
            [actions.todos.add]: (dispatch, payload) => {
                // The payload argument is the same value that the action would
                // have received. i.e. in this case the "todo"
                console.log(payload); 
                dispatch.toast.clear();
                dispatch.toast.set('Todo added');
            },
            [actions.user.logout]: (dispatch, payload) => {
                dispatch.toast.clear();
                dispatch.toast.set('You have been logged out');
            }
        })
    )
})

Also, would be good to reconsider the "dispatch" that gets passed into "effects". May be better to default scope it to it's path, similar to state for actions. Try to promote isolated behaviour. Could rename the property to "actions", and then have another helper "getDispatch" similar to "getState" for the exceptional cases.

ctrlplusb commented 5 years ago

The other option would be for something a bit more generic and "future proof".

import { createStore, model } from 'easy-peasy';

const store = createStore({
  todos: { 
    ...
  },
  user: { 
    ...
  },
  toast: model({
    state: {
      notification: '',
    },
    actions: {
      set: (state, payload) => {
        state.notification = payload;
      }
    },
    effects: {
      ...
    },
    listeners: actions => ({
        [actions.todo.add]: (dispatch, payload) => {
          dispatch.toast.set('Todo added');
        },
    })
  })
});
skulptur commented 5 years ago

@ctrlplusb I didn't give it a deep thought but what about something like this:

import { createStore, when } from 'easy-peasy'

const store = createStore({
  todos: {
    // ...
  },
  toast: {
    ...when((actions) => ({
      [actions.todo.add]: (dispatch, payload) => {
        dispatch.toast.set('Todo added')
      },
    })),
  },
})

In this case the when function would return an object that is namespaced with a prefix that you can use to identify listeners.

Either way, between the two options you had I prefer the latter.

christianchown commented 5 years ago

Conceptually, I think we're close, and we all like @ctrlplusb's idea of attaching these via an object:

{
  [actions.todo.add]: (dispatch, payload) => {
    dispatch.toast.set('Todo added');
  },
}

I thought about a listener() function at the same conceptual level of effect(), reducer() or select():

import { createStore, listener } from 'easy-peasy';

const store = createStore({
  todos: { 
    ...
  },
  user: { 
    ...
  },
  toast: {
    notification: '',
    set: (state, payload) => {
      state.notification = payload;
    },
    todos: listener({
      [actions.todo.add]: (dispatch, payload) => {
        dispatch.toast.set('Todo added');
      },
    }),
  },

The only thing, is that it doesn't really need (or want) a key - I gave it todos, but now I wrote it, I'm not sure that's best. I didn't understand why you spread the ...when @skulptur until I did it this way! (maybe ...listeners() instead? But requiring the spread seems a bit off πŸ€”)

ctrlplusb commented 5 years ago

I like your attempt at keeping things "easy" @skulptur , πŸ˜‰, but I agree with @christianchown that the spread doesn't feel right in this case.

Perhaps we could have a "special" key, reserved for this case:

import { createStore } from 'easy-peasy'

const store = createStore({
  todos: {
    // ...
  },
  toast: {
    notification: '',
    set: (state, payload) => ...,
    when: ((actions) => ({
      [actions.todo.add]: (dispatch, payload) => {
        dispatch.toast.set('Todo added')
      },
    })),
  },
})
skulptur commented 5 years ago

@ctrlplusb special key was the first thing I thought but do you think it could be just a simple word without namespacing or will we start running into conflicts?

Something I thought about since I started using easy-peasy is that it would be cool to have the possibility to make some keys not be available for direct access, like for example when an action is/should only be called by an effect. For example a "load" effect and a "loaded" action... Perhaps we could say "_loaded" and it wouldn't show up outside of effects to prevent it from being called directly.

I say that because if we go with the route of using special keys then we might think about it in a bigger picture.

skulptur commented 5 years ago

Ah and the spread might not feel right but there's two points for it: you as the library author would have control over the key that is used and could return something like __easy_peasy__listeners__ and not take up a potentially useful key from the final users. Also it kinda reads like english "...when". So it looking off is actually a nice indicator that something exterior is going on there.

skulptur commented 5 years ago

@ctrlplusb I was also thinking... what if I have helpers that have listeners? My entire model is based on smaller helpers to avoid repetition and to make it more standardized. If we use a single key, when I spread the helper into my model it might clash if it also has a listener key.

This is an example of part of my model now:

const model = {
  categories: {
    ...fetchedCollection({ endpoint: api.getCategories }),
  },
  products: {
    ...fetchedCollection({ endpoint: api.getProducts }),
  },
}

The fetchedCollection helper itself is a composition of collection and fetched helpers that can also be used individually. One of the things I have to do still is make products fetch again once the active category has changed. I can totally see myself wanting to abstract behaviors like that into helpers themselves.

In fact I've been thinking of writing a lib of easy-peasy helpers for all sorts of basic things like toggles, arrays that can have items added/removed, counters, etc..

I think you get the point. Right now everything in easy-peasy allows for composition. We should keep it that way imo.

ctrlplusb commented 5 years ago

Very good point @skulptur - and taking this into consideration perhaps it would be best to follow @christianchown's "labelled" approach. This would allow composition as well as conceptual grouping of "listeners". It would also fit in quite nicely with the existing API.

e.g.

import { createStore, when } from 'easy-peasy';

const store = createStore({
  todos: {
    // ...
  },
  toast: {
    notification: '',
    clear: (state) => {
      state.notification = '';
    },
    set: (state, payload) => {
      state.notification = payload;
    },
    setNotificationListeners: when((actions) => ({
      [actions.todo.add]: (dispatch, payload) => {
        dispatch.toast.set('Todo added');
      },
    })),
    clearNotificationListeners: when((actions) => ({
      [actions.user.logout]: (dispatch, payload) => {
        dispatch.toast.clear();
      },
    }))
  }
});
christianchown commented 5 years ago

Been thinking of some listener(actions => ({ ... })) options

  1. when()
  2. on()
  3. upon()
  4. after()

but I think my favourite is (and easiest to grok for Saga users) is

  1. take()
ctrlplusb commented 5 years ago

My preferences, in order:

  1. when
  2. on
  3. after
  4. upon
  5. take

I definitely want to go for something that would be generally understood, I appreciate take may make sense for those with redux-saga experience, but to myself it isn't immediately obvious. I feel like when/on/after are very easy to infer behaviour from.

Appreciate this is completely subjective though!

What were the hard things in computer science again? 😊

skulptur commented 5 years ago
  1. external()

@ctrlplusb Between the two you guys mentioned I prefer to have something like @christianchown mentioned and you displayed in your last example.

I also agree with the take being a weird name (haven't used sagas). I'd be biased if I mentioned my favorites so I'll leave that to you.

ctrlplusb commented 5 years ago

πŸ‘€ ☝️

ctrlplusb commented 5 years ago

Reworked #61