feat(BBD 842): Improper error handling in api-javascript by Filip-F · Pull Request #81 · groupby/api-javascript

Filip-F · Nov 15, 2017

Labeling feat since we're also switching from axios to fetch-ponyfill.

codecov-io · Nov 15, 2017

Codecov Report

Merging #81 into develop will increase coverage by 4.08%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop      #81      +/-   ##
===========================================
+ Coverage    95.08%   99.17%   +4.08%     
===========================================
  Files            3        3              
  Lines          224      241      +17     
  Branches        19       21       +2     
===========================================
+ Hits           213      239      +26     
+ Misses           1        0       -1     
+ Partials        10        2       -8

Impacted Files	Coverage Δ
src/core/bridge.ts	`100% <100%> (+5.55%)`	⬆️
src/core/query.ts	`98.42% <0%> (+3.14%)`	⬆️

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 528ac3d...acd83a8. Read the comment docs.

jatkinson10

I’m good with this. Can i get a quick demo?

This reverts commit a45cea8.

This reverts commit 7e599ba.

zfcb · Nov 15, 2017

@@ -1,4 +1,5 @@
 import * as axios from 'axios';


zfcb · Nov 15, 2017

+
+      expect(bridge.config).to.eql({
+        timeout: 4000
+      });


object can be one line

zfcb · Nov 15, 2017

+          _id: id,
+          _u: url,
+          _t: title,
+        };


object can be one line

Too many keys for a one line object.

zfcb · Nov 15, 2017

+    });
+    bridge.search(new Query('skirts')).catch((err) => {
+      expect(err).to.be.an.instanceof(BridgeTimeout);
+      expect(err.message).to.eql('Timed out in 1500 ms');


may want to comment that this is the default timeout value

effervescentia

some changes

effervescentia · Nov 15, 2017


  constructor(config: BridgeConfig) {
    this.config = Object.assign({}, DEFAULT_CONFIG, config);
+    this.fetch = fetchPonyfill().fetch;


is there a reason we set this in the constructor rather than with the instance variable declaration?

class AbstractBridge { fetch: fetchPonyfill = fetchPonyfill().fetch; }

effervescentia · Nov 15, 2017

 export abstract class AbstractBridge {

  config: BridgeConfig;
+  fetch: fetchPonyfill;


is this the correct type?

I'm not sure what the correct type is here, this seemed most correct to me, is there a better one?

effervescentia · Nov 15, 2017

+export class BridgeTimeout extends Error {
+  constructor (err: string) {
+    super(err);
+    Object.setPrototypeOf(this, BridgeTimeout.prototype);


Why are we calling this?

Tests were failing before when I didn't have it. Now they're not. I guess I'll just remove it.

effervescentia · Nov 15, 2017

    };
-    return axios(options)
-      .then((res) => res.data)
+    const timeoutPromise = () =>


rather than make this a function created every time fireRequest() is called, let's make this a function at the top level called createTimeoutPromise() that accepts a timeout parameter

effervescentia · Nov 15, 2017

+      });
+
+    const params = qs.stringify(queryParams);
+    url = params ? url + '?' + params : url;


let's use a template literal here

`${url}?${params}`

effervescentia · Nov 15, 2017

+
+    const params = qs.stringify(queryParams);
+    url = params ? url + '?' + params : url;
+    const response = Promise.race([this.fetch(url, options), timeoutPromise()])


I like this solution 👍 very elegant

effervescentia · Nov 15, 2017

+          return res.json();
+        } else {
+          return res.json().then((err) => {
+            throw {


We should throw another custom Error type here for clarity

Filip-F · Nov 15, 2017

+export class BridgeError extends Error {
+  constructor (statusText: string, public status: number, public data: any) {
+    super(statusText);
+    Object.setPrototypeOf(this, BridgeError.prototype);


We need this one or else the prototype is going to be the same as Error, and the getter I defined lower down won't work.

effervescentia

looks good 👍

effervescentia · Nov 15, 2017

+export class BridgeError extends Error {
+  constructor (statusText: string, public status: number, public data: any) {
+    super(statusText);
+    Object.setPrototypeOf(this, BridgeError.prototype);


effervescentia

found a couple of changes

effervescentia · Nov 15, 2017

+
+    const params = qs.stringify(queryParams);
+    url = params ? `${url}?${params}` : url;
+    const response = Promise.race([this.fetch(url, options), createTimeoutPromise(this.config.timeout)])


can just be returned directly

effervescentia · Nov 15, 2017


+export class BridgeTimeout extends Error {
+  constructor (err: string) {
+    super(err);


we lost overall coverage because of these lines, can we add a simple test to this

effervescentia · Nov 15, 2017

+
+export class BridgeError extends Error {
+  constructor (statusText: string, public status: number, public data: any) {
+    super(statusText);


we lost overall coverage because of these lines, can we add a simple test to this

effervescentia · Nov 15, 2017

+  }
+
+  get statusText() {
+    return this.message;


we lost overall coverage because of these lines, can we add a simple test to this

Oops forgot to commit it last time, done.

Jay Atkinson and others added 14 commits November 9, 2017 14:34

initial switch to fetch-ponyfill

d4e3c28

messing with fetchPonyfill

c1478ca

Check for error codes on request

4465d3c

Move throw and declaration onto same line

65d6f6b

Add timeout function to api-javascript

e8458d5

Make promise const

39272c1

Place fetch on api-javacsript object

28982be

Add headers to request, return body from failed request

94d821c

Fix tests, comment out tests for query params

6495e8d

Add better error message on timeout, add missing tests

34aeb43

Add more tests

5f42fd8

One more missing test

c6a2417

Remove queryParams

a45cea8

Remove tests that expected query params

7e599ba

Remove axios from package.json

64afd4d

Filip-F requested a review from groupby-ops November 15, 2017 17:50

Remove axios again

95d1df4

jatkinson10 previously approved these changes Nov 15, 2017

View reviewed changes

Filip-F added 2 commits November 15, 2017 13:07

Revert "Remove queryParams"

47c402f

This reverts commit a45cea8.

Revert "Remove tests that expected query params"

272bb4e

This reverts commit 7e599ba.

Filip-F dismissed jatkinson10’s stale review via 272bb4e November 15, 2017 18:08

zfcb suggested changes Nov 15, 2017

View reviewed changes

Filip-F added 3 commits November 15, 2017 13:11

Add query string parsing

c4642f7

Add query param stringifying

01dd774

Move some objects onto one line

93abfb4

zfcb previously approved these changes Nov 15, 2017

View reviewed changes

effervescentia suggested changes Nov 15, 2017

View reviewed changes

Move function out into broader scope

8c343d2

Filip-F dismissed zfcb’s stale review via 8c343d2 November 15, 2017 19:34

Filip-F added 2 commits November 15, 2017 14:51

Return specific object instead of just object literal

b68edd6

Add test that checks for bridge error

2f369de

Filip-F commented Nov 15, 2017

View reviewed changes

effervescentia approved these changes Nov 15, 2017

View reviewed changes

effervescentia suggested changes Nov 15, 2017

View reviewed changes

Filip-F added 3 commits November 15, 2017 15:16

Add test for status text

a350da3

Return promise directly

fde6976

Fix some missing coverage

acd83a8

effervescentia approved these changes Nov 15, 2017

View reviewed changes

effervescentia merged commit 2272c54 into develop Nov 15, 2017

effervescentia deleted the BBD-842 branch November 15, 2017 20:36

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

Filip-F commented Nov 15, 2017

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

jatkinson10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effervescentia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effervescentia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effervescentia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Nov 15, 2017 •

edited

Loading