Skip to content

fix: replace axios with fetch #288

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

Merged
merged 1 commit into from
May 15, 2024
Merged

Conversation

jsalaber
Copy link
Contributor

Changes

  • replace axios with fetch after upgrading broke the extension

@jsalaber jsalaber requested a review from a team May 15, 2024 20:10
@jsalaber
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @jsalaber and the rest of your teammates on Graphite Graphite

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @jsalaber - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

  • Hardcoded secret detected in Authorization header. (link)
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🔴 Security: 1 blocking issue
  • 🟡 Testing: 1 issue found
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

const writeStream = tar.x({ cwd: OUTPUT_DIR })
const response = await axios.get(sourceUrl, {
responseType: 'stream',
const response = await fetch(sourceUrl, {
Copy link

Choose a reason for hiding this comment

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

suggestion: Validate the response status code from fetch to ensure the stream is valid.

Checking the response status before processing the stream can prevent issues related to unexpected response statuses, such as 404 or 500 errors.

@jsalaber jsalaber force-pushed the COR-2754-replace-axios-with-fetch branch from 38238d0 to 357827c Compare May 15, 2024 20:17
@jsalaber jsalaber force-pushed the COR-2754-replace-axios-with-fetch branch from 357827c to fa2e903 Compare May 15, 2024 20:43
@jsalaber jsalaber merged commit 1372870 into main May 15, 2024
4 checks passed
@jsalaber jsalaber deleted the COR-2754-replace-axios-with-fetch branch May 15, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants