Use redux-saga #2195

tofumatt · 2017-03-27T20:28:59Z

Adds support for redux-saga and server-side rendering of sagas.

This patch also gets around redux-saga not blocking on request by simply rendering placeholder content for the components it renders, which I think is a nicer solution anyway and something we should do more of in the future. It's more manual but creates a much nicer feel and I think will make the client-side jankiness we loathe much less of an issue.

Closes mozilla/addons#10293.

I think I've cleaned this up enough it's ready for r?

Screenshot of the new loading indicator:

kumar303 · 2017-03-27T22:09:27Z

Were you able to find out if redux-saga supports an isomorphic render on the server? We should figure that out first before going too far with it.

tofumatt · 2017-03-28T07:11:11Z

Yeah, they have an issue about it and have a special END action with example usage that allows server side rendering. I'll actually need to add that before this patch is considered complete. So we should be able to do server side rendering with saga.

tofumatt · 2017-03-28T09:19:25Z

END issue is here: redux-saga/redux-saga#255

So it's implemented and people use it, apparently the docs aren't perfect but good enough, heh.

tofumatt · 2017-04-13T12:34:42Z

The test failures here are totally confounding me. Any idea what's going on?

kumar303 · 2017-04-17T17:39:47Z

The test failures here are totally confounding me. Any idea what's going on?

The error doesn't make a whole lot of sense -- I guess you don't get it locally? One thing that comes to mind is that we need to support async functions in babel somehow. If it's working for you locally already then maybe it's set up. We use the async and await keywords on web-ext but perhaps that's a different approach? For that we have to specify regenerator: true and we use the regenerator-runtime. Does addons-frontend need something like that?

tofumatt · 2017-04-17T17:52:24Z

Yeah it’s not failing locally at all, and this used to pass on Travis… but recently it seems a tonne of branches are failing, including this one once pushed again. I don’t think I have anything special set up locally that would cause it to fail on travis; all my changes are checked in.

tofumatt · 2017-05-03T15:35:21Z

Basically the only issue I've hit that isn't easily solvable is that redux-async-connect sends its END_GLOBAL_LOAD event before the saga has finished to the loading bar won't display. There doesn't seem to be a straightforward way around this but I'll look further. This should be ready for review soon and fortunately changing something over to a saga is pretty easy so I should be able to do it for search in another PR and get the loading bar working then.

tofumatt · 2017-05-05T14:33:08Z

