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

fix: replace ofetch native fetch #147

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

alvarosabu
Copy link
Contributor

  • Removed the ofetch dependency from multiple files and replaced it with a customFetch utility for better error handling and flexibility.
  • Updated API calls in login, pull languages, and user actions to use the new fetch method.
  • Introduced a new ResponseError interface for improved error management.
  • Added a getStoryblokUrl utility function to centralize URL construction based on region.
  • Enhanced error handling in API error management to accommodate the new FetchError class.

How to test this PR

Unit tests remained the same. They are all passing

Other information

- Removed the ofetch dependency from multiple files and replaced it with a customFetch utility for better error handling and flexibility.
- Updated API calls in login, pull languages, and user actions to use the new fetch method.
- Introduced a new ResponseError interface for improved error management.
- Added a getStoryblokUrl utility function to centralize URL construction based on region.
- Enhanced error handling in API error management to accommodate the new FetchError class.
@alvarosabu alvarosabu requested a review from edodusi January 10, 2025 11:16
@alvarosabu alvarosabu self-assigned this Jan 10, 2025
@alvarosabu alvarosabu added p3-significant [Priority] Moderate issues, major enhancements feature [Issue] New feature or request labels Jan 10, 2025
try {
const headers = {
'Content-Type': 'application/json',
...options.headers,
Copy link
Contributor

Choose a reason for hiding this comment

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

since it's needed in every call, why don't you put the Authorization: $token here too? It's overridable, but it spares you from manually adding it to every single customFetch call

fetchOptions.body = options.body
}
catch {
fetchOptions.body = JSON.stringify(options.body)
Copy link
Contributor

Choose a reason for hiding this comment

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

why you try to stringify a string here?

data = await response.json()
}
catch {
// Ignore JSON parse errors
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure we can ignore all .json() errors here?

})
}

// Handle empty responses
Copy link
Contributor

Choose a reason for hiding this comment

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

is this comment correct?

if (contentType?.includes('application/json')) {
return await response.json()
}
return await response.text() as T
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why if the response is json then you don't type with the generics but otherwise you return as plain text but typed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature [Issue] New feature or request p3-significant [Priority] Moderate issues, major enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants