Skip to content

Refactor Gist Web to use Fetch API #82

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 3 commits into from
Apr 2, 2025

Conversation

stephen-pope-customer-io
Copy link
Contributor

To replace axios

Part of INAPP-12465
Part of INAPP-13292

To replace axios

Part of INAPP-12465
Part of INAPP-13292
@stephen-pope-customer-io stephen-pope-customer-io requested a review from a team as a code owner March 28, 2025 19:22
@stephen-pope-customer-io stephen-pope-customer-io requested review from Copilot and removed request for a team March 28, 2025 19:43
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the network service to replace axios with the Fetch API, streamlining network requests for the Gist Web application.

  • Replaces axios with the Fetch API and AbortController for timeouts.
  • Reorganizes baseURL selection and header construction based on API version and user token.
  • Implements a POST helper function with JSON serialization.
Comments suppressed due to low confidence (1)

src/services/network.js:49

  • [nitpick] Consider wrapping the thrown error in an instance of Error (e.g., new Error(...)) to preserve the stack trace and ensure consistent error handling.
throw {

Copy link

@mvanderlinde mvanderlinde left a comment

Choose a reason for hiding this comment

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

Just a couple opportunities for simplification, but nothing blocking from my perspective. 👌

});

if (response.status < 200 || response.status >= 400) {

Choose a reason for hiding this comment

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

💡 This could be simplified by using the ok Response attribute.

Suggested change
if (response.status < 200 || response.status >= 400) {
if (!response.ok) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its because we also need to handle 304s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bernard is correct, we return a 304 Not Modified when users hit the cache for messages instead of going to Firestore.

Part of INAPP-13292
@stephen-pope-customer-io stephen-pope-customer-io merged commit e33ae2e into develop Apr 2, 2025
1 check passed
@stephen-pope-customer-io stephen-pope-customer-io deleted the inapp-13292-axios branch April 2, 2025 17:55
stephen-pope-customer-io added a commit to customerio/cdp-analytics-js that referenced this pull request Apr 2, 2025
Part of INAPP-13292

Related PR: customerio/gist-web#82
stephen-pope-customer-io added a commit to customerio/cdp-analytics-js that referenced this pull request Apr 2, 2025
Part of INAPP-13292

Related PR: customerio/gist-web#82
@jefago
Copy link

jefago commented Apr 8, 2025

does this mean that the dependency on axios can be removed? it doesn't seem to be used anymore, and the referenced version has a CVE vulnerability axios/axios#6463

@stephen-pope-customer-io
Copy link
Contributor Author

does this mean that the dependency on axios can be removed? it doesn't seem to be used anymore, and the referenced version has a CVE vulnerability axios/axios#6463

Yes, I have opted to refactor using fetch api instead of updating axios.

@jefago
Copy link

jefago commented Apr 8, 2025

Thank you! Will there be a new release without the axios dependency (since it's still referenced in the package.json right now)?

@stephen-pope-customer-io
Copy link
Contributor Author

Ah, yes, of course! I will remove the dependency and release again.

stephen-pope-customer-io added a commit that referenced this pull request Apr 8, 2025
Leftovers from PR #82

Part of INAPP-13292
@jefago
Copy link

jefago commented Apr 8, 2025

Thank you!

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

Successfully merging this pull request may close these issues.

5 participants