Make add-on reducer state more explicit #3073

kumar303 · 2017-09-06T22:28:32Z

This will hopefully make the central add-on reducer easier to deal with for features and bug fixes but it has potential to break a lot of stuff. It would help if you could test out add-on/theme installs. I've manually tested AMO and the Discopane.

codecov-io · 2017-09-06T22:36:33Z

Codecov Report

Merging #3073 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#3073      +/-   ##
==========================================
+ Coverage   95.02%   95.06%   +0.04%     
==========================================
  Files         152      152              
  Lines        2773     2778       +5     
  Branches      541      543       +2     
==========================================
+ Hits         2635     2641       +6     
+ Misses        119      118       -1     
  Partials       19       19

Impacted Files	Coverage Δ
src/core/types/addons.js	`0% <ø> (ø)`	⬆️
src/core/constants.js	`100% <ø> (ø)`	⬆️
src/core/actions/index.js	`100% <ø> (ø)`	⬆️
src/disco/sagas/disco.js	`100% <100%> (ø)`	⬆️
src/core/utils.js	`100% <100%> (ø)`	⬆️
src/core/reducers/addons.js	`100% <100%> (+3.57%)`	⬆️
src/core/sagas/addons.js	`100% <100%> (ø)`	⬆️

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 721f43d...7061c1a. Read the comment docs.

willdurand

I have a few questions.

willdurand · 2017-09-07T10:04:06Z

src/core/reducers/addons.js



+export const ADDONS_LOADED = 'ADDONS_LOADED';


Can we have LOAD_ADDON instead? See: #3025 (comment)

ah, right, thanks

willdurand · 2017-09-07T10:08:05Z

src/core/reducers/addons.js

+    name: apiAddon.name,
+    previews: apiAddon.previews,
+    public_stats: apiAddon.public_stats,
+    ratings: apiAddon.ratings || undefined,


Why does this one need this?

ah, whoops, good catch. That was leftover from me battling with Flow :/

willdurand · 2017-09-07T10:10:03Z

src/core/reducers/addons.js

 }

