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

Network: Replace superagent with fetch for HTTP requests #676

Merged
merged 5 commits into from
Oct 27, 2022
Merged

Conversation

jasonpaulos
Copy link
Contributor

This is a continuation of #661, opened by @PureBrent.

@jasonpaulos jasonpaulos changed the title Replace superagent with fetch for HTTP requests Network: Replace superagent with fetch for HTTP requests Oct 25, 2022
Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

src/client/urlTokenBaseHTTPClient.ts Show resolved Hide resolved
@jasonpaulos
Copy link
Contributor Author

Another benefit of this PR is that our browser bundle size will decrease, since cross-fetch appears to use less browser code than superagent.

Note that there is an opportunity to further decrease our browser bundle size, since most modern browsers have native fetch support, meaning cross-fetch's polyfill code is likely unnecessary.

Here are the numbers I got when locally compiling with webpack under different configurations:

  • With superagent (develop before this PR is merged): 473 KiB
  • With cross-fetch (this PR): 430 KiB
  • With no browser fetch library (this PR + additional modification): 422 KiB
    • I tested this by just commenting out the cross-fetch import in src/client/urlTokenBaseHTTPClient.ts and compiling with webpack again -- this is ok for producing browser code, but it would fail for any node versions which don't have native fetch support.

To properly pursue the final bullet point, we'd need a way of conditionally importing a fetch library in node, but not in the browser. Since that involves more complexity for only a marginal benefit, I've opted not to do that in this PR.

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

Successfully merging this pull request may close these issues.

3 participants