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

PostReducer implementation #1976

Closed
etienne-dldc opened this issue Sep 21, 2016 · 2 comments
Closed

PostReducer implementation #1976

etienne-dldc opened this issue Sep 21, 2016 · 2 comments

Comments

@etienne-dldc
Copy link

etienne-dldc commented Sep 21, 2016

Hi,

I'm currently working on a big Webapp (Typescript / React / Redux) where we have some data to fetch. Do do so, we have tried different things:

Let's take the following case : the state is { user: /* a user */, account: null }
and we have the component that display the list of account for the current user.

First attempt: componentDidMount

So at first, we did something like this :

// In the component that need accounts
componentDidMount() {
  if (this.props.accounts === null) {
    dispatch(fetchAccounts(this.props.user))
  }
}

This solution had many problems:

  • If more than one component need accounts and are not parent <> child then the fetchAccount() is dispatch twice.
  • Even with only one component, while the data is fetching we re-fetch at each render.
  • Since this component need account to render more than no data or loading we do a render just to trigger the fetch action.
  • We need to pass the user to the component while it's only needed for the request

Second attempt: Improve the state

Let's solve the first two problem by adding more info in the state.
State is now:

{
  user: /* a user */,
  account: {
    status: 'void',
    content: null,
    fetchedUser: null
  }
}
// In the component that need accounts
componentDidMount() {
  if (this.props.accounts.status === 'void' || this.props.accounts.fetchedUser !== this.props.user) {
    // This set `accounts.status` to `loading` and set user in `accounts.fetchedUser`
    dispatch(fetchAccounts(this.props.user));
  }
}

Now we have prevent useless fetching but the code above need to be copied if to component that need accounts are not parent <> child.
To solve this, let's make fetchAccounts a thunk and put the logic inside:

// In the component that need accounts
componentDidMount() {
  dispatch(fetchAccountsIfNeeded(this.props.accounts, this.props.user));
}

It's better, but we still have some copy/past to do...

Wait, why is this in a component ?

In fact the real problem was that the fetch request has nothing to do in a component. Because fetching account data depend on 2 things : the user in state and the need to render a component that need accounts.
And since the render is predictable from the state, the only thing we need to fetch accounts is the state.

Third Attempt: a reducer after the reducer

What we needed was something that take the result of the reducer and do all the logic / resolve all state dependencies.
So we create a createStoreEnhancer that take a postReducer as argument, then the enhancer would return a reducer that work that way :

  • call original reducer
  • call the postReducer
  • is the postReducer has changed the state call the postReducer again.
    The result was that while the postReducer find something to change, we call the postReducer. This way we get some sort of propagation.

But this approach has 2 major issues:

  • We did side effect in post reducer (that was our initial question: where should we put side effect) which was really bad because it mean we were doing side effects in the reducer.
  • The state was mutated in only one cycle, so you dispatched an action and then you get a lot of changes in the state without knowing where it came from...

Fourth Attempt: Between the reducer and listener()

Since inside the reducer was not a good idea (at all), I needed to put my "postReducer" on step after so I changed my createStoreEnhancer instead of retuning a new reducer, I will now return a
new subscribe :

function subscribe (listener) {
  return initialSubscribe(() => {
    function dequeuePostReducerDispatchQueue() {
      store.dispatch(postReducerDispatchQueue.shift());
    }

    function dispatchFromPostReducer(action) {
      postReducerDispatchQueue.push(action);
    }

    if (postReducerDispatchQueue.length > 0) {
      dequeuePostReducerDispatchQueue();
      return;
    }

    // Run postReducer
    postReducer(dispatchFromPostReducer, state);

    if (postReducerDispatchQueue.length > 0) {
      dequeuePostReducerDispatchQueue();
      return;
    }

    listener();
  });
}

The result is quite the same than in third attempt but this time action are dispatched and side effects are not in the reducer.
BTW, this is the current solution.

So is this the right implementation ? (spoiler: nope)

No, this is still not right. in fact it is almost as bad as the previous one.
This solution currently works on our webapp because we use react-redux, so only one call is made to subscribe and the postReducer is apply only once.
But if we add just one listener to the store... Then post reducer are apply twice and everything breaks 💥

future fifth attempt: The correct one

I was first to early (in reducer) then too late (in subscribe) so the correct answer was between these two. In fact to work as expected I should directly subscribe to the store in createStoreEnhancer and then create my own subscribe method.

@markerikson
Copy link
Contributor

FYI, there's a variety of options for doing multiple dispatches with only a single render. You may want to read through the discussion in #1813 , where numerous approaches are compared. The Redux FAQ question at http://redux.js.org/docs/FAQ.html#actions-multiple-actions is also relevant.

@etienne-dldc
Copy link
Author

I solved my issue as I was updating it, that explain why there are no question/proposal.

@etienne-dldc etienne-dldc changed the title Add a *post-reducer* step PostReducer implementation Sep 21, 2016
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

No branches or pull requests

2 participants