-export default function addon(
+const initialState = {};


No Flow type here?

There is a type for it -- you can see it assigned in the function signature: https://github.com/mozilla/addons-frontend/pull/3073/files/39fb66595b5b8033cd43288e61a19f35d9f32dbf#diff-21aa4f8ea4fba6406233649d0ce65beaR193

willdurand · 2017-09-07T10:15:02Z

src/core/reducers/addons.js

+  return addon.guid;
+}
+
+export function removeUndefinedProps(object: Object): Object {


Is this method needed because there are spreads in different places? Otherwise I would argue that the state should have a consistent shape.

Yes it is needed for spreads. I added a comment about it below. It's going to be hard to remove the spreads and I think it's a low priority fix.

I think it's a low priority fix.

👍

tofumatt

Wow, this is super-great. It makes the code a lot more readable and easy to grok.

r+wc from me.

tofumatt · 2017-09-07T15:29:56Z

src/core/actions/index.js

-  type: string,
-|};
-
-export function loadEntities(entities: Array<Object>): LoadEntitiesAction {


tofumatt · 2017-09-07T15:31:26Z

src/core/reducers/addons.js

-type Action = Object;
-type AddonState = Object;
+export function loadAddons(
+  entities: {| addons?: ExternalAddonMap |}


As mentioned previously I think it would be great to keep flow definitions out of the function signatures if they're being included. They really crowd the function signature and make it very difficult to parse exactly what's happening inside it.

The issue I have with making them separate is that it slows down the reader. If you want to know what the type is, you have to cross-reference it from somewhere else. I understand how separation can be helpful for big definitions but this one only spans about 30 characters so I don't think that warrants separation.

tofumatt · 2017-09-07T15:32:31Z

src/core/reducers/addons.js

+
+    // These are custom properties not in the API response.
+
+    // TODO: remove this if possible. I think it was added by mistake


tofumatt · 2017-09-07T15:33:14Z

src/core/reducers/addons.js

-  return apiAddon;
+
+  if (apiAddon.current_version && apiAddon.current_version.files.length > 0) {
+    // TODO: support specific platform files.


All these comments are very helpful 🥂!

tofumatt · 2017-09-07T15:33:59Z

src/core/reducers/addons.js

+      const { addons } = action.payload;
+      const newState = { ...state };
+      Object.keys(addons).forEach((key) => {
+        newState[key] = createInternalAddon(addons[key]);


🎉💃🎉 THANK YOU THIS IS SO MUCH EASIER TO READ 🎉💃🎉

tofumatt · 2017-09-07T15:39:58Z

src/core/sagas/addons.js

 import { fetchAddon as fetchAddonFromApi } from 'core/api';
-import { FETCH_ADDON } from 'core/reducers/addons';
+import { loadAddons, FETCH_ADDON } from 'core/reducers/addons';


Nitpick but I think we usually load constants first. It technically is alphabetical that way but I guess until we have a rule for sorting imports... 😞

tofumatt · 2017-09-07T15:40:17Z

src/core/sagas/addons.js

@@ -25,7 +24,7 @@ export function* fetchAddon(
    yield put(showLoading());
    const state = yield select(getState);
    const response = yield call(fetchAddonFromApi, { api: state.api, slug });
-    yield put(loadEntities(response.entities));
+    yield put(loadAddons(response.entities));


This is just way easier to read too. Thanks a million.

tofumatt · 2017-09-07T15:41:23Z

src/core/types/addons.js

+/*
+ * This is the external API representation of an add-on.
+ *
+ * This is a detailed API response. Not all API responses


Linking to the API docs from here would be cool just so developers could understand better the varied details in responses. Maybe just http://addons-server.readthedocs.io/en/latest/topics/api/addons.html ?

tofumatt · 2017-09-07T15:43:36Z

tests/unit/core/reducers/test_addons.js

  });

-  it('sets `isRestartRequired` to `true` when at least one of the files declares it', () => {
+  it('sets `isRestartRequired` to `true` when some files declare it', () => {


if the aim here was to shorten this spec I'd say "when any file declares it".

…wap import orderswap import orderswap import orderswap import order

kumar303 · 2017-09-07T20:49:39Z

Thanks for the reviews! Let's start testing this on dev...

kumar303 added 24 commits September 1, 2017 14:26

add todo

19136c0

added todo

8c04024

shorten some lines

4913bf7

Clean up addons reducer

13fe294

add better test data, more tests

7b8dbfe

Got rid of add-on spreads

1b9afce

delete denormalizeAddon

8f5b8b2

Handle a legacy case that relies on rest/spreads

7cf3edd

Make theme_data more spread-friendly

aaf281c

cleaned up getGuid function

11e25b9

clean-up

7ade8b0

Merge branch 'master' into addons-reducer-iss3024

bcf513f

updated todo comment

f8a9857

ENTITIES_LOADED -> ADDONS_LOADED

98d329a

Renamed loadEntities() to loadAddons()

7d94692

fix typo

ccc42df

Fixed external/internal add-on types

1d83330

Fix up some comments

ef6cee5

EEsh, I don't know about removing properties yet

4c04c61

move ADDONS_LOADED to the duck

65259ef

Fix reducer state type

4eb0813

not going to fixup discopane actions right now

c2d366d

clean up

eb9bdd8

more cleanup

39fb665

kumar303 requested review from tofumatt and willdurand September 6, 2017 22:28

willdurand reviewed Sep 7, 2017

View reviewed changes

tofumatt approved these changes Sep 7, 2017

View reviewed changes

Merge branch 'master' into addons-reducer-iss3024

eea43b2

kumar303 added 5 commits September 7, 2017 15:18

rename action type to LOAD_ADDONS

57ce507

this was not needed

5376e96

swap import orderswap import orderswap import orderswap import orders…

8a073e4

…wap import orderswap import orderswap import orderswap import order

link to API docs

9b4f4dc

fix spec

7061c1a

kumar303 merged commit 8489a8e into mozilla:master Sep 7, 2017

kumar303 deleted the addons-reducer-iss3024 branch September 7, 2017 20:50

tsl143 pushed a commit to tsl143/addons-frontend that referenced this pull request Sep 12, 2017

Make add-on reducer state more explicit (mozilla#3073)

3dcc368

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make add-on reducer state more explicit #3073

Make add-on reducer state more explicit #3073

kumar303 commented Sep 6, 2017

codecov-io commented Sep 6, 2017 •

edited

willdurand left a comment

willdurand Sep 7, 2017

kumar303 Sep 7, 2017

willdurand Sep 7, 2017

kumar303 Sep 7, 2017

willdurand Sep 7, 2017 •

edited

kumar303 Sep 7, 2017

willdurand Sep 7, 2017

kumar303 Sep 7, 2017

willdurand Sep 7, 2017

tofumatt left a comment

tofumatt Sep 7, 2017

tofumatt Sep 7, 2017

kumar303 Sep 7, 2017

tofumatt Sep 7, 2017

tofumatt Sep 7, 2017

tofumatt Sep 7, 2017

tofumatt Sep 7, 2017

tofumatt Sep 7, 2017

tofumatt Sep 7, 2017

kumar303 Sep 7, 2017

tofumatt Sep 7, 2017

kumar303 commented Sep 7, 2017


		// These are custom properties not in the API response.

		// TODO: remove this if possible. I think it was added by mistake

Make add-on reducer state more explicit #3073

Make add-on reducer state more explicit #3073

Conversation

kumar303 commented Sep 6, 2017

codecov-io commented Sep 6, 2017 • edited

Codecov Report

willdurand 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

willdurand Sep 7, 2017 • edited

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 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

kumar303 commented Sep 7, 2017

codecov-io commented Sep 6, 2017 •

edited

willdurand Sep 7, 2017 •

edited