quickbooks-ruby icon indicating copy to clipboard operation
quickbooks-ruby copied to clipboard

[WIP]: Upgrade to Faraday v2

Open Zajn opened this issue 2 years ago • 4 comments

Removes the explicit dependency on Faraday < 2.0.

This removes the custom gzip middleware in favor of a gem which seems to have approval from the Faraday maintainers, and also adds the middleware gem which was extracted from Faraday during the v1 -> v2 transition.

Marked as WIP because I don't know what else needs to be tested, if anything, and would love some feedback on other areas which might need to be touched.

Zajn avatar Jun 01 '22 22:06 Zajn

It does look like the Faraday upgrade will require at least Ruby 2.6. CI failed for 2.5.

Zajn avatar Jun 01 '22 22:06 Zajn

Thanks @Zajn

It does look like the Faraday upgrade will require at least Ruby 2.6. CI failed for 2.5.

Ugh. I'm on Ruby 2.5.8 for the foreseeable future for $reasons.

I wonder if this means either forking the gem and maintaining two versions or; if something like this is possible:

# in the gemspec
if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.6.0')
  # new versions
  gem.add_dependency 'faraday', '~> 2.3.0'
  gem.add_dependency 'faraday-multipart', '~> 1.0.3'
  gem.add_dependency 'faraday-gzip', '~> 0.1.0'
else
 # older ruby; use current versions
  gem.add_dependency 'faraday', '< 2.0'
end

Does anyone know of other gems which take this approach? Or am I crazy?

ruckus avatar Jun 02 '22 23:06 ruckus

Does anyone know of other gems which take this approach? Or am I crazy?

@ruckus I looked through some of the top gems on github and couldn't find any that take a similar approach. It seems like that would work though.

Maybe a version update would suffice? I'm not sure where a language requirement change falls in the semver world. I guess a minor version update seems reasonable?

Zajn avatar Jun 04 '22 02:06 Zajn

hey @ruckus, apologies for the direct ping, but is there a way forward with this PR? We're in the process of upgrading Faraday and it'd be great if we didn't have to fork this gem ourselves to be able to do so. Thanks :) (similar story with https://github.com/ruckus/quickbooks-ruby/pull/579)

TomA-R avatar Sep 18 '22 07:09 TomA-R

Hi @TomA-R ; I'm traveling right now with sporadic internet access. The path forward for me would be to implement this dynamic version stuff that I posited in

https://github.com/ruckus/quickbooks-ruby/pull/578#issuecomment-1145430167

But timing-wise I won't be able to get around for this for a couple of weeks.

ruckus avatar Sep 21 '22 01:09 ruckus

Got it, thank you!

TomA-R avatar Sep 21 '22 04:09 TomA-R

I started an early exploration of doing the split-Gemfile as posited in: https://github.com/ruckus/quickbooks-ruby/pull/578#issuecomment-1145430167

.... and I don't think its a good idea. The Faraday version differences necessitate different instantiation styles alone. And it could just get messy putting version checks everywhere as needed.

I guess I have come to the conclusion perhaps maintaining two version lines is the only path forward.

1.x 2.x
<= Ruby 2.5.8 >= Ruby 2.6

Is this a sane approach too? Am I crazy?

ruckus avatar Oct 03 '22 04:10 ruckus

1.x 2.x <= Ruby 2.5.8 >= Ruby 2.6 Is this a sane approach too? Am I crazy?

Unless you're planning on adding a lot of features that will need to be ported to both versions (which I assume isn't the case?), that seems like the most reasonable approach to me too :+1:

TomA-R avatar Oct 03 '22 09:10 TomA-R

Can this PR be modified so that the Faraday gem is not locked to 2.3? Faraday is already at 2.6

marcrohloff avatar Nov 10 '22 16:11 marcrohloff

Also wanted to add that Ruby 2.5 reached EOL back in March 2021. Maybe it would be simplest just to remove it from the supported versions

marcrohloff avatar Nov 10 '22 16:11 marcrohloff

@marcrohloff I agree with this. People can pin older versions in their Gemfile if they need to use EOL versions of Ruby.

joshuapinter avatar Nov 10 '22 16:11 joshuapinter

I think this PR can now be closed, @Zajn -- the 2.0 release incorporates this :+1:

ashkulz avatar Dec 28 '22 05:12 ashkulz

Just confirming that we upgraded to 2.0.0 and this is no longer an issue. We were able to remove the pin to < 1 of faraday in our Gemfile without conflict. Thanks!

joshuapinter avatar Dec 28 '22 16:12 joshuapinter