Add better return types for the API functions #218

willdurand · 2019-02-15T14:59:48Z

Fixes #181

This PR allows us to write code like this now: https://github.com/mozilla/addons-code-manager/pull/218/files#diff-eaeb6d1753d7786e20e90d2089ffe11e. In other words, we now have precise return types for our API calls.

The PR also adds new thunks and a bunch of test cases.

codecov-io · 2019-02-15T15:01:23Z

Codecov Report

Merging #218 into master will increase coverage by 1.28%.
The diff coverage is 94.59%.

@@            Coverage Diff            @@
##           master    #218      +/-   ##
=========================================
+ Coverage   94.02%   95.3%   +1.28%     
=========================================
  Files          22      22              
  Lines         318     341      +23     
  Branches       76      81       +5     
=========================================
+ Hits          299     325      +26     
+ Misses         18      15       -3     
  Partials        1       1

Impacted Files	Coverage Δ
src/test-helpers.tsx	`100% <100%> (ø)`	⬆️
src/reducers/versions.tsx	`100% <100%> (ø)`	⬆️
src/api/index.tsx	`100% <100%> (ø)`	⬆️
src/pages/Browse/index.tsx	`100% <100%> (ø)`	⬆️
src/components/App/index.tsx	`100% <100%> (+29.41%)`	⬆️
src/reducers/users.tsx	`85.18% <77.77%> (-3.71%)`	⬇️

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 be2ca27...3054212. Read the comment docs.

kumar303

This seems useful for detecting errors but I'd like to not keep investing in all of this code that ignores errors. I suggested an approach that would throw all errors but if you think we should tackle that in a follow-up instead just let me know.

src/api/index.tsx

src/components/App/index.tsx

willdurand · 2019-02-18T11:41:46Z

This seems useful for detecting errors but I'd like to not keep investing in all of this code that ignores errors. I suggested an approach that would throw all errors but if you think we should tackle that in a follow-up instead just let me know.

Yes, it is time to resolve #182 now.

willdurand · 2019-02-18T13:25:26Z

Yes, it is time to resolve #182 now.

@kumar303 one idea built on top of this PR is this: 8fbbb9d

We dispatch all (known) errors and store them into a centralized notifications reducer, as "error" notifications. We could use the same reducer for other notifications, e.g., success messages.

kumar303 · 2019-02-19T17:06:42Z

@kumar303 one idea built on top of this PR is this: 8fbbb9d
...
We dispatch all (known) errors and store them into a centralized notifications reducer, as "error" notifications.

I like this approach.

I think ultimately we should go with that approach but there may be some complexities in how we deal with rendering / dismissing notices.

How about taking a baby step towards that approach like this?

    if (isErrorResponse(response)) {
      log.error(`TODO: handle this error response: ${response.error}`);
    } else {
      dispatch(versionActions.loadVersionInfo({ version: response }));
    }

willdurand · 2019-02-19T17:10:38Z

How about taking a baby step towards that approach like this?

💯 👍

I'll update this patch with that, and I'll try to think more about the other patch with the notifications reducer.

willdurand · 2019-02-19T22:32:37Z

@kumar303 so I think this is good for another look. I had to introduce a updateWithProps() helper because Enzyme does not support async componentDidUpdate()... (it does support async componentDidMount() though). I added tests for everything I touched.

src/components/App/index.spec.tsx

willdurand · 2019-02-20T21:21:54Z

@kumar303 this should be ready for another look! It's full of thunks 🌟

kumar303

This looks good. I just have some minor cleanup requests.

I miss sinon.spy(store, 'dispatch') a little. I tried wrapping up the jest methods in a helper function but couldn't figure out how to mirror the function parameters of jest.spyOn. TypeScript!

My failed attempt 😞

