Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP]: Upgrade to Faraday v2 #578

Closed
wants to merge 3 commits into from
Closed

Conversation

Zajn
Copy link
Contributor

@Zajn Zajn commented Jun 1, 2022

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 added 3 commits June 1, 2022 17:45
This required adding the `multipart` middleware which used to be
bundled with Faraday in versions < 2.
@Zajn
Copy link
Contributor Author

Zajn commented Jun 1, 2022

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

@ruckus
Copy link
Owner

ruckus commented Jun 2, 2022

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
Copy link
Contributor Author

Zajn commented Jun 4, 2022

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
Copy link

TomA-R commented Sep 18, 2022

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 #579)

@ruckus
Copy link
Owner

ruckus commented Sep 21, 2022

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

#578 (comment)

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

@TomA-R
Copy link

TomA-R commented Sep 21, 2022

Got it, thank you!

@ruckus
Copy link
Owner

ruckus commented Oct 3, 2022

I started an early exploration of doing the split-Gemfile as posited in: #578 (comment)

.... 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
Copy link

TomA-R commented Oct 3, 2022

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 👍

@marcrohloff
Copy link

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

@marcrohloff
Copy link

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
Copy link

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

marcrohloff added a commit to marcrohloff/quickbooks-ruby that referenced this pull request Nov 10, 2022
* Replace custom Faraday GZip implementation with `faraday-gzip` gem

Credit goes to [Zachary Jones/Zajn](https://github.com/Zajn)
Original [PR ruckus#58](ruckus#578)
@ashkulz
Copy link

ashkulz commented Dec 28, 2022

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

@Zajn Zajn closed this Dec 28, 2022
@joshuapinter
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants