reduxjs / redux

A JS library for predictable global state management
https://redux.js.org
MIT License
60.88k stars 15.27k forks source link

Find a way to cut getReducer and replaceReducer from the API #350

Closed gaearon closed 9 years ago

gaearon commented 9 years ago

They are used for hot reloading by react-redux and code splitting. However the solution feels wrong and ad-hoc to me.

Something like a store enhancer that would wrap the reducer a la devTools seems like the way forward. This way hot reloading would be opt in, but much more reliable.

Whatever the solution may be, it has to solve the following issues to be considered:

37 #301 #340 #346 #348 https://github.com/gaearon/redux-devtools/issues/68

Ideas welcome!

taylorhakes commented 9 years ago

Here is my proposal. I am sure it has some issues.

Expose reducer as a simple function to Higher Order Stores.

Higher order stores would be created like this:

createStore(middleware, devtools, persistState);

Higher Order Stores would have the following signature:

function higherOrderStore({ reducer, dispatch, getState, subscribe }) {
  // wrap components

  return {
    reducer: newReducer,
    dispatch: newDispatch,
    // ...
  };
}

The create store function would have the following signature

createStore(...funcs) {
  const store = new Store();
  const finalStore = compose(...funcs, store);
  finalStore.dispatch({ action: '@@init' });
  return store;
}

This allows the store to be very flexible. @@init can be called by other components or not at all.

This is just off the top of my head. Haven't thought of all the implications of this change. It definitely would solve #376 though.

gaearon commented 9 years ago

Interesting, thanks. On the first sight I like it, but I haven't thought through the implications. I'd say we'll release 1.0 as is, but this will be on our radar as the next important breaking change.

gaearon commented 9 years ago

@taylorhakes Can you hack up a proof of concept of this in a branch?

wbecker commented 9 years ago

+1

taylorhakes commented 9 years ago

Sure, I can do it this weekend.

acstll commented 9 years ago

I have one more proposal. The idea is that createStore returns itself, so you can create a copy of a store updating reducer and/or state. It's a function but of course keeps the 3 main methods getState, dispatch and subscribe.

EDIT: plus a reducer getter or the same getReducer method.

Here's how I'd implement it: https://github.com/acstll/chopped-redux/blob/next/index.js#L32-L40 You'd also have to take listeners as third argument, so you can keep them as well in new copies.

So the API would be this:

let store = createStore(reducer, initialState)

// Create a new store replacing `reducer` but keeping state and listeners.
store = store(nextReducer)

With this, I think React bindings code would be like this:

Now

  componentWillReceiveProps(nextProps) {
    const { store } = this.state;
    const { store: nextStore } = nextProps;

    if (store !== nextStore) {
      const nextReducer = nextStore.getReducer();
      store.replaceReducer(nextReducer);
    }
  }

After

  componentWillReceiveProps(nextProps) {
    const { store } = this.state;
    const { store: nextStore } = nextProps;

    if (store !== nextStore) {
      store = store(nextStore.reducer)
    }
  }

// the before EDIT `store = nextStore(null, store.getState())` would lose its original listeners

I have to say I actually don't see any problems with the getReducer() and replaceReducer() code, it's a lot more explicit, which should be good.

taylorhakes commented 9 years ago

@acstll What does the higher order store API look like? Wondering if it will solve this? https://github.com/gaearon/redux/issues/376

acstll commented 9 years ago

@taylorhakes what you get from the first createStore call is the actual store instance, there's no higher order store. :)

Wondering if it will solve this? #376

I don't think so :grin: If you need to chain-wrap the reducer you need to have it exposed in some way. Either with getReducer() or a getter or something.

From #376:

The previous example shows that compose makes the store.dispatch and reducer wrapping go in opposite directions and that is an issue.

Maybe the problem is with compose and the applyMiddleware implementation? I actually like your idea of applying middleware directly to the store instance and not creating a higher-order function. Looks like this needs a lot more discussion.

gaearon commented 9 years ago

See my proposal: #624, #625.

acstll commented 9 years ago

Brilliant :ok_hand:

