Add an explicit fallback to net_http to support both faraday 1.x and 2.x implementations #229
Conversation
…estriction except under jruby
…2.x implementations
rsolr.gemspec
Outdated
| s.required_ruby_version = '>= 1.9.3' | ||
|
|
||
|
|
||
| s.requirements << 'Apache Solr' | ||
|
|
||
| s.add_dependency 'builder', '>= 2.1.2' | ||
| s.add_dependency 'faraday', '>= 0.9.0' |
There was a problem hiding this comment.
FWIW it looks like the faraday dependency will never be able to <1 due to faraday-net_http's requirements. Should this be bumped up to >= 1.0 to not give false hopes?
There was a problem hiding this comment.
I don't think that's true -- faraday-net_http is only a dependency of faraday depending on what version of faraday you are using. If you are using faraday < 1, you don't have faraday-net_http at all. I think there's no reason for rsolr to stop letting users use a version of faraday they may be currently using without problems -- especially on a minor version release.
There was a problem hiding this comment.
@jrochkind faraday-net_http is being required here on the next line. From what I can see in rubygems, faraday-net_http has only every supported faraday 1.0 and higher. By adding a hard dependency on faraday-net_http here then rsolr would essentially be requiring faraday 1.0+.
There was a problem hiding this comment.
I think maybe we could have done this differently to preserve backwards compat with all previously supported faraday versions -- we didn't need to explicitly depend on faraday-net_http, and didn't need to do the require if Faraday was less than 2, which we could have checked for. Maybe. I could be wrong.
I would have appreciated y'all giving this more time for discussion and experimentation, when we are in the middle of a discussion, and I raised concerns-- what's the rush/urgency to commit this within an hour of PR'ing it?
| @@ -319,7 +320,7 @@ def connection | |||
| conn.request :retry, max: options[:retry_after_limit], interval: 0.05, | |||
| interval_randomness: 0.5, backoff_factor: 2, | |||
| exceptions: ['Faraday::Error', 'Timeout::Error'] if options[:retry_503] | |||
| conn.adapter options[:adapter] || Faraday.default_adapter | |||
| conn.adapter options[:adapter] || Faraday.default_adapter || :net_http | |||
There was a problem hiding this comment.
Does Faraday.default_adapter return nil in 2.x? Why do they even have that method?
There was a problem hiding this comment.
According to the upgrade guide, downstream consumers can set default_adapter.
|
Note this PR changed our dependency to require faraday 1.0 instead of 0.9.0. Personally I would argue that should not be done in a minor release. But no other committers think this/are concerned about it? I can get overruled. Would have liked to sit on this a bit longer for discussion -- is there any reason we need to be merging things effecting backwards compat the same hour they are submitted, when they are receiving discussion? |
I didn't see this as effecting backward compatibility. It looked to me like all points raised were addressed. |
I think this is a bug fix, because we previously said any version of faraday >= 0.9.0 would work, but if you use faraday 2, it breaks. |
|
Ah, I understand the urgency now, I still wish we had tried to find a way to not change the lower bound of faraday we support. But I'm also seeing faraday isn't doing what I thought it was going to do here, so the path I was thinking doesn't exactly work. I don't myself use faraday 0.9.0, this won't be a problem for me. Just in this thing that is very "legacy" and used outside our community in who knows what ways, I think we have a responsibility to be super careful with backwards compat. But hopefully it will be fine for anyone depending on it. I think removing support for 0.9.0 is technically a backwards incompat, if faraday 1.0 could have any backwards incompats with 0.9.0, which it could. I wonder if we should set an upper bound of |
Yeah, that sounds like a good idea. |
|
I would +1 making this a major release since as far as I can tell most Samvera gems aren't even on faraday 1 yet. |
|
Hey! I've just released Faraday 2.0.1 which brings back Thanks for using Faraday btw 🙏! |
|
We may be able to do undo this becuase of the faraday correction @iMacTia mentions?
lostisland/faraday#1358 (reply in thread) We're still going to have a problem with the I think releasing an Rsolr 3.0 is really a last resort, because it's so hard to get everything to upgrade RSolr versions, just to get everything to update their gemspec dependencies to allow it even if they don't need any changes to work with it... there are things still not using 2.x yet. Ideally if we were going to do a 3.0, it would be for more than faraday bookkeeping, to justify the pain of changing so many gemspecs and updating so many gems. |
No description provided.