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

Added appEpics #1644

Merged
merged 3 commits into from
Mar 27, 2017
Merged

Added appEpics #1644

merged 3 commits into from
Mar 27, 2017

Conversation

kappu72
Copy link
Contributor

@kappu72 kappu72 commented Mar 27, 2017

This pull lets pass an object with app specific Epics
If Epic's name, in appEpics, is the same the one in plugins this will override plugin's one

… If Epic's name is the same the one in plugins this will override plugin's one
mbarto
mbarto previously requested changes Mar 27, 2017
@@ -26,7 +26,7 @@ const {createEpicMiddleware} = require('redux-observable');
const SecurityUtils = require('../utils/SecurityUtils');
const ListenerEnhancer = require('@carnesen/redux-add-action-listener-enhancer').default;

module.exports = (initialState = {defaultState: {}, mobile: {}}, appReducers = {}, plugins, storeOpts) => {
module.exports = (initialState = {defaultState: {}, mobile: {}}, appReducers = {}, appEpics = {}, plugins, storeOpts) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

New arguments at the end, for compatibility reasons, please

Copy link
Member

@offtherailz offtherailz Mar 27, 2017

Choose a reason for hiding this comment

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

This signature is better. This doesn't break the binding with StandardApp or other apps. I should put it in that place instead of the bottom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Binding appEpics wouldn't be possible

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Please remove

@@ -72,5 +73,11 @@ describe('PluginsUtils', () => {
expect(desc1.items[0].cfg).toExist();

});
it('combineEpics', () => {
const plugins = {MapSearchPlugin: MapSearchPlugin};
const appEpics = {appEpics: (actions$) => actions$};
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like an infinite loop. Is not executed but it should. Please remove it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 77.08% when pulling ad35ca8 on kappu72:appEpics into ffe8487 on geosolutions-it:master.

@offtherailz offtherailz dismissed mbarto’s stale review March 27, 2017 13:45

as discussed we removed this because is the correct usage

@offtherailz offtherailz merged commit 8617224 into geosolutions-it:master Mar 27, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 77.08% when pulling 3e95ba1 on kappu72:appEpics into ffe8487 on geosolutions-it:master.

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