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

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

Closed
gaearon opened this issue Jul 28, 2015 · 27 comments
Closed

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

gaearon opened this issue Jul 28, 2015 · 27 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2015

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 reduxjs/redux-devtools#68

Ideas welcome!

@taylorhakes
Copy link
Contributor

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
Copy link
Contributor Author

gaearon commented Jul 31, 2015

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
Copy link
Contributor Author

gaearon commented Aug 7, 2015

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

@wbecker
Copy link

wbecker commented Aug 7, 2015

+1

@taylorhakes
Copy link
Contributor

Sure, I can do it this weekend.

@acstll
Copy link

acstll commented Aug 8, 2015

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
Copy link
Contributor

@acstll What does the higher order store API look like? Wondering if it will solve this? #376

@acstll
Copy link

acstll commented Aug 9, 2015

@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 😁 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
Copy link
Contributor Author

gaearon commented Aug 25, 2015

See my proposal: #624, #625.

@acstll
Copy link

acstll commented Aug 25, 2015

Brilliant 👌

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

@gaearon
Copy link
Contributor Author

gaearon commented Aug 25, 2015

@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
Copy link

acstll commented Aug 25, 2015

@gaearon yes I meant that.

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

Looking forward to it. 👍

@taylorhakes
Copy link
Contributor

I think this proposal is great, but it doesn't address the issue discussed here #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
Copy link
Contributor Author

gaearon commented Aug 25, 2015

@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
Copy link

arackaf commented Sep 7, 2015

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
Copy link
Contributor Author

gaearon commented Sep 12, 2015

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

@gaearon gaearon closed this as completed Sep 12, 2015
zousandian added a commit to zousandian/redux-in-chinese that referenced this issue Sep 22, 2015
根据 reduxjs/redux#350 及文档原文,
API 已将 `getReducer()` 移除,`replaceReducer(nextReducer)` 得以保留
@Markus-ipse
Copy link

@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
Copy link
Contributor Author

gaearon commented Sep 23, 2015

@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
Copy link

@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
Copy link
Contributor Author

gaearon commented Sep 23, 2015

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
Copy link
Contributor

eriknyk commented Oct 16, 2015

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

@gaearon
Copy link
Contributor Author

gaearon commented Oct 16, 2015

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

@arackaf
Copy link

arackaf commented Oct 16, 2015

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
Copy link
Contributor Author

gaearon commented Oct 16, 2015

@gaearon
Copy link
Contributor Author

gaearon commented Oct 16, 2015

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
Copy link

arackaf commented Oct 16, 2015

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

@maximoleinyk
Copy link

Pretty simple example for those who may be interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants