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

new version of http that uses fetch #302

Closed
wants to merge 5 commits into from

Conversation

mtuchi
Copy link
Collaborator

@mtuchi mtuchi commented Jul 10, 2023

Summary

Migrate from using axios to fetch in common.http

  1. we have request() in common/util.js that uses fetch() from undici
  2. http common to have all http function (get(), post(), head(), patch(), etc) and all these function expandRefences in their arguments.
  3. All other adaptors that need access http functions they should use http common function and not implement their own version of get() or post() etc
  4. If other adaptors can not use http common function then extend it from common (For example in other adaptors we have create() which is basically a http common post() so it make sense to extend post from http common)

Details

  • buildRequest
  • buildUrl
  • request
  • tests

Issues

Ref #283

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • If this is a new adaptor, has the migration tool been run and the
    migration guide followed?
  • Are there any unit tests? Should there be?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.

@mtuchi mtuchi linked an issue Jul 10, 2023 that may be closed by this pull request
@josephjclark
Copy link
Collaborator

Hi @mtuchi - can we put the breaks on this until I've had a chance to really dig into it?

I keep making snap decisions on things without thinking them through - I think maybe we all do - and basically before taking this any further I want to make sure we have a really tight spec about what we want to do, and why.

As part of the issue I'm going to review the new request pattern laid down in template. I've let it sail over me a little bit and I just want to make sure it's right.

@mtuchi mtuchi mentioned this pull request Aug 4, 2023
4 tasks
@mtuchi mtuchi closed this Aug 10, 2023
@mtuchi mtuchi deleted the 283-new-version-of-http-that-uses-fetch branch August 10, 2023 07:13
josephjclark added a commit that referenced this pull request Jun 11, 2024
…confirm-2.0.6

build(deps): bump @inquirer/confirm from 0.0.28-alpha.0 to 2.0.6
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.

new version of http that uses fetch
2 participants