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

feat(lib): provide store functionality #185

Closed
wants to merge 1 commit into from

Conversation

AntJanus
Copy link
Collaborator

@AntJanus AntJanus commented Jan 5, 2018

Ref: #19

@deini would love some input on this. Here's the idea:

  1. when you dispatch, dispatch goes through the provided store. The provided store does its magic and triggers all of its subscribers (Angular and non-Angular)
  2. ng-redux creates an "empty" store which has a pass-through reducer (state => state)
  3. ng-redux subscribes to the provided store and on state update, dispatches the entire state to the "empty" store
  4. ng-redux users subscribe to the "empty" store and get notified as usual of any and all changes.

I haven't tested this out yet.

const resolveStoreEnhancer = storeEnhancer => isString(storeEnhancer)
? $injector.get(storeEnhancer)
: storeEnhancer;
return createNgReduxStore($injector, _middlewares, _storeEnhancers, _reducer, _initialState);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the diff looks a bit weird but I basically moved the this.$get logic out into its own function so that we can create stores on-demand.

.subscribe(() => {
let newState = providedStore.getState();

ngReduxStore.dispatch(newState);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here's where the magic happens. The only thing I'm unsure about is cleanup. But as I understand it, Angular 1 never really "unloads" during runtime so this shouldn't be an issue.

@AntJanus AntJanus added the 4.1.0 label Jan 8, 2018
@AntJanus AntJanus requested a review from deini January 8, 2018 16:41
Copy link
Contributor

@derrickpelletier derrickpelletier left a comment

Choose a reason for hiding this comment

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

With these changes, this is functioning well for me. Though it'd still be worth figuring out the dispatch. Maybe just ensuring the defaultMapDispatchToTarget was providing the write dispatch fn when using a provided store?

})
;

return Object.assign({},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention to not provide .connect() functionality when using a provided store? I fiddled with it and added it back in, the only caveat is that connected actions dispatch to the wrong store, so you always need to use the $ngRedux.dispatch.

const resolveReducerKey = (result, key) => assign({}, result,
{ [key]: getReducerKey(key) }
);
function createNgReduxStore($injector, _middlewares, _storeEnhancers, _reducer, _initialState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

By moving this function out, i'm getting _reducerIsObject is not defined, on line 77. Looks like it doesn't have the scope of all the vars defined at the top of ngReduxProvider()

Copy link
Contributor

Choose a reason for hiding this comment

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

I resolved it for myself by moving createNgReduxStore inside of the ngReduxProvider method's scope but i'm not sure if that was a good solution or not.

? $injector.get(middleware)
: middleware;
if (_providedStore) {
const emptyStore = createNgReduxStore($injector, [], [], state => state, undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

This pass through reducer is throwing errors for me. It's expecting it to be a well formed action object {type: 'foobar'}.

Changed to (state, action) => action.payload and then changed line 6 of the store wrapper to just dispatch as:

ngReduxStore.dispatch({type:'@@NGREDUX_PASSTHROUGH', payload: newState});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah! nice! I figured that might cause some issues.

@AntJanus
Copy link
Collaborator Author

@derrickpelletier thanks for all the feedback! I'll have to get back to this once I get a chance. If you want to take over the branch and push the fixes you've got, you're more than welcome to.

@derrickpelletier
Copy link
Contributor

Happy to, will push something soon.

@derrickpelletier
Copy link
Contributor

@AntJanus how would you suggest going about it? I branched off yours, can PR into your branch or just re-submit the PR?

@AntJanus
Copy link
Collaborator Author

@derrickpelletier resubmit it and I'll just close this one out. Thanks for the hard work! :)

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

Successfully merging this pull request may close these issues.

2 participants