@gaearon you could even expose the onNextReducer method to allow people wanting to dynamically load reducers to do it.

gaearon commented 9 years ago

@acstll

Do you mean expose on the store instance? Truth be told, I'd rather avoid it. My goal is to make { dispatch, getState, subscribe } the store API, as I don't think cluttering the store object from enhancers is any better than inheritance. There should be separation of store interface and the means to control enhancers.

I'll think about how to implement code splitting with the same switchReducer—it shouldn't be difficult.

acstll commented 9 years ago

@gaearon yes I meant that.

I'll think about how to implement code splitting with the same switchReducer

Looking forward to it. :+1:

taylorhakes commented 9 years ago

I think this proposal is great, but it doesn't address the issue discussed here https://github.com/rackt/redux/issues/376 (it doesn't necessarily have to). I would like to reopen that issue if this is the road we decide.

Sorry I wasn't able to implement my proposal above. I have been really busy with other things. I will try to find some time this week.

gaearon commented 9 years ago

@taylorhakes

I was a bit disoriented by the approach taken in localState there. It would really help if you could come up with a smaller not-working-with-current-API-but-should-have-been-working example.

arackaf commented 9 years ago

I've been reading this thread, and the related ones with interest. I'm new to Redux, though code splitting is definitely something I've used in the past with success, and likely will continue to do so going forward.

Can I ask why you're looking to get rid of replaceReducer? It looks like a direct, simple and effective solution to this. Are there deeper issues with it I'd need more experience with Redux to be aware of?

gaearon commented 9 years ago

I think I'm satisfied with removing getReducer(). It seems like any way to cut replaceReducer() is more awkward than leaving it in.

Markus-ipse commented 9 years ago

@gaearon Sorry for commenting on a closed issue, but I ended up here from issue #37, about code-splitting in large apps, which I'm trying to do now, but that issue and this one is closed but I'm not entirely sure that the code-splitting issue is solved or is it? If there is a way to handle code-splitting is there any docs, examples or issues to point me in the right direction?

gaearon commented 9 years ago

@hummlas The current solution is to use store.replaceReducer(). For example, if you combine reducers from a bunch of them, when you load a new one, you want to combine them again and replace the store's reducer with the new combined one.

Markus-ipse commented 9 years ago

@gaearon Wow, thanks for the quick reply! I thought I saw somewhere that store.replaceReducer was deprecated, but anyhow, after conferring with a colleague we decided that it might be easier (for us at least) to always have all reducers in the main bundle and and let the route-specific bundles only contain components, etc. :)

gaearon commented 9 years ago

I thought I saw somewhere that store.replaceReducer was deprecated

It was un-deprecated later :-). Yeah, depends on your use case. If reducers are small, it's no big deal.

eriknyk commented 9 years ago

HI, any benevolent soul that can provide an example of its implementation?

gaearon commented 9 years ago

http://stackoverflow.com/questions/32968016/how-to-dynamically-load-reducers-for-code-splitting-in-a-redux-application

arackaf commented 9 years ago

Hi @gaearon - is it expected that after a call to replaceReducer, a default call to dispatch would be made with an undefined state, so the new default can be set? It doesn't seem to happen.

As it is, the workaround is trivial

store.replaceReducer(reducer); store.dispatch(initialize());

with

case INITIALIZE: return initialState;

I'm just curious if I'm missing something.

gaearon commented 9 years ago

I'm pretty sure it happens: https://github.com/rackt/redux/blob/92f44c4b902934f08834e545416ae0fb9814d30c/src/createStore.js#L136.

gaearon commented 9 years ago

Of course, you'll only have the initial state for parts of the tree that didn't have any state associated with them. This is useful when you're doing hot reloading, or when you're adding a new code-split reducer under an existing reducer tree.

If you want to clean up all the state when running replaceReducer(), you should fire a custom action that does that on the root reducer level.

arackaf commented 9 years ago

That clarifies things perfectly - thank you much! I'm still quite new to react and redux.

maximoleinyk commented 8 years ago

Pretty simple example for those who may be interested.