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

Replace Excon adapter with Faraday::Excon gem, and fix autoloading issue with Faraday::NetHttpPersistent #1257

Merged
merged 2 commits into from Apr 12, 2021

Conversation

@iMacTia
Copy link
Member

@iMacTia iMacTia commented Apr 12, 2021

Description

Use Faraday::Excon gem instead of internal adapter.
Also: fixes autoloading issues with Faraday::NetHttpPersistent.

Also: fixes autoloading issues with Faraday::NetHttpPersistent
@iMacTia iMacTia requested a review from olleolleolle Apr 12, 2021
@olleolleolle olleolleolle changed the title Replace Excon adapter with Faraday::Excon gem Replace Excon adapter with Faraday::Excon gem, and fix autoloading issue with Faraday::NetHttpPersistent Apr 12, 2021
@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented Apr 12, 2021

I see you did 2 things, so I added the 2nd thing to the title. Makes the commit message truer.

OK, cop.

faraday.gemspec:20:3: C: Gemspec/OrderedDependencies: Dependencies should be sorted in an alphabetical order within their section of the gemspec. Dependency faraday-excon should appear before faraday-net_http_persistent.
  spec.add_dependency 'faraday-excon', '~> 1.0'
Copy link
Member

@olleolleolle olleolleolle left a comment

❤️ This has been a long time coming, and it is happening!

@iMacTia thank you!

@olleolleolle olleolleolle merged commit 20be816 into master Apr 12, 2021
6 of 7 checks passed
6 of 7 checks passed
@github-actions
linting
Details
@github-actions
build 2.4
Details
@github-actions
build 2.5
Details
@github-actions
build 2.6
Details
@github-actions
build 2.7
Details
@github-actions
build 3.0
Details
codeclimate 2 issues to fix
Details
@olleolleolle olleolleolle deleted the faraday-excon-gem branch Apr 12, 2021
@gurgeous
Copy link
Contributor

@gurgeous gurgeous commented Apr 17, 2021

Thank you. This speeds up faraday load time by ~20%.

@rainerborene
Copy link

@rainerborene rainerborene commented Apr 17, 2021

shouldn't faraday-excon and faraday-net_http_persistent be optional dependencies? what is the reasoning behind shipping two HTTP adapters? I believe most people will use the default Net::HTTP adapter. I don't have any data to sustain this statement but a quick search on GitHub could show that.

@iMacTia
Copy link
Member Author

@iMacTia iMacTia commented Apr 18, 2021

@rainerborene totally agree! That's the plan for Faraday v2.0 🎉!
The only reason we keep shipping them in the 1.x releases is for backwards compatibility, but they will all go away in v2.0 and you'll simply add the one you want to use 👌

@tisba
Copy link

@tisba tisba commented Apr 18, 2021

I was wondering why excon made it into my dependencies then I found this (we don't use excon). I think it should be mentioned in the Changelog explicitly as this is quite confusing.

@iMacTia
Copy link
Member Author

@iMacTia iMacTia commented Apr 18, 2021

@tisba Ah I think I see what @rainerborene also meant now. The adapters need to be included, as they were included with Faraday before, but excon and net_http_persistent were not, people had to manually add them to the Gemfile before.
They now are part of the adapters dependencies.

This clearly doesn't work, by the time we move out all adapters people will have their dependencies full of clients they might not be using. I'll get this fixed and release a v1.4.1 to address that, thanks both for raising it 👍 !

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

Successfully merging this pull request may close these issues.

None yet

5 participants