Remove EM-HTTP SSL patch duplication

[kylekeesling]

Description

This PR is meant to address the duplication of an SSL patch call. Fixes #1201

Please let let me know if there are any changes/updates necessary!

[olleolleolle]

And, thanks for contributing a PR!

[kylekeesling]

Happy to help! Thanks for being a maintainer. This library is the backbone of so many gems/projects that I use, I figure the least I can do is submit a little patch 😉

[olleolleolle]

You can learn a bit more about the ins and outs of begin-rescue-else-ensure-end in Jonan's article here:
https://blog.newrelic.com/engineering/weird-ruby-2-rescue-interrupt-ensure/

And, I think you can run the RuboCop linting locally, too.

Here's a not-complete example of the structure I was thinking of when I said "body of the else" (an imprecise term, sorry about that!)

begin
  # ...
rescue
  # ...
else
  if something
     # do that require
  end
end

[kylekeesling]

Seems there is an error loading the version from `EventMachine::HttpRequest::VERSION'

The version looks to be in this spot there when looking at the gem though:
https://github.com/igrigorik/em-http-request/blob/master/lib/em-http/version.rb

Any ideas?

[olleolleolle]

Perhaps we have yet to actually load the dependency.

In this adapter, we have the call dependency "em-http".

That method can also be used like this:

dependency do
   require 'em-http'
   # And: Do other things, like the thing you want to do
end

Here's the source code for `dependency`:
https://github.com/lostisland/faraday/blob/01c25e880c94ef65d488019da44c39eb7823772c/lib/faraday/dependency_loader.rb#L12

Perhaps this inspires other ways to get this working?

[kylekeesling]

I'm a bit out of my element on this (haven't done much work on gems) but as I was digging around inside the 'em-http-request' gem, it doesn't seem as though version.rb is included anywhere, which in my mind would indicate that it would then not be accessible when we require em-http

When I change the dependency call to a block, and explicity include the version file, I can get the sepcs to pass:

dependency do
  require 'em-http'
  require 'em-http/version'
end 

If that's the case, would the long-term fix actually be to include the version file in the em-http-request bootstrap file? IE appending this to it: require 'em-http/version'

https://github.com/igrigorik/em-http-request/blob/master/lib/em-http.rb

If so I'm happy to open a PR on that library as well to get it fixed.

[ayanko]

If so I'm happy to open a PR on that library as well to get it fixed.

No. That's not issue in library. Library itself is NOT responsible to define version.
Originally version is defined in gemspec file. So actually you should get it from gemspec API.
But thus it is long way there is kind of convention to have separated small file called version.rb
So that it reused in gemspec and provides simple version access for developers.

[kylekeesling]

I've updated the conditional to require the version files if we get to that step. All tests seem to be passing.

[kylekeesling]

Ok I think we might have it this time - thanks for your patience as I work through this!

[olleolleolle]

Cool, now we reached the place where the whole patch logic (the whole test for "do we need it?" and the patch file require) needs to attempt to move into the dependency block - a time and place where we know we have access to the loaded em_http/version.rb file.

Faraday must avoid non-runtime dependencies on its optional gems, which leads to these shenanigans.

You're well on the way!

[kylekeesling]

Thanks @olleolleolle - I just pushed an update that moves that logic into the dependency block and only requires the version file if we get to that step

[olleolleolle]

@kylekeesling Awesome work! Let's get this merged!

[ayanko]

Release?