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

createStoreWith accepts either a function or an object with reducers #76

Merged

Conversation

hgsigner
Copy link
Contributor

Hi,

This PR aims to solve this issue -> #75 but in a different manner. We've implemented a way to pass the reducer as a single Function or as an Object. This is extremely helpful because we can create the reducers as an angular services and initialize it in the .config.

The reducer object would accept a function or a string for its values. Using it as a string, we can later use $injector.get("myReducerAsService") and get it injected.

It'll work like this:

  "$ngReduxProvider", "Redux"
  "ReduxThunk"
  (
    $ngReduxProvider, Redux, thunk
  ) ->
    $ngReduxProvider.createStoreWith({reducerAsService: "reducerAsService", reducerAsFunctions: fnction(){}}, [thunk])
    return
])```

Let me know what you think.
Thanks

@Fire-Dragon-DoL
Copy link

Thanks Hugo, I'm really looking forward this

import digestMiddleware from './digestMiddleware';

import assign from 'lodash.assign';
import isArray from 'lodash.isarray';
import isFunction from 'lodash.isfunction';
import isObejct from 'lodash.isobject';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a big typo here

@hgsigner
Copy link
Contributor Author

@Fire-Dragon-DoL Fixed ;)

@jballands
Copy link

Hi Hugo, thanks for looking into this! I went through your pull request and I'm wondering if this completely solves the problem of providing 1 reducer for n modules.

Correct me if I'm wrong, but it seems that I must provide an Angular service that has the ability to provide all reducers for my application. In issue #75, I mention that, in order to not violate the single responsibility principle, I would like each module to be responsible for registering it's own reducers with ngRedux; in other words, $ngReduxProvider should be able to provide an $ngRedux that is composed of multiple reducers from multiple different controllers and services, not just one service that gathers all the reducers for the application up. That should be the $ngReduxProvider's job, in my opinion.

Again, perhaps I'm just misunderstanding what you wrote, so thanks again for taking this on :)

@Fire-Dragon-DoL
Copy link

@jballands True, this PR adds "only" the part where you can inject reducers into the store, but we also carefully evaluated the following:

You can't register the reducer in the reducer itself. This is simply not possible because if the reducer register itself, it means the reducer doesn't get injected anywhere, and so angular will never load it.

Sure we can have somewhere an object $ngReducersProvider that will allow this, but I would consider that in a separate pull request in that case.

I consider this pull request extremely useful because it will allow you to use reducers as services, which in turn allows you to use services in reducers. This will:

  1. Avoid having reducers as constants
  2. Not having reducer as constants will allow you to have action types as constants, instead of having them on a global object (you can't inject stuff in constants)
  3. You can use other services inside your reducers, so for example you have a helper that turns your list of users into a UserID => UserObject and you want to use it, in this way you can.

@Fire-Dragon-DoL
Copy link

@jballands I was thinking that while this doesn't solve #75 , it's definitely a first step (groundbase) toward having a solution for your problem. I still don't see how you are going to face the never-injected problem in angular (which in turn will never load the reducer), but I guess we can work towards it

@jballands
Copy link

@Fire-Dragon-DoL I agree, I think this pull request is a step in the right direction and is certainly very helpful! I just don't think it solves issue #75 exactly. :)

@Fire-Dragon-DoL
Copy link

@jballands yeah I agree. I wonder if @wbuchwalter has the chance to review the pull request

@wbuchwalter wbuchwalter merged commit 912f5d1 into angular-redux:master Apr 24, 2016
@wbuchwalter
Copy link
Member

Hey!
Sadly I really don't have much time these days.
Changes seems good to me but couldn't really test them, so I release them under version 3.3.4-beta.0.
Play with this version, and when you confirm it's good I'll release it non-beta.
Thanks!

@Fire-Dragon-DoL
Copy link

@wbuchwalter sure! No problem, we are going to use this PR on a software used extensively by our customers, so if there are issues, we will notice that immediately, will let you know. Thanks a lot for taking time to merge this stuff ;)

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

Successfully merging this pull request may close these issues.

4 participants