Fix timedout issue

[Mickael-Roger]

Fix the time out issue reported by Kartik in slack and add his command line parsing ehancement using shlex

[toranb]

Proof this implementation not only has a green build but won't bust your app 👍

[toranb]

I added a new commit to move the redux-thunk imports to app/ instead of addon/

By doing this the consuming app can truly/safely remove redux-thunk as a dependency and add something like redux-saga :)

[toranb]

Because this was a move the service diff isn't a great read - if you want to see a simple diff I pushed a patch up to dropbox (worth a look if you want a simple idea of what has changed in the service)

[toranb]

I think the last remaining step here could/would? be to make makeStoreInstance something we import explicitly so "power users" can truly just override that part producing a store configured any way they want (like you would by hand using react-redux for example).

Anyone have thoughts on this? where would we export it from exactly?

[foxnewsnetwork]

makeStoreInstance is already merged in with the other stuff as a part of extending Ember.Service

Therefore, power-users can already do something like:

import ReduxService from 'ember-redux/services/redux';

export default ReduxService.extend({
  makeStoreInstance({ middlewareConfig, enhaners, reducers }) { 
    doWhatever()
  })

[foxnewsnetwork]

LGTM. Also, please consider also editing over some of the howto extend from README.markdown stuff on my branch: https://github.com/foxnewsnetwork/ember-redux/tree/feature/addonify-redux-service#customizing-the-redux-service

[toranb]

@brettburley I've rebased this and cut the addon/middleware down to just an empty array as you suggested. I still need to pull in some additional documentation for this as @foxnewsnetwork suggested above ^^

[toranb]

Okay I've got this rebased - still needs some docs love (copy/paste of what Thomas linked us to above). I need to see how this looks/feels in a real app so that's on me this coming week

[bstro]

Any estimate on when this sweet PR might land? Anything I can do to help?

[toranb]

@bstro The only real blocker at the moment is my internal debate over the "breaking change" this introduced. The problem is that previously we wouldn't include a reducer for you (meaning an error message would pop up when you ran the build w/out reducers/index.js )

We are planning to cut v3.0 in early July and this is likely the 2nd PR that will land officially (in that case any of the fears about a breaking api change will have a major version number to land under).

I don't have anything else planned until that release next month but we could use some examples/ docs/ for people who want to extend this like yourself. Would you want to write up a small section for the docs that covers this in more detail?

[bstro]

Ah, so this won't land in v2.x? In order to have these changes, I'd also need to upgrade our version of node+ember-cli?

ember-cli: 2.4.3
node: 4.8.2
os: darwin x64

[toranb]

@bstro It's possible we could land this in 2x -worth a debate on the "breaking change" I mentioned above. That said, in the discussion about 3X I showed a graph after node 8 was released recently and asked about what may be blocking you from taking on node 6 as a build time dependency.

You can still use ember-cli v2.4.3 w/ node 6.10 and that will be supported in both 2X and 3X of ember-redux. The problem arises when you need to build your ember-cli v2.4.3 app with node 4 (you will find a break as a result of the upgrade from babel 5 to babel 6).

What is blocking you/ your team from adopting node 6 as a build time dependency today?

[brettburley]

@toranb I don't think it's a breaking change since there aren't any meaningful apps with a missing reducers/index.js file. I don't think this PR changes the API in any way for existing apps, and only changes behaviors for new apps, so wouldn't necessitate a major version change because there is no work an app owner would have to do. I also think adding the runtime warning is probably sufficient warning for the new app case.

I'd strongly support landing this in the current major version (with the caveats of my previous feedback). :+1:

[bstro]

@toranb I'm going to spike this week on upgrading our Node to 6 LTS. Some promising progress so far.

[toranb]

@brettburley it sounds like we agree that this change does not introduce a real/ functional breaking change of any kind.

@brettburley I've updated the name you called out middlewareConfig to instead match the others (plural) middlewares. One question with that name change - is it correct? Meaning, we could bolt on 2+ middleware (one for logging/ one for async work/ etc)

The last step before we merge this into the 2x series is for me to find a way to test this from the dummy app (like we did w/ patchMiddleware). If that's not possible/or easily possible I'd like to test this manually with ember-cli v2.4.3 and v2.13.2

[toranb]

@foxnewsnetwork @brettburley @bstro

I threw together a working ember-cli v2.4.3 app using this fork and customized the redux store creation. Looking for feedback on this before we merge it next week

https://github.com/toranb/configme243/commit/d5fe04cb9f2a4223251379942fc38767fcda2bb7

[bstro]

Your 2.4.3 example looks consistent with how I've been using it! It feels pretty good to me. Nothing to critique on my end.

[toranb]

@bstro one issue I've found today is that I can destructure enhancers just fine but not the reducers for some reason. The below doesn't blow up, but the reducers don't fire as they do when I wire it up myself. I'll poke around at this until I can get a config that truly provides reducers, middlewares, and enchancers.

export default ReduxService.extend({
  makeStoreInstance({reducers, enhancers}) {
    const middleware = applyMiddleware(sagaMiddleware);
    const createStoreWithMiddleware = compose(middleware, enhancers)(createStore);
    const store = createStoreWithMiddleware(reducers);
    sagaMiddleware.run(root);
    return store;
  }
});

[bstro]

@toranb now that you mention it, I recall running into that too. I had to manually import my reducers.

[toranb]

@bstro I believe the variables we destructure in makeStoreInstance reference the "default" imports that ship with ember-redux itself (meaning the reducer/middleware/enchancer in this addon .... that do next to nothing).

@foxnewsnetwork is this how you'd expect this to work? Not a complete deal breaker obviously, just trying to understand how those vars coming into makeStoreInstance should work/ what the developer experience is like using them

[bstro]

@toranb Doesn't that make the params to makeStoreInstance effectively useless?

[bstro]

@toranb I don't think its a big deal to manually import reducers and enhancers...

[toranb]

@bstro I'm personally good with this myself (just wasn't sure if that was the expectation when I took my first stab at extending it). This does have the added benefit of "no breaking change" for people in the 2X series. Plus it still allows us to provide a default middleware using redux-thunk without the user even knowing it was required (often the very first question people new to redux ask after "hello world" when they attempt something async).

