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

Request and Request-Promise packages are deprecated [4 XMR] #95

Closed
CryptoGrampy opened this issue Jun 24, 2022 · 24 comments · Fixed by #230
Closed

Request and Request-Promise packages are deprecated [4 XMR] #95

CryptoGrampy opened this issue Jun 24, 2022 · 24 comments · Fixed by #230
Labels
💰bounty There is a bounty on this issue help wanted Extra attention is needed

Comments

@CryptoGrampy
Copy link

Just noticed these packages are deprecated. Not sure if this is possible, but there is a native 'fetch' API that's NodeJS 18+ compatible: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API and https://blog.logrocket.com/fetch-api-node-js/ that might be nice to use as it's native for browser/Node and doesn't require any extra packages (and allows us to remove packages from monero-javascript!) , but Axios is also a very good, popular and full-featured request library.

@trasherdk
Copy link
Contributor

  1. Firefox native fetch still doesn't support digest auth, required by Monero.
  2. Moving beyond Node.js LTS version seems like a really bad idea.
  3. Something like undici is probably a better choice, as it's the thingy implemented in
    Node.js 16.5+ (experimental). It's not for the browser though, so Axios seems like a decent alternative.

@CryptoGrampy
Copy link
Author

Definitely agree on the LTS subject, just important to acknowledge that this will be a built in feature down the road. I'm a big fan of the axios lib, personally.

@trasherdk
Copy link
Contributor

@CryptoGrampy did you try debug/tracing what's going on below the surface, during requests?

Getting rid of browser fetch was not a simple task, if memory serves me. Other than that, I agree. Getting rid of those deprecation warnings would be nice.

@47Key
Copy link

47Key commented Jul 11, 2022

@trasherdk @CryptoGrampy I can try and take care of this issue if needed, with either axios or node-fetch. Personally I've used node fetch just for simplicity and standardizing the fetch calls. But can use axios if needed.

Let me know what progress you've made on this and I will try to dig farther from the point you left off.

@CryptoGrampy
Copy link
Author

@trasherdk - I haven't dug into this at all beyond noticing that the library being used isn't supported anymore.
@47Key - Monero-Javascript is used from both nodejs as well as browser context, so any library used should support both; for that reason, Axios is probably the best modern request library to go with until the fetch API for node matures.

@47Key
Copy link

47Key commented Jul 16, 2022

@CryptoGrampy On it, any unit tests or guidelines I should adhere too before finishing?

@trasherdk
Copy link
Contributor

@CryptoGrampy The problem is Firefox native fetch() have a broken digest authentication. Chrome was OK.
And under the hood, the request logic is very complex. I have single-stepped this thing many times and Oh boy, it'll make your head spin.

@47Key Looking forward to see what you come up with 😃

@woodser
Copy link
Owner

woodser commented Nov 21, 2023

If anyone wants to help with this issue, HttpClient.ts just needs to be updated to support the dependencies.

@mainnet-pat
Copy link
Contributor

stumbled upon this. request and request-promise do not work for me in quasar.js with vite. polyfilling this only helps for running in dev mode, production build is messed up.

@mainnet-pat
Copy link
Contributor

mainnet-pat commented Aug 19, 2024

found a workaround to define missing symbols which these packages expect

      rawDefine: {
        'process.version': JSON.stringify(process.version),
        'require.cache': JSON.stringify({}),
      },

@woodser
Copy link
Owner

woodser commented Aug 19, 2024

@mainnet-pat If this fixes issues running in Vite, you could be interested in the bounty to make the Vite sample app work: #229

@mainnet-pat
Copy link
Contributor

wasn't this task taken by someone else?

@mainnet-pat
Copy link
Contributor

what about a bounty for this issue? I am running in an issue with require('bluebird') now. basically what I have posted fixes the prod build but breaks the dev env

@woodser
Copy link
Owner

woodser commented Aug 19, 2024

wasn't this task taken by someone else?

The bounty is open to the first working/merged solution. I've commented on the issue to clarify.

@mainnet-pat
Copy link
Contributor

what about a bounty for this issue? I am running in an issue with require('bluebird') now. basically what I have posted fixes the prod build but breaks the dev env

if you missed my message above ^

I can think of replacing the request with plain fetch, if firefox issue with digest auth will be reconfirmed (after these 2 years passed) I can replace the native fetch with undici fetch

@woodser woodser changed the title Request and Request-Promise packages are deprecated Request and Request-Promise packages are deprecated [2 XMR] Aug 19, 2024
@woodser woodser added the 💰bounty There is a bounty on this issue label Aug 19, 2024
@woodser
Copy link
Owner

woodser commented Aug 19, 2024

Ok I've added a 2 XMR bounty.

Note that any solution will need to be tested in Node.js, Firefox, Brave/Chrome, and with the sample web applications: webpack, Next.js, and Vite.

Copy link

There is a bounty on this issue. The amount is in the title. The reward will be awarded to the first person or group of people who resolve this issue.

If you are starting to work on this bounty, please write a comment so that we can assign the issue to you. We expect contributors to provide a PR in a reasonable timeframe or, in case of an extensive work, updates on their progress. We will unassign the issue if we feel the assignee is not responsive or has abandoned the task.

Read the full conditions and details of the bounty system.

@mainnet-pat
Copy link
Contributor

Ok I've added a 2 XMR bounty.

Note that any solution will need to be tested in Node.js, Firefox, Brave/Chrome, and with the sample web applications: webpack, Next.js, and Vite.

Well, the task does not seem that primitive and testing will certainly take time. What about 4 XMR?

@woodser woodser changed the title Request and Request-Promise packages are deprecated [2 XMR] Request and Request-Promise packages are deprecated [4 XMR] Aug 19, 2024
@woodser
Copy link
Owner

woodser commented Aug 19, 2024

Ok I've added a 2 XMR bounty.
Note that any solution will need to be tested in Node.js, Firefox, Brave/Chrome, and with the sample web applications: webpack, Next.js, and Vite.

Well, the task does not seem that primitive and testing will certainly take time. What about 4 XMR?

Ok I've bumped it to 4 XMR to update the request and request promise dependencies with testing. :)

@mainnet-pat
Copy link
Contributor

Cool, I am on it

@mainnet-pat
Copy link
Contributor

mainnet-pat commented Aug 19, 2024

Leaving the notes on current research:

Firefox native fetch still doesn't support digest auth, required by Monero.

The whatwg group made the authorization header removal part of fetch design, this will not ever change for firefox. So fetch is out of question.

Undici is node-only so we won't use it for browser.

Axios uses xhr wrapped in promises so will work everywhere. Going with this option.

@Myestery
Copy link

I'll start working on this too

@woodser
Copy link
Owner

woodser commented Aug 22, 2024

The deprecated request libraries are now replaced with Axios.

Amazing work @mainnet-pat. Let's get you paid for this and #229. Please post or DM an address. :)

@mainnet-pat
Copy link
Contributor

Thanks! Sent you an email on prton mail

borgvin pushed a commit to borgvin/haven-web-core that referenced this issue Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💰bounty There is a bounty on this issue help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants