ruckus / quickbooks-ruby

Quickbooks Online REST API V3 - Ruby
MIT License
374 stars 302 forks source link

[WIP]: Upgrade to Faraday v2 #578

Closed Zajn closed 1 year ago

Zajn commented 2 years ago

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 commented 2 years ago

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

ruckus commented 2 years ago

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?

Zajn commented 2 years ago

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?

TomA-R commented 1 year ago

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)

ruckus commented 1 year ago

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.

TomA-R commented 1 year ago

Got it, thank you!

ruckus commented 1 year ago

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?

TomA-R commented 1 year ago

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:

marcrohloff commented 1 year ago

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

marcrohloff commented 1 year ago

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

joshuapinter commented 1 year ago

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

ashkulz commented 1 year ago

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

joshuapinter commented 1 year ago

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!