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

Changed axios and axios fetch adapter usage into node-fetch #2736

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

dsawali
Copy link
Contributor

@dsawali dsawali commented Nov 23, 2023

closes #2461

  • Removed axios implementation in taquito-http-utils
  • Added node-fetch to match Axios' implementation in Taquito
  • Wrapped fetch in the new createRequest method
  • Added better error handling
  • Updated some integration tests to match new implementation
  • Updated axios reference in the sapling package to use fetch

Thank you for your contribution to Taquito.

Before submitting this PR, please make sure:

  • Your code builds cleanly without any errors or warnings
  • You have run the linter against the changes
  • You have added unit tests (if relevant/appropriate)
  • You have added integration tests (if relevant/appropriate)
  • All public methods or types have TypeDoc coverage with a complete description, and ideally an @example
  • You have added or updated corresponding documentation
  • If relevant, you have written a first draft summary describing the change for inclusion in Release Notes.

In this PR, please also make sure:

  • You have linked this PR to the issue by putting closes #TICKETNUMBER in the description box (when applicable)
  • You have added a concise description on your changes

Release Note Draft Snippet

If relevant, please write a summary of your change that will be suitable for
inclusion in the Release Notes for the next Taquito release.

Copy link

netlify bot commented Nov 23, 2023

Deploy Preview for taquito-test-dapp ready!

Name Link
🔨 Latest commit efdf457
🔍 Latest deploy log https://app.netlify.com/sites/taquito-test-dapp/deploys/65668a3f34ab270008af5786
😎 Deploy Preview https://deploy-preview-2736--taquito-test-dapp.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Nov 23, 2023

New packages have been deployed to the preview repository at https://npm.preview.tezostaquito.io/.

Published packages:

npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/
npm i @taquito/[email protected] --registry https://npm.preview.tezostaquito.io/

Copy link

github-actions bot commented Nov 23, 2023

A new deploy preview is available on Netlify at https://f449fb3--tezostaquito.netlify.app

@dsawali dsawali marked this pull request as ready for review November 24, 2023 00:17
@dsawali dsawali requested review from hui-an-yang and ac10n November 27, 2023 23:19
Copy link
Collaborator

@hui-an-yang hui-an-yang left a comment

Choose a reason for hiding this comment

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

lgtm


const isNode = typeof process !== 'undefined' && !!process?.versions?.node;
import fetch from 'node-fetch';
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering, in here, is this line (and the similar one in packages/taquito-sapling/fetch-sapling-params.js), specific to node? What about browser?

If we change this line so that it only imports if fetch is not already available?

From this answer:

let fetch = globalThis?.fetch;

if (!fetch && process?.versions?.node) {
  fetch = (await import('node-fetch')).default;
}

console.log(fetch);

But I guess something more relevant to us can conceptually be:

import fetch as node_fetch from 'node-fetch';
let fetch = globalThis?.fetch;
if (!fetch && process?.versions?.node) {
  fetch = node_fetch;
}

But I don't have enough context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason for that existing before is to use the axios-fetch-adapter when the runtime is a browser.

But since we've now updated to fetch completely, there would be no need to check what runtime Taquito is currently running on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, fetch is natively available on browsers, and node-fetch is there to make it available for node. So in browser we don't need to import fetch from node-fetch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I see what you mean. That makes sense, I hadn't consider that. I'll update it

@dsawali dsawali merged commit 882a565 into master Nov 29, 2023
@dsawali dsawali deleted the 2461-axios-to-fetch branch November 29, 2023 20:29
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.

Replace Axios with Fetch in Taquito
3 participants