Convert add-on detail page to use sagas #2602

kumar303 · 2017-06-16T21:54:37Z

Here is what it looks like before the add-on has loaded:

kumar303 · 2017-06-16T21:57:38Z

@tofumatt for some reason the <LoadingText> component in the title doesn't show up. I may need some help to figure that one out. Is it Flex weirdness since they render as empty divs?

kumar303 · 2017-06-19T16:55:16Z

Whoops, I forgot to handle 404s. This is not ready for a review yet.

kumar303 · 2017-06-19T20:14:48Z

@tofumatt 404s are implemented now. It's ready for review.

tofumatt

I've left some comments and questions but largely this looks great.

I'm gonna check it out locally and see about the loading indicator.

I think other than some nits or little improvements my main worry is with the addon component not dispatching a new viewContext when it's updated without being remounted. eg If you were on a theme and you linked to an extension, setViewContext wouldn't be dispatched because the component had already mounted and the router would simply updated its props.

The LandingPage component handles this kind of scenario and I think all of our components will need to do this–if we don't then components that dispatch on update won't work as expected when they link to themselves, if that makes sense.

https://github.com/mozilla/addons-frontend/blob/master/tests/unit/amo/components/TestLandingPage.js#L47 is an example of testing it from the LandingPage.

I'll have a look through and make suggestions on LoadingText and such tonight or tomorrow.

tofumatt · 2017-06-16T23:17:12Z

src/amo/components/Addon/index.js

 import InstallButton from 'core/components/InstallButton';