diff --git a/src/pages/Browse/index.spec.tsx b/src/pages/Browse/index.spec.tsx
index 48c2876..42b0871 100644
--- a/src/pages/Browse/index.spec.tsx
+++ b/src/pages/Browse/index.spec.tsx
@@ -6,6 +6,7 @@ import {
   createFakeThunk,
   fakeVersion,
   shallowUntilTarget,
+  spyOn,
 } from '../../test-helpers';
 import configureStore from '../../configureStore';
 import {
@@ -92,9 +93,7 @@ describe(__filename, () => {
     const version = fakeVersion;
 
     const store = configureStore();
-    const dispatch = jest
-      .spyOn(store, 'dispatch')
-      .mockImplementation(jest.fn());
+    const dispatch = spyOn(store, 'dispatch');
     const fakeThunk = createFakeThunk();
 
     render({
diff --git a/src/test-helpers.tsx b/src/test-helpers.tsx
index 2084dca..e3051ab 100644
--- a/src/test-helpers.tsx
+++ b/src/test-helpers.tsx
@@ -290,3 +290,7 @@ export const getFakeLogger = () => {
     info: jest.fn(),
   };
 };
+
+export const spyOn = (...args: Parameters<typeof jest.spyOn>) => {
+  return jest.spyOn(...args).mockImplementation(jest.fn());
+};

kumar303 · 2019-02-20T22:30:22Z

src/components/App/index.tsx

-  async componentDidUpdate(prevProps: Props) {
-    const { apiState, dispatch, profile } = this.props;
+  componentDidUpdate(prevProps: Props) {
+    const { apiState, profile } = this.props;

    if (!profile && prevProps.apiState.authToken !== apiState.authToken) {


These kind of if statements make me sad. We should look into simplifying it with useEffect soon.

Agreed. Should we file an issue? This patch gets too big.

Definitely. Not high priority: #246

src/components/App/index.spec.tsx

src/components/App/index.tsx

src/pages/Browse/index.tsx

src/reducers/users.spec.tsx

willdurand · 2019-02-21T10:23:43Z

I miss sinon.spy(store, 'dispatch') a little. I tried wrapping up the jest methods in a helper function but couldn't figure out how to mirror the function parameters of jest.spyOn. TypeScript!

My failed attempt 😞

I added this function. I tweaked it a bit to make it TS-compliant.

willdurand · 2019-02-21T10:32:44Z

@kumar303 thanks a lot for the last review, I hope this is good to go now.

kumar303

Thanks again for thunking all the things. This looks great

src/test-helpers.tsx

willdurand requested review from bobsilverberg and kumar303 February 15, 2019 15:08

kumar303 suggested changes Feb 15, 2019

View reviewed changes

src/api/index.tsx Show resolved Hide resolved

src/api/index.tsx Show resolved Hide resolved

src/api/index.tsx Show resolved Hide resolved

src/components/App/index.tsx Outdated Show resolved Hide resolved

willdurand force-pushed the api-return-types branch from 4680113 to 8f929f0 Compare February 18, 2019 11:40

willdurand force-pushed the api-return-types branch from 8f929f0 to fa26555 Compare February 19, 2019 22:25

willdurand requested a review from kumar303 February 19, 2019 22:33

kumar303 reviewed Feb 19, 2019

View reviewed changes

src/components/App/index.spec.tsx Outdated Show resolved Hide resolved

src/components/App/index.spec.tsx Outdated Show resolved Hide resolved

willdurand force-pushed the api-return-types branch 4 times, most recently from 3f4522d to 72b1345 Compare February 20, 2019 18:26

willdurand changed the title ~~Add better return types for the API functions~~ [WIP] Add better return types for the API functions Feb 20, 2019

willdurand mentioned this pull request Feb 20, 2019

Move loadCurrentUser in App to a thunk #237

Closed

willdurand force-pushed the api-return-types branch 2 times, most recently from 1d71bd8 to 8b2e149 Compare February 20, 2019 21:11

willdurand requested a review from kumar303 February 20, 2019 21:21

willdurand changed the title ~~[WIP] Add better return types for the API functions~~ Add better return types for the API functions Feb 20, 2019

kumar303 self-assigned this Feb 20, 2019

kumar303 mentioned this pull request Feb 20, 2019

Dispatch an actual loading state in fetchCurrentUserProfile #239

Closed

kumar303 suggested changes Feb 20, 2019

View reviewed changes

willdurand requested review from kumar303 and removed request for bobsilverberg February 21, 2019 10:29

willdurand mentioned this pull request Feb 21, 2019

Show the default file in the browse view #243

Merged

kumar303 approved these changes Feb 21, 2019

View reviewed changes

src/test-helpers.tsx Show resolved Hide resolved

willdurand added 4 commits February 21, 2019 20:08

Add better return types for the API functions

94b52c2

Add thunks

acdc70c

fixes

2d8ec4f

add missing env var

3054212

willdurand force-pushed the api-return-types branch from d955f2b to 3054212 Compare February 21, 2019 19:13

willdurand merged commit c935598 into master Feb 21, 2019

willdurand deleted the api-return-types branch February 21, 2019 19:22

Add better return types for the API functions #218

Add better return types for the API functions #218

Uh oh!

Conversation

willdurand commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kumar303 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willdurand commented Feb 18, 2019

Uh oh!

willdurand commented Feb 18, 2019

Uh oh!

kumar303 commented Feb 19, 2019

Uh oh!

willdurand commented Feb 19, 2019

Uh oh!

willdurand commented Feb 19, 2019

Uh oh!

Uh oh!

Uh oh!

willdurand commented Feb 20, 2019

Uh oh!

kumar303 left a comment

Choose a reason for hiding this comment

Uh oh!

kumar303 Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

willdurand Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

kumar303 Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willdurand commented Feb 21, 2019

Uh oh!

willdurand commented Feb 21, 2019

Uh oh!

kumar303 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

willdurand commented Feb 15, 2019 •

edited

Loading

codecov-io commented Feb 15, 2019 •

edited

Loading