Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(fetchOEmbedData): Handle non-ok responses #104

Merged
merged 2 commits into from
Mar 19, 2020

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Mar 18, 2020

What:

Check OEmbed response status and throw if it's not ok (200-299 status range)

Why:

Because plugin currently doesn't handle case of non-ok response and have potential to throw very confusing errors that are hard to track down to root cause. Having explicit handling allow us to throw actionable errors (by pointing which links are problematic)

How:

Adding check for response.ok and throwing if it's falsy.

The change also added need to do some changes to existing tests, where I changed mocks to use actual Response object instead of fully mocked object (instead of adding ok and status there).

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

Closes #99
Closes #103

@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #104 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #104   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          179       182    +3     
  Branches        16        17    +1     
=========================================
+ Hits           179       182    +3     
Impacted Files Coverage Δ
src/transformers/utils/index.js 100.00% <100.00%> (ø)

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 da399e4...6909e24. Read the comment docs.

@pieh pieh force-pushed the non-200-twitter branch from dad4868 to 6909e24 Compare March 19, 2020 20:35
@pieh
Copy link
Contributor Author

pieh commented Mar 19, 2020

I rebased and dropped one commit ( dad4868 ) that was adding twitter specific error, because it's no longer needed after #102 was merged (that PR does log problematic URL, and all that my Twitter specific changes did was adding URL to error message)

@MichaelDeBoey MichaelDeBoey changed the title fix: handle non-ok responses feat(fetchOEmbedData): Handle non-ok responses Mar 19, 2020
Copy link
Owner

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Looks really nice, thanks @pieh! 👊

@MichaelDeBoey MichaelDeBoey merged commit 337a1af into MichaelDeBoey:master Mar 19, 2020
@MichaelDeBoey
Copy link
Owner

🎉 This PR is included in version 1.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants