Register Faraday adapter

[philsturgeon]

Faraday has removed the Typhoeus adapter from master (lostisland/faraday#715), so it'll be gone in v0.14 (as yet unreleased). Your adapter is fantastic, but without this line it'll stop working as its no longer registered.

[philsturgeon]

I've added another commit to this (a9d1dc0), to remove the remove_method calls. If my understanding is right, you were essentially "extending" Faradays bundled adapter and so removing these was important. Now it's been removed you should not need to remove any methods.

I'm running the full faraday test suite against this adapter (lostisland/faraday#745) and it seems to work just fine when I comment out these lines.

[philsturgeon]

With this line removed typhoeus seems to continue to work just fine according to lostisland/faraday#745, and it seems to pass locally with v0.9 - v0.13. I cannot understand why it caused a problem, and I worry about removing it. Any ideas?

[philsturgeon]

Plans have changed on the Farday side, and outright removal looks like it will not happen in 0.14 to ensure typhoeus users are not forced to upgrade. lostisland/faraday#745 (comment)

[iMacTia]

Here is the reason for the exception:

class Test < Faraday::Adapter
  def test
    return self.class.method_defined?(:call)
  end
end

Test.new.test #=> true 

Apparently, method_defined? checks ancestors as well, while remove_method stops at the current class level 😞

[hanshasselberg]

@philsturgeon thanks a lot for your work! I don't know a lot about faraday and I would love to improve our adapter. right now the specs are failing. I would be happy to merge your PR once you let me know! ❤️

[philsturgeon]

This PR will no longer be necessary once @iMacTia has merged lostisland/faraday#748, it should keep both gems compatible with each other. We can ensure that with #583 though. :)

[iMacTia]

It would still be good to have these changes in so that we'll be able to get rid of the internal adapter once and for all in v1.0! @philsturgeon can you please add them to #583?
All we need is adding the registration at the end of the file (I hope Faraday won't complain if we register it twice!) and getting rid of the remove_methods calls if possible.

[philsturgeon]

Faraday does not seem to care about having something registered twice, which is nice.

[iMacTia]

Luck is on our side for once 😅

[philsturgeon]

I've removed the 2nd commit, this one literally just registers the adapter in case faraday hasn't done it already; future proofing this for the change ahead.

[philsturgeon]

sadly the test suite is failing due to this:

Failure/Error: res = ::Net::HTTP.get_response("localhost", '/__identify__', port)

Errno::EADDRNOTAVAIL:

  Cannot assign requested address - connect(2)

No idea whats happening there.

[PericlesTheo]

👋 @philsturgeon, I appreciate it's been a while. Is this PR still relevant and if it is, how can we get it merged?

[woahdae]

I'm attempting to upgrade all the gems in our app, and now running into method 'call' not defined in Faraday::Adapter::Typhoeus, which is strange since this seemed to be an issue 5 years ago. I don't grok the status of this, especially since the method_defined? checks the ancestor tree while remove_method is just for the current class, which here is currently being defined, so how did it ever work? self.superclass.remove_method(:call) works. So... did it ever work? I don't get it. Am I using something wrong?

[iMacTia]

@woahdae which combination of faraday and typhoeus are you currently using?
There's a chance this is still working fine up to faraday 1.x, but it doesn't work with faraday 2.x unless we merge this fix.

The reason why this works in 1.x is that we kept this file around to give time to merge this PR.

However that file was ultimately removed in Faraday 2.0, potentially causing the issue again

[woahdae]

I got it to work via the faraday-typhoeus gem and omitting the built-in adapter:

require 'faraday'
require 'faraday/typhoeus'

At a quick glance it appears to be the built in adapter without the remove_method calls, but I didn't really stare at it. I'm still not sure what the most-official way is, since both faraday (the team, not the project) and typhoeus offer an adapter, so maybe the answer for typhoeus is to change the docs? If you want to be out of the adapter business.

[iMacTia]

Good catch @woahdae, I completely forgot about the new adapter!
Just to clarify, faraday-typhoeus is not maintained by the faraday core team, it was created by a community member (thank you @dleavitt 🙏!) and it seems fully compatible with Faraday 2.x 🙌

I guess your point on updating the Typhoeus documentation would make sense: we should clarify that the built-in adapter will work up until faraday 1.x, but if you're running faraday 2.x then you'll need to use the external adapter above☝️.

@PericlesTheo I'd love to hear your thoughts on this, but here is my suggestion:

• Close this PR as the changes we made on faraday means that you can still use this adapter with 1.x
• Open a new PR to update the documentation and explain that the adapter is only compatible up to faraday 1.x, while the external adapter from @dleavitt will need to be used with faraday 2.x
• [optional] Cherry on the cake: add a line that raises an error or issues a warning if the built-in faraday adapter is required in a project where Faraday 2.0 or newer is present

[dleavitt]

@iMacTia @woahdae Let me know if there's anything I can help with! Happy to open a documentation PR on this repo or elsewhere outlining the situation.

[iMacTia]

Thank you @dleavitt 🙏, I'd suggest waiting to hear from the typhoeus core team first and hear what their preference is 👍

[hanshasselberg]

@iMacTia @dleavitt thank you both for your work! Your suggestions seem great to me and we would be super happy if you could provide these PRs. 🙇