-import { ADDON_TYPE_THEME, ENABLED, UNKNOWN } from 'core/constants';
+import {
+  ADDON_TYPE_EXTENSION, ADDON_TYPE_THEME, ENABLED, UNKNOWN,


Nit but once we start doing more imports than fit on a line it's one-per-line eg:

import { ADDON_TYPE_EXTENSION, ADDON_TYPE_THEME, ENABLED, UNKNOWN, } from 'core/constants';

Though I really wish we could have a rule for that...

Nit but once we start doing more imports than fit on a line it's one-per-line

it's pretty easy to fix once they can't fit on a line anymore

tofumatt · 2017-06-19T23:40:50Z

src/amo/components/Addon/index.js

@@ -73,9 +80,13 @@ export class AddonBase extends React.Component {
  }

  componentWillMount() {


I notice there's nothing here for componentDidUpdate–if an add-on linked to another add-on the components on the page wouldn't change but the props would update and we would want to do things like update the viewContext.

Of course, it would be nice if we did that more often with routing–as in this case we could get away with using the URL to know addonType but for now we should dispatch(setViewContext()) on componentDidUpdate.

This is fixed now

Thanks; sorry it was an awkward place to leave a comment as I know it won't be cleared. 😅

tofumatt · 2017-06-19T23:42:42Z

src/amo/components/Addon/index.js

-        endSpan: '</span>',
-      });
+    let errorBanner = null;
+    if (errorHandler.hasError()) {


I haven't seen this HOC used much but that's really nice in usage. 👍

tofumatt · 2017-06-19T23:44:48Z

src/amo/components/Addon/index.js

@@ -289,9 +361,13 @@ export class AddonBase extends React.Component {
 }

 export function mapStateToProps(state, ownProps) {
+  // TODO: make this function handle a falsy addon


Seems like this checks for a falsy add-on below–is this TODO still needed?

tofumatt · 2017-06-19T23:49:07Z

src/core/sagas/addons.js

+import { getApi } from './utils';
+
+
+export function* fetchAddon(


I'm only commenting here to say this code is very easy to follow and is making me happy we're moving to sagas. It makes our async code feel a bit more idiomatic too, compared to the other stuff which was a bit less consistently structured. 👍

tofumatt · 2017-06-19T23:49:21Z

tests/unit/amo/components/TestAddon.js

@@ -1,4 +1,5 @@
 /* global window */
+import { shallow } from 'enzyme';


tofumatt · 2017-06-19T23:51:36Z

tests/unit/amo/components/TestAddon.js

+      fakeDispatch, fetchAddonAction({ errorHandler, slug: slugParam }));
+
+    // These should be empty:
+    expect(root.find(InstallButton)).toHaveLength(0);


Oh cool, I didn't know you could use components; I've always been using selectors. That's awesome.

tofumatt · 2017-06-19T23:53:39Z

tests/unit/amo/components/TestAddon.js

+
+    // These should be empty:
+    expect(root.find(InstallButton)).toHaveLength(0);
+    expect(root.find(AddonCompatibilityError)).toHaveLength(0);


Not sure if https://github.com/blainekasten/enzyme-matchers#tobeempty is a better fit for these tests, but it's up to you. I think I've written tests with expect(root.find('.selector')).toHaveLength(0); before and I'm fine with it.

I do like that toHaveLength(0) and toHaveLength(1) contrast well with each other in this test though, so that's a reason to keep using it over the toBeEmpty() matcher.

tofumatt · 2017-06-19T23:56:42Z

tests/unit/amo/components/TestAddon.js

+
+  it('renders 404 page for missing add-on', () => {
+    const id = 'error-handler-id';
+    const store = createStore().store;


Could use const { store } = createStore(); here... and I'd argue we should try to just use dispatchClientMetadata() over raw createStore() wherever possible because it loads data we can use if we ever have to and is just as easy to use.

But up to you on the usage of createStore() vs the dispatch helpers. Obviously these tests are mostly not dispatching things so it doesn't really matter.

tofumatt · 2017-06-20T00:06:01Z

Looks like there are style regressions from this patch, likely to do with the change in the HTML output by the Addon component. A theme should look like:

But in your patch looks like:

tofumatt

Just added a few more comments after running it locally. Looks like there are style regressions for both extension and theme detail pages; if you compare them to master you'll see things aren't quite right. I didn't yet explore why but give me a shout if it's not obvious and I can track it down.

Once the styles are back in sync let me know if LoadingText is still an issue.

One recommendation, kind of unrelated to this patch, is to change <LoadingText /> to output a <span> rather than a <div>, as I saw some complaints in the console if we put <LoadingText /> inside a <p> (react doesn't like putting a DIV in a P tag). So changing it might be better... I can just do that in a separate PR though if you'd rather :-)

tofumatt · 2017-06-20T09:39:21Z

src/amo/components/Addon/index.js

+      const summary = addon.summary ? addon.summary : addon.description;
+      summaryProps.dangerouslySetInnerHTML = sanitizeHTML(summary, ['a']);
+    } else {
+      summaryPlaceholder = <LoadingText />;


If someone thinks they're clever and tries to remove this else block like I did locally, and just set the placeholder, we'll get an invariant error because React won't allow .children and .dangerousSetInnerHTML. I think it might be nice to just remove the placeholder variable and set summaryProps.children = <LoadingText /> here.

Whoa, using .children makes everything way easier!

tofumatt · 2017-06-20T09:40:54Z

src/amo/components/Addon/index.js

+      // Themes lack a summary so we do the inverse :-/
+      // TODO: We should file an API bug about this...
+      const summary = addon.summary ? addon.summary : addon.description;
+      summaryProps.dangerouslySetInnerHTML = sanitizeHTML(summary, ['a']);


I think we should remove the /* eslint-disable react/no-danger */ at the top of the file and around the render() method–instead we should just use // eslint-disable-next-line react/no-danger, as it's a good thing to call out and recognise every time we use it 😄

actually, we don't even need it because the linter isn't smart enough to follow the summaryProps object 😆

Oh no! 😆

tofumatt · 2017-06-20T09:43:51Z

src/amo/components/Addon/index.js

+      );
+      titleProps.dangerouslySetInnerHTML = sanitizeHTML(title, ['a', 'span']);
+    } else {
+      titlePlaceholder = <LoadingText />;


I tried locally and I think this would benefit from a higher minWidth which is a property on <LoadingText /> – it sets the minimum % the text block will fill. As titles aren't usually a few letters something like 70% or even 100% might work better for a lot of these short fields.

tofumatt · 2017-06-20T09:47:31Z

src/amo/components/Addon/index.js

+              <h2 className="visually-hidden">
+                {i18n.gettext('Extension Metadata')}
+              </h2>
+              {addon ? <AddonMeta addon={addon} /> : <LoadingText />}


I think this would benefit from AddonMeta being able to handle an empty addon and rendering each section as <LoadingText /> instead of just one bar, eg:

if (!addon) { return ( <div className="AddonMeta"> <div className="AddonMeta-item AddonMeta-users"> <h3 className="visually-hidden"><LoadingText /></h3> <p className="AddonMeta-text"><LoadingText /></p> <p className="AddonMeta-text AddonMeta-review-count"> <LoadingText /> </p> <Rating className="AddonMeta-Rating" rating={0} readOnly styleName="small" /> </div> </div> ); }

Though probably better implemented in the component like you did in Addon so we aren't duplicating the HTML.

tofumatt · 2017-06-20T09:48:49Z

src/amo/components/Addon/index.js

          {this.renderRatingsCard()}

-          <AddonMoreInfo addon={addon} />
+          {addon ? <AddonMoreInfo addon={addon} /> : null}


Same here as for AddonMeta; handling an empty addon would be good and we should render this component with the LoadingText. That way fewer elements appear out of nowhere once the add-on loads and the page looks less empty whilst loading.

AddonMoreInfo was pretty tough to convert. I don't think it's worth the effort right now.

It'd be good to file an issue then. I find it weird that it says "no reviews" while it's loading. The empty review stars are fine (that's actually what I'd stick with) but the "no reviews" instead of <LoadingText /> isn't the best UX:

The 'No reviews yet' is fixed.

As for AddonMoreInfo, I don't think it's worth spending time on but, sure, I filed and marked it as contrib: welcome https://github.com/mozilla/addons-frontend/issues/2629

kumar303 · 2017-06-20T20:12:33Z

@tofumatt I fixed the style regressions, thanks for catching that. It should be ready for another look now.

tofumatt

🎉

A few changes requested but I don't think any of them should require another review and I know you want to get other stuff through after this.

I tested it locally and it feels great. Really nice experience going from the search results.

r+wc

tofumatt · 2017-06-22T20:18:27Z

src/amo/components/Addon/index.js


-    dispatch(setViewContext(addon.type));
+  componentWillReceiveProps({ addon: newAddon }) {
+    const { addon: oldAddon, dispatch } = this.props;


Woooah I've never seen this syntax! I'm guessing it's the same as:

const addon = this.props.oldAddon; const dispatch = this.props.dispatch;

Neat-o, just didn't know about it.

I don't think it's the greatest syntax. It's actually a shortcut for this which is different from the code you posted:

const oldAddon = this.props.addon; const dispatch = this.props.dispatch;

Ohh, wow. That's not what I expect, making it a bit hard to read. Maybe it would be nicer to do:

const { dispatch } = this.props; const addon = this.props.oldAddon;

I just find that syntax opposite to what I expect.

I don't think we should add workarounds for standard JavaScript syntax. It is what it is and it's well documented. It will be around for a long time :) We can help teach contributors if they get stuck with it.

Plus, your workaround suggestion adds an extra line of code -- less code is always easier to read IMO.

Fair enough!

I don't think it's easier to read, but it is a standard syntax so at least someone can look it up. I think there are language features we don't use because they aren't pleasant and I'd nominate this one as well... I think it's easier to reason about what this is doing with the two lines of code, and it's not logic just assignment so I think it doesn't add complexity.

Once I got used to it it no longer seemed foreign. I really wish it looked more like this but oh well:

const { addon as oldAddon } = this.props;

^ This is how I read it in my head now which helps me parse it.

omg, agreed. Why didn't they just do that?!

That actually helps though, thanks 👍

tofumatt · 2017-06-22T20:43:02Z

src/amo/components/Addon/index.js

-          version={addon.current_version}
-        />
+        {addon ?
+          <RatingManager


I think it should be done in another patch, I guess the Reviews one maybe but it would be great if this showed LoadingText in place of "No reviews yet" as well. Anyway that's just a note/UX thing to keep in mind, this patch is big enough now and no need to introduce more complexity.

Actually, yeah, I was thinking about this too. The 'No reviews yet' is an answer to a question asked, which is 'how many reviews are there for this add-on?' In this case, it is not the right answer to give. I'll see if I can change it without much churn.

I commented below–it looks like it should be straightforward.

But if it becomes a beast I'm happy to wait for another patch.

tofumatt · 2017-06-22T20:47:47Z

src/amo/components/Addon/index.js

          {this.renderRatingsCard()}

-          <AddonMoreInfo addon={addon} />
+          {addon ? <AddonMoreInfo addon={addon} /> : null}


It'd be good to file an issue then. I find it weird that it says "no reviews" while it's loading. The empty review stars are fine (that's actually what I'd stick with) but the "no reviews" instead of <LoadingText /> isn't the best UX:

tofumatt · 2017-06-22T20:48:42Z

src/amo/components/AddonMeta.js

-    const averageRating = addon.ratings.average;
-    const addonRatingCount = addon.ratings.count;
+    const averageRating = addon ? addon.ratings.average : null;
+    const addonRatingCount = addon ? addon.ratings.count : false;


This is hyper-semantic but is the count being set to false really correct? It's more null than false. To me null signifies we don't have any info yet while 0 signifies a zero count. false feels different than either of those things and more deliberate.

tofumatt · 2017-06-22T20:50:36Z

src/amo/components/AddonMeta.js

+        { total: i18n.formatNumber(averageDailyUsers) },
+      );
+    } else {
+      userCount = <LoadingText />;


This could probably have a high width because even "2 users" is a pretty long string and usually fills up most of that space. When a short length LoadingText shows here it looks a bit odd:

tofumatt · 2017-06-22T20:56:37Z

tests/unit/amo/components/TestAddon.js

+    const fakeDispatch = sinon.stub();
+    const root = render({ dispatch: fakeDispatch });
+    fakeDispatch.reset();
+    root.componentWillReceiveProps({


With enzyme I think you can even do root.setProps({ addon: { [...] } }) and it will trigger componentWillReceiveProps... might be a bit more natural than calling it directly and it simulates real behaviour a bit more.

oops, I forgot to convert the rest of this suite to use Enzyme. Heading down the rabbit hole, brb! 😓

Cool, let me know if you think it warrants another review; should be easy to do and quick!

tofumatt · 2017-06-22T20:57:48Z

tests/unit/amo/components/TestAddon.js

+    const fakeDispatch = sinon.stub();
+    const root = render({ addon: fakeAddon, dispatch: fakeDispatch });
+    fakeDispatch.reset();
+    // Update with the same addon (this apparently happens IRL).


Can you change IRL to in usage? The abbreviation might be tough for an ESL speaker.

Also: it wouldn't be the same add-on, it would be a different one but the component would stay "mounted" just receiving entirely new props. So if you had a theme that linked to an extension, the props would update but the component would not call componentWillMount.

So really this is simulating "update the component with a new add-on of a different type".

Surprisingly, it does update with the same add-on for some reason. I discovered this because the page got stuck in an infinite loop and crashed Firefox.

Ah, weird. I think componentDidUpdate is only called if the props changed whereas componentWillUpdate is called either way maybe? My bad if I suggested the wrong one. We should double-check we're using the right one according to the lifecycle docs: https://facebook.github.io/react/docs/react-component.html#the-component-lifecycle

tofumatt · 2017-06-22T21:00:16Z

tests/unit/amo/components/TestAddon.js

+    const fakeDispatch = sinon.stub();
+    const root = render({ dispatch: fakeDispatch });
+    fakeDispatch.reset();
+    root.componentWillReceiveProps({});


I think this can use root.setProps({}) as well.

tofumatt · 2017-06-22T21:03:45Z

tests/unit/ui/components/TestLoadingText.js

@@ -0,0 +1,24 @@
+import { shallow } from 'enzyme';


Damn, did this not have any tests before? I'm sorry 😳

Oh, it did. This test file was just never renamed after the component changed names (all I did here was rename it).

Oh, cool! Thanks!

tofumatt · 2017-06-22T21:04:59Z

tests/unit/ui/components/TestLoadingText.js

+
+  it('lets you set a fixed width', () => {
+    const root = render({ width: 55 });
+    expect(root.prop('style')).toMatchObject({ width: '55%' });


This can be expect(root).toHaveProp('style', { width: '55%' }); because I don't think we expect the component to be setting style props other than width.

But if you want to leave it as is I suppose it's more extensible. Up to you.

I think it helps for maintenance to keep assertions isolated to exactly what they are testing. In this case, the test is only checking for the width style rule. It should be flexible enough to allow new style rules in the future without the test breaking.

Cool. I do really like toMatchObject for that 👍

kumar303 · 2017-06-23T18:19:28Z

Thanks for all the reviews.

kumar303 added 10 commits June 14, 2017 17:09

Handle case when add-on hasn't been loaded yet

a9d26d7

Merge branch 'master' into addon-saga-iss2442

a92b898

Make sure addon.type doesn't get dispatched

c0e2e14

Happy path of fetching an add-on works via saga

adb03c9

Merge branch 'master' into addon-saga-iss2442

0db59f3

Merge branch 'master' into addon-saga-iss2442

3c9aa84

handle errors and also fix displaying 502 proxy errors

5bbe60a

move the add-ons saga to core

91a8fb6

Merge branch 'master' into addon-saga-iss2442

e413efc

fix merge fallout

577a751

kumar303 added the state:pull_request_ready label Jun 16, 2017

kumar303 requested a review from tofumatt June 16, 2017 21:56

kumar303 removed the request for review from tofumatt June 19, 2017 16:55

Handle add-on 404s

dca6cec

kumar303 requested a review from tofumatt June 19, 2017 20:14

tofumatt suggested changes Jun 20, 2017

View reviewed changes

kumar303 added 9 commits June 20, 2017 11:20

Merge branch 'master' into addon-saga-iss2442

7ba50a1

Synced AddonDetail HTML changes with master

da0d1fa

remove no-danger lint comment

bf94cb8

Use .children, duh

c47ae07

Add a fixedWidth prop to LoadingText

845e67d

Rename fixedWidth prop to width

1fc454b

Use fixed width placeholders

97d7676

Converted TestAddonMeta to use shallow(), fixed lint

4663cad

Let AddonMeta render with a null addon

f74bda5

Clean up some TODOs

72d3ce7

kumar303 requested a review from tofumatt June 20, 2017 20:12

kumar303 added 5 commits June 20, 2017 15:35

update view context when props get updated

dcf1537

Use shorthand for store

03e5277

Only set view context on update when necessary

6865526

Simpler code to check for addon type changes

ae14938

Add missing tests for error handler action creators

2dd84c7

tofumatt approved these changes Jun 22, 2017

View reviewed changes

kumar303 added 2 commits June 23, 2017 09:45

Merge branch 'master' into addon-saga-iss2442

7273f81

Change 'No reviews yet' to LoadingText

95f0e82

kumar303 mentioned this pull request May 5, 2024

Show placeholders for 'more info' on Add-on detail page when loading mozilla/addons#10511

Closed

kumar303 added 4 commits June 23, 2017 10:23

false -> null

da92132

use fixed width for user count placeholder

c4194a9

Convert most of TestAddon to Enzyme

1069c03

fix abbreviation

59db61f

kumar303 merged commit 6b01159 into mozilla:master Jun 23, 2017

kumar303 deleted the addon-saga-iss2442 branch June 23, 2017 18:19

kumar303 removed the state:pull_request_ready label Jun 23, 2017

		@@ -73,9 +80,13 @@ export class AddonBase extends React.Component {
		}

		componentWillMount() {

		import { getApi } from './utils';


		export function* fetchAddon(

		@@ -1,4 +1,5 @@
		/* global window */
		import { shallow } from 'enzyme';

Convert add-on detail page to use sagas #2602

Convert add-on detail page to use sagas #2602

Conversation

kumar303 commented Jun 16, 2017 • edited Loading

kumar303 commented Jun 16, 2017

kumar303 commented Jun 19, 2017

kumar303 commented Jun 19, 2017

tofumatt 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

tofumatt commented Jun 20, 2017

tofumatt left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 commented Jun 20, 2017

tofumatt 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

kumar303 Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt Jun 23, 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

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

Choose a reason for hiding this comment

kumar303 commented Jun 23, 2017

kumar303 commented Jun 16, 2017 •

edited

Loading

tofumatt left a comment •

edited

Loading

kumar303 Jun 23, 2017 •

edited

Loading

tofumatt Jun 23, 2017 •

edited

Loading