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 library deprecated #12

Open
wmurphyrd opened this issue Dec 2, 2020 · 10 comments
Open

Request library deprecated #12

wmurphyrd opened this issue Dec 2, 2020 · 10 comments

Comments

@wmurphyrd
Copy link
Member

wmurphyrd commented Dec 2, 2020

Find a new library for sending http requests

@wmurphyrd
Copy link
Member Author

this looks promising: https://github.com/blocktrail/superagent-http-signature

However it's forked from an hold version of http-signature so it will almost certainly need the patches we're currently applying to http-signature added to it

@gregid
Copy link

gregid commented Apr 18, 2021

I have played a bit with request-compose over the weekend and I must say I really like it for it's flexibility/customization.

I've made draft version of request with this library and http-signature-header for signature (as discussed in #40):
https://gist.github.com/gregid/e49f041ac5e8d857edec6cc6d2f0b19b

Generating signatures is done but I still have problems with verification of the generated signature so I may have made some mistakes as this is my first time dealing with HTTP Signatures.

But overall I do think request-compose is a good fit for replacement especially if considering other enhancements, like json-ld signatures in the future.

@wmurphyrd
Copy link
Member Author

@gregid this looks great! Can you start a pull request and we'll work out the kinks together?

@gregid
Copy link

gregid commented Apr 18, 2021

Will do - I didn't want to impose this without your interest.

@ThisIsMissEm
Copy link

@wmurphyrd I'd perhaps be inclined to look towards Node 18's fetch support, which is undici under the hood (requires at least node 16)

Then you'd just need the ability to take a Request / Response object and do http-signatures on it, potentially even allowing setting fetch on the apex instance, this would allow "bring your own fetch implementation", which would also help in unit testing the code.

@wmurphyrd
Copy link
Member Author

@ThisIsMissEm oh now this is interesting. I'd glanced at node's fetch when it was announced, but didn't think it would work here because of the need to manipulate an existing Request object. I just looked again based on your prompting and realized that all these years using fetch in the browser, I'd never noticed it's possible to construct a Request first and then pass it to fetch.

I also just created an interface mock for http-signature to use in the unit tests, so I'm pretty certain I could coerce http-signature to work with a Request

https://developer.mozilla.org/en-US/docs/Web/API/fetch

@wmurphyrd
Copy link
Member Author

@ThisIsMissEm well, converting the code to fetch was simple, but unfortunately it does not play well with the unit tests:

  • nock is not able to intercept outgoing fetch (Undici support nock/nock#2183)
  • If I set the Host header (required for http signature and not set automatically by node), I get an an SSL error on the outgoing side of the request, Host: localhost. is not in the cert's altnames: DNS:landingpage.com (no clue where this landingpage.com cert is coming from)

So, no idea if it actually works, and refactoring request mocking for the test suites would be a massive chore. Looks like native fetch is out at least for now

@ThisIsMissEm
Copy link

Ah, yeah, we've seen similar issue with the question of "how to mock fetch in node.js 18" in the @inrupt Solid SDKs, the approach we're taking is to shim it, so we'll have a module that either directs to fetch or to the mock.

It's still early days for us, but we're going to have to rewrite all our unit tests to this new setup away from module mocking cross-fetch.

Once we've something working, I kinda want to take it to WinterCG to propose a standardised way to mock fetch.

@wmurphyrd
Copy link
Member Author

wmurphyrd commented Feb 2, 2023

Ah, yeah, we've seen similar issue with the question of "how to mock fetch in node.js 18" in the @inrupt Solid SDKs, the approach we're taking is to shim it, so we'll have a module that either directs to fetch or to the mock.

It's still early days for us, but we're going to have to rewrite all our unit tests to this new setup away from module mocking cross-fetch.

Once we've something working, I kinda want to take it to WinterCG to propose a standardised way to mock fetch.

Interestingly, undici, which powers node's native fetch, does have built-in mocking https://undici.nodejs.org/#/docs/api/MockAgent

I can't see any way to use this with fetch, but I think we could just use undici directly instead of the fetch wrapper

oh and here's a useful reference from another project making the same conversion from nock to MockAgent: vansergen/poloniex-node-api#91

@ThisIsMissEm
Copy link

Oh? Interesting! I thought it worked for that. Either way, we'll hopefully have a new module out soon that'll help

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

No branches or pull requests

3 participants