When I look at the source in /app/services/redux.js I see that we are importing all 3 at the top and pass those in like so ...

import ReduxService from 'ember-redux/services/redux';
import reducers from '../reducers/index';
import enhancers from '../enhancers/index';
import middlewares from '../middleware/index';

export default ReduxService.extend({ reducers, enhancers, middlewares });

Making me think it's logical that anyone extending this would then be required to import reducers/middlewares/enhancers or whatever they need to create the redux store manually.

TL;DR

I think this all makes sense and with a legit documentation section (under configuration) we could easily show how one might bolt on a custom middleware, or reducer, or enhancer.

I'll give @brettburley and @foxnewsnetwork time next week to think this over and provide feedback. I'm often in my own little world ;) so I'll step back and let them reflect on this (now) massive PR (sorry in advance guys)

[toranb]

@bstro one last look at how we could show this in action. I took a step back and found this worked perfectly (as I import the reducers/enhancers myself and construct the saga middleware by hand)

import redux from 'redux';
import ReduxService from 'ember-redux/services/redux';
import createSagaMiddleWare from 'redux-saga';
import reducers from '../reducers/index';
import enhancers from '../enhancers/index';
import root from '../sagas/index';

const { createStore, applyMiddleware, compose } = redux;

const sagaMiddleware = createSagaMiddleWare();

const makeStoreInstance = ({reducers, enhancers}) => {
  const middleware = applyMiddleware(sagaMiddleware);
  const createStoreWithMiddleware = compose(middleware, enhancers)(createStore);
  const store = createStoreWithMiddleware(reducers);
  sagaMiddleware.run(root);
  return store;
};

export default ReduxService.extend({
  reducers,
  enhancers,
  makeStoreInstance
});

https://github.com/toranb/configme243/commit/2dbd770fbc30f671577bfb945f257a2f3e2385cd

Update

working example using ember-cli v2.13.1

https://github.com/toranb/configme213/commits/master

working example using ember-cli v2.14.0-beta.2

https://github.com/toranb/configme214/commits/master

Update 2

I've added an example to the docs/configuration page and rebased this branch

[foxnewsnetwork]

As far as I can tell, this LGTM; just be sure to at least bump minor (if not major) version so that we can always patch if users come up with unforeseen problems

P.S.: great work, m8!

[bstro]

your usage example looks great to me. +1 -- thanks for your work on this! all your work has been super helpful for me and several people on my team.

[toranb]

@bstro @brettburley @foxnewsnetwork this has been released as part of ember-redux v2.10

[bstro]

Nice!!

[codecov-commenter]

Codecov Report

Merging #121 (b5c71c5) into master (a265480) will decrease coverage by 0.49%. The diff coverage is 82.05%.

:exclamation: Current head b5c71c5 differs from pull request most recent head 9711559. Consider uploading reports for the commit 9711559 to get more accurate results

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   79.24%   78.74%   -0.50%     
==========================================
  Files          17       17              
  Lines         954      974      +20     
==========================================
+ Hits          756      767      +11     
- Misses        198      207       +9     

Impacted Files	Coverage Δ
chaosk8s/deployment/actions.py	85.36% <75.00%> (-4.64%)	:arrow_down:
chaosk8s/replicaset/actions.py	82.35% <75.00%> (-11.40%)	:arrow_down:
chaosk8s/statefulset/actions.py	92.68% <75.00%> (-2.32%)	:arrow_down:
chaosk8s/pod/actions.py	92.23% <76.92%> (-3.56%)	:arrow_down:
chaosk8s/__init__.py	86.74% <92.85%> (+1.61%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 897c38d...9711559. Read the comment docs.