Skip to content

Continue Axios Migration #150

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
Feb 19, 2024

Conversation

benflap
Copy link
Contributor

@benflap benflap commented Nov 23, 2023

Fixes: #143

Axios is not a drop-in replacement for Popsicle. Here are a few of the API differences:

  • Request config:
    • Axios uses data instead of body.
    • Axios.get() will override the the method option.
    • There is no transport option.
  • Response schema:
    • Headers are turned into an object with lower cased names.
    • Response data is atomically parsed and can be accessed with res.data.
    • The response can't be stringified using JSON.stringify() because of res.request.

Breaking Change

Given the transport issue, this PR does introduce a breaking change. Users that receive a PDF will have to change their code from this:

oauthClient.makeApiCall({
  url: `${url}v3/company/${companyID}/invoice/${invoiceNumber}/pdf?minorversion=59`,
  headers:{'Content-Type': 'application/pdf','Accept':'application/pdf'},
  transport: popsicle.createTransport({type: 'buffer'})
})

To this:

oauthClient.makeApiCall({
  url: `${url}v3/company/${companyID}/invoice/${invoiceNumber}/pdf?minorversion=59`,
  headers:{'Content-Type': 'application/pdf','Accept':'application/pdf'},
  responseType: 'arraybuffer'
})

In my opinion, switching to Axios is still worth it.

Testing

The only things I use this package for are creating redirects and creating tokens, so those are the only things I've tested. Other users should test out the rest of the package before this PR is merged.

@rajeshgupta723
Copy link
Collaborator

rajeshgupta723 commented Feb 19, 2024

Thanks for raising this PR for migrating to Axios from Popsicle. Few comments,

  • I tested the sample client with your branch code and it seems to be working fine. However, as it introduces some breaking changes, the overall impact needs to be assessed and tested.
  • In terms of popularity (# of downloads), agree that Axios popularity is pretty high (as per https://npmtrends.com/axios-vs-popsicle-vs-request ).
  • Axios release 1.x.x with ~500 outstanding issues (as per https://www.npmjs.com/package/axios) raises some questions on the stability of Axios ver 1.x.
  • Overall, we will continue to watch on this migration/merge as it requires further analysis.

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.

2 participants