src/amo/components/Categories.js

  render() {
    const { addonType, categories, error, loading, i18n } = this.props;

    if (loading) {
      return (
        <div className="Categories">
-          <p>{i18n.gettext('Loading...')}</p>
+          <span className="visually-hidden">{i18n.gettext('Loading…')}</span>
+          <ul className="Categories-list"


I think some of this could be refactored so the categories were just an array of <LoadingText /> elements and if loading === true they are displayed in <span> instead of <a> tags. I can refactor this to duplicate less markup I think.

tofumatt · 2017-05-05T14:38:26Z

src/amo/components/Categories.js

    error: PropTypes.bool,
    loading: PropTypes.bool.isRequired,
    i18n: PropTypes.object.isRequired,
  }

+  componentWillMount() {
+    const { addonType, categories, clientApp, dispatch } = this.props;
+    if (!Object.values(categories).length) {


I would say this is the big difference for redux-saga: our components are now expected to load/work without data and then dispatch their own actions to get said data if that's the case. For this simple case of categories it was easy and I ended up reloading them from state because they nearly never change and it made sense. For things like search I suspect there'd be more effort involved though in reality it will mostly be shuffling around existing code. But I think this component now makes a lot more sense to look at: if no categories exist (basically only happens on first load) it fetches them and shows a placeholder until then. The mechanism for loading is in the saga, yes, but I think this is a bit easier to follow.

tofumatt · 2017-05-05T14:54:19Z

src/amo/components/Categories.js

+  componentWillMount() {
+    const { addonType, categories, clientApp, dispatch } = this.props;
+    if (!Object.values(categories).length) {
+      dispatch(showLoading());


This should have been in the saga, my bad. Will move it.

kumar303

I'm pretty excited about sagas! I had some change requests, mostly minor but a few major ones.

kumar303 · 2017-05-05T19:07:01Z

src/amo/routes.js

@@ -32,7 +32,15 @@ export default (
    <IndexRoute component={Home} />
    <Route path="addon/:slug/" component={DetailPage} />
    <Route path="addon/:addonSlug/reviews/" component={AddonReviewList} />
-    <Route path=":visibleAddonType/categories/" component={CategoryList} />
+    {/* These user routes are to make the proxy serve each URL from */}


This looks like maybe it got added back when resolving a conflict? All of these routes got removed in a611f0a#diff-c798d928b4d830910f653f823fa95de4L32

Indeed. I messed up resolving a conflict and should have kept these removed. Will fix!

kumar303 · 2017-05-05T19:08:19Z

src/amo/sagas/categories.js

+import { getApi } from './utils';
+
+
+// worker Saga: will be fired on every CATEGORIES_FETCH action.


I don't think this comment is necessary because the call to yield takeEvery(CATEGORIES_FETCH, fetchCategories) is well documented and also quite intuitive.

kumar303 · 2017-05-05T19:45:16Z

src/amo/store.js

  );
+
+  store.runSaga = sagaMiddleware.run;


Assigning this to store.runSaga is risky and I don't think it's totally necessary. It's risky because the store API is out of our control; it's defined by Redux. It could also trip someone up if they're staring at store.runSaga() and trying to find it in the Redux documentation.

I think you can do it by returning { store, sagaMiddleware } from createStore() and using it directly when you need it:

const { sagaMiddleware, store } = createStore(); // ... sagaMiddleware.run(rootSagas);

You will have to update all existing calls from...

const store = createStore();

to...

const { store } = createStore();

Ten four. There are a few examples of this usage and it was quick and easy, but you're right. I'll go through and change it to export both items, it'll be a bit noisier in the diff.

kumar303 · 2017-05-05T19:49:48Z

src/core/actions/categories.js

  return {
-    type: CATEGORIES_GET,
+    type: CATEGORIES_FETCH,
    payload: { loading: true },


👍 for removing loading from all of these action payloads. You can remove this one too because the reducer already sets loading: true. Dispatching an action with an empty payload is fine.

kumar303 · 2017-05-05T19:52:52Z

src/amo/components/Categories.js

+  componentWillMount() {
+    const { addonType, categories, clientApp, dispatch } = this.props;
+    if (!Object.values(categories).length) {
+      dispatch(categoriesFetch({ addonType, clientApp }));


When I comment this out, all the tests still pass which means it's not covered. It should have test coverage to make sure that categories are fetched when the component first renders. The way we would typically do this is to use mapDispatchToProps() to provide something like this.props.fetchCategories() and then that can be replaced with a sinon stub to make sure it gets called on first render.

Ah thanks, good catch. It works when testing in the browser too (I spent a lot of time looking at it when doing the loading text placeholder) so I knew it worked and didn't give it much thought. 👍

I ended up just passing in props directly, see: https://github.com/mozilla/addons-frontend/pull/2195/files#diff-eaa60fc7f4b8c5473dadb4f134abbe3eR85

kumar303 · 2017-05-05T21:23:33Z

src/ui/components/LoadingText/styles.scss

+  vertical-align: middle;
+}
+
+@for $percentage from 1 through 100 {


This looks like it will add 300 lines to our CSS bundle because of the duplicated rules. Can that be avoided? Could it use a CSS transition or something instead? Or could it do a gradual fade-in like a fake progress indicator?

If this is controlled only by client-side code then setting style props directly is the answer for this sort of thing, since react should poke the style properties directly and does not cause CSP issues. See also https://github.com/mozilla/addons-frontend/issues/2321

The only caveat to using style props is for server rendering because the initial server render would use inline-styles and they would be blocked by CSP.

In this particular case, at least for now, that would work as that loading text is only seen during client-side loading. Though that could change in the future if we wanted certain stuff that loaded only on the client-side.

I think I'll go ahead with a client-side only solution because this loading animation is client-side only. I'll document that fact though, just in case it confuses anyone.

kumar303 · 2017-05-05T21:26:41Z

tests/client/amo/components/TestCategories.js

+  it('maps state to props', () => {
+    const props = mapStateToProps({
+      api: { clientApp: 'android', lang: 'pt' },
+      categories: {


could you dispatch an action to produce this state instead?

Definitely. This is largely old code just moved but I'll refactor, thanks 👍

kumar303 · 2017-05-05T21:27:02Z

tests/client/amo/components/TestCategories.js

+describe('mapStateToProps', () => {
+  it('maps state to props', () => {
+    const props = mapStateToProps({
+      api: { clientApp: 'android', lang: 'pt' },


you can dispach setClient and setLang to produce this state

kumar303 · 2017-05-05T21:55:33Z

tests/client/amo/sagas/testCategories.js

+  it('should get Api from state then make API request to categories', () => {
+    const fetchCategoriesGenerator = fetchCategories();
+
+    let next = fetchCategoriesGenerator.next();


Oof, I know you are just following the docs but I think this testing approach is very fragile and not very helpful. I agree with all the same problems raised in this issue: redux-saga/redux-saga#518

I did some digging and this library looks the most promising as a solution. Do you want to try rewriting the tests with that to see how it feels?

Totally. I saw that library too and indeed even made a mistake during a refactor forgetting to re-assign next to the next result of the generator.

I'll give it a go with that lib.

kumar303 · 2017-05-05T21:57:41Z

tests/client/core/actions/test_categories.js

-  it('sets the query', () => {
-    assert.deepEqual(action.payload, params);
-  });
+  // it('sets the query', () => {


Should this be removed or put back?

Oops, should be removed! Thanks.

tofumatt · 2017-05-23T12:04:11Z

Okay! I think I fixed up everything, ready for another r?

kumar303

Everything looks great, just some minor cleanup requests

kumar303 · 2017-05-24T17:13:50Z

src/amo/components/Categories.js

+          ref={(ref) => { this.categories = ref; }}
+        >
+          {categories.map((category) => (
+            <li className="Categories-list-item" key={category.slug}>


When the array is filled with zeroes the key will be 0.slug which is undefined. I don't know if that's a big deal or not?

Probably not a huge deal but that's a good point. I'll grab the index and put that in there if category.slug is undefined.

Since it just needs to be a unique key, you could use the index for both cases

kumar303 · 2017-05-24T19:13:48Z

src/core/server/base.js

      let store;

      try {
        cookie.plugToRequest(req, res);

-        store = createStore();
+        const _store = createStore();


It's a bit weird but this should technically work since the variables are already in scope:

{ sagaMiddleware, store } = createStore();

I was getting syntax errors when I did that.

Ah, ok. In that case, renaming _store to storeData would read better to me.

kumar303 · 2017-05-24T19:49:49Z

tests/client/amo/sagas/testCategories.js

+    // Next action is showing the loading bar.
+    assert.deepEqual(calledActions[1], showLoading());
+
+    // Next action is loading the categories returned by the API.


This comment is stale

kumar303 · 2017-05-24T19:53:24Z

tests/client/amo/sagas/testCategories.js

+    mockApi.verify();
+  });
+
+  it('should yield takeEvery() for the main generator', () => {


Is this test really necessary? It doesn't seem so useful to me.

It ensures we response to all category actions instead of just one. Though actually because of how the categories API works it might be better to just have one request... I'll see.

Okay, I think I see what you were going for. I think it would be better to test the actual logic with an approach more like this:

sagaTester.dispatch(actions.categoriesFetch()); sagaTester.dispatch(actions.categoriesFetch()); // dispatch another assert.equal(sagaTester.numCalled(actions.categoriesLoad({ result })), 2);

Just to note the actual API for numCalled is to just pass the action type and not the entire { type, payload } object (as would result from this). I blindly copied this then wondered why it didn't work for about five minutes, hah!

It's actually:

sagaTester.dispatch(actions.categoriesFetch()); sagaTester.dispatch(actions.categoriesFetch()); assert.equal(sagaTester.numCalled(CATEGORIES_LOAD), 2);

kumar303 · 2017-05-24T19:55:11Z

tests/client/amo/sagas/testCategories.js

+      .withArgs({
+        api: { ...initialState.api },
+      })
+      .throws(error);


Since you already have a test that verifies how the API is called, I think you could simplify this to:

mockApi.throws(error);

Also, you won't need to call mockApi.verify().

mockApi.throws(error); seems to cause an error because mockApi.throws is not a function. I can trim it down to mockApi.expects('categories').throws(error);

kumar303 · 2017-05-24T20:01:55Z

tests/client/amo/sagas/testIndex.js

+
+
+describe('amo rootSagas', () => {
+  it('should get Api from state then make API request to categories', () => {


is this descripition accurate?

Oops, no, holdover from when the test was different.

kumar303 · 2017-05-24T20:03:06Z

tests/client/amo/sagas/testIndex.js

+    const sagaGenerator = rootSagas();
+
+    assert.deepEqual(sagaGenerator.next().value, [
+      fork(categoriesSaga),


Hmm, this test doesn't seem so useful. It looks like it will be hard to maintain, especially as sagas grow. Maybe you could just call sagaGenerator.next() to make sure no errors are thrown?

We do essentially the same thing for reducers right now... though we might be able to use the redux saga testing utils to make it better. I forgot to use them in this test. I'll see about that and failing that do your thing, yeah.

I know, the current test we have for registering reducers feels a bit overkill for me. I get how we want to add coverage to this code but other than that it feels like a duplication of effort.

Fair enough actually and I think doing rootSagas().next() does not throw is fine. I did that and coverage remained the same; I agree that's a better and more maintainable test. 👍

kumar303 · 2017-05-24T20:04:52Z

tests/client/amo/sagas/testUtils.js

+  it('should return API state', () => {
+    const state = { api: signedInApiState, somethingElse: true };
+
+    assert.deepEqual(getApi(state), signedInApiState);


This would be a better test if it used the SagaTester. I don't know how hard that is but the redux-saga-tester docs have some good examples of adding a saga function to the tester.

kumar303 · 2017-05-24T20:07:51Z

tests/client/core/reducers/test_categories.js

+        [ADDON_TYPE_LANG]: [],
+        [ADDON_TYPE_OPENSEARCH]: [],
+        [ADDON_TYPE_THEME]: [],
+      },


Instead of hard coding this list, I think it would be better to call emptyCategoryList() directly. I don't think you gain much by hard coding all the values.

kumar303 · 2017-05-24T20:08:10Z

tests/client/core/reducers/test_categories.js

+          [ADDON_TYPE_LANG]: [],
+          [ADDON_TYPE_OPENSEARCH]: [],
+          [ADDON_TYPE_THEME]: [],
+        },


same comment here about emptyCategoryList()

tofumatt · 2017-05-25T01:46:11Z

Okay, I think I addressed all the comments and tightened up the tests. I realised as well I was using signedInApiState which while I know technically isn't too bad to use, is still raw state and I want to get away from using it so I used the store and dispatched the actions. Yay!

kumar303 · 2017-05-25T14:40:47Z

I was using signedInApiState ... and I want to get away from using it so I used the store and dispatched the actions

Good call! That was a bad habit I introduced as a baby step to DRY'ing up the state-based tests. We should fix that everywhere one day.

kumar303

tofumatt added the state:pull_request_ready label Mar 27, 2017

tofumatt force-pushed the redux-saga-2150 branch from 17c67d3 to 3dd15de Compare March 29, 2017 10:49

tofumatt force-pushed the redux-saga-2150 branch from 3dd15de to 51947b4 Compare April 5, 2017 13:17

tofumatt force-pushed the redux-saga-2150 branch from 51947b4 to 1fe9386 Compare April 12, 2017 15:05

tofumatt force-pushed the redux-saga-2150 branch 2 times, most recently from 452fbac to d5347a5 Compare April 27, 2017 15:41

kumar303 mentioned this pull request May 3, 2017

Re-implement local state fallback with Redux for better integration mozilla/addons#10362

Closed

tofumatt requested a review from kumar303 May 5, 2017 13:44

tofumatt force-pushed the redux-saga-2150 branch 2 times, most recently from 33d9ed3 to 3537eba Compare May 5, 2017 13:51

tofumatt commented May 5, 2017

View reviewed changes

tofumatt changed the title ~~WIP: Use redux-saga~~ Use redux-saga May 5, 2017

kumar303 mentioned this pull request May 5, 2017

Introduce redux-thunk to get implicit dispatching mozilla/addons#10361

Closed

kumar303 suggested changes May 5, 2017

View reviewed changes

kumar303 mentioned this pull request May 5, 2024

Do all pre-loading with redux-saga, not redux-async-connect mozilla/addons#10365

Closed

tofumatt force-pushed the redux-saga-2150 branch 2 times, most recently from 0d7adb2 to 0ab05db Compare May 23, 2017 10:12

kumar303 self-requested a review May 23, 2017 16:17

kumar303 suggested changes May 24, 2017

View reviewed changes

tofumatt requested a review from kumar303 May 25, 2017 01:45

tofumatt force-pushed the redux-saga-2150 branch from d715862 to 1b6d8b8 Compare May 25, 2017 01:46

kumar303 approved these changes May 25, 2017

View reviewed changes

feat: Use redux-saga (fix #2150)

c74a895

tofumatt force-pushed the redux-saga-2150 branch from 1b6d8b8 to c74a895 Compare May 25, 2017 15:14

tofumatt merged commit 1bc558d into master May 25, 2017

tofumatt deleted the redux-saga-2150 branch May 25, 2017 15:30

tofumatt removed the state:pull_request_ready label May 25, 2017

		import { getApi } from './utils';


		// worker Saga: will be fired on every CATEGORIES_FETCH action.



		describe('amo rootSagas', () => {
		it('should get Api from state then make API request to categories', () => {

Use redux-saga #2195

Use redux-saga #2195

Conversation

tofumatt commented Mar 27, 2017 • edited Loading

kumar303 commented Mar 27, 2017

tofumatt commented Mar 28, 2017 via email

tofumatt commented Mar 28, 2017

tofumatt commented Apr 13, 2017

kumar303 commented Apr 17, 2017

tofumatt commented Apr 17, 2017 via email

tofumatt commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented May 23, 2017

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented May 25, 2017

kumar303 commented May 25, 2017

kumar303 left a comment

Choose a reason for hiding this comment

tofumatt commented Mar 27, 2017 •

edited

Loading

tofumatt May 9, 2017 •

edited

Loading

kumar303 May 24, 2017 •

edited

Loading