Skip to content
This repository has been archived by the owner on Jan 16, 2022. It is now read-only.

fix(api): correctly handle responses with missing content-type header #112

Merged
merged 2 commits into from
Aug 10, 2019

Conversation

ThisIsMissEm
Copy link

Also prevents non .tgz requests from being handled as tgz requests — the previous if condition was incorrect

Type: bug

The following has been addressed in the PR:

  • There is a related issue? No
  • Unit or Functional tests are included in the PR? Yes

Description:

Previously if for whatever reason an API request resulted in a 400 error without a content-type header, then this code would fail, as it was trying to call includes on null, resulting in what looked like a plugin/configuration error. Additionally, I noticed in testing that the check for handling .tgz files was incorrect, and it should've been request.url.includes('.tgz') === true rather than !== null, though I'm not sure the best way to test that.

Also prevents non .tgz requests from being handled as tgz requests — the previous if condition was incorrect
@codecov-io
Copy link

Codecov Report

Merging #112 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
+ Coverage   71.16%   71.28%   +0.11%     
==========================================
  Files          88       88              
  Lines         874      874              
  Branches      156      156              
==========================================
+ Hits          622      623       +1     
+ Misses        240      239       -1     
  Partials       12       12
Impacted Files Coverage Δ
src/utils/api.ts 32% <100%> (+4%) ⬆️

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 62b6edc...cfb0caf. Read the comment docs.

@juanpicado juanpicado self-requested a review August 10, 2019 20:08
return Promise.all([response.ok, response.text()]);
}

// unfortunatelly on download files there is no header available
if (response.url && response.url.endsWith('.tgz') !== null) {
if (response.url && response.url.endsWith('.tgz') === true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


const handled = await handleResponseType(response);

// Should this actually return [false, null] ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According the fetch https://developer.mozilla.org/en-US/docs/Web/API/Response#Properties docs the Response . The first part of the array should be boolean and thus as second argument a the promise resolved according the type blob etc etc ...

Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this and 👏 for the unit test !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants