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

removal of proc mechanism for middleware #1389

Closed
notEthan opened this issue Jan 29, 2022 · 4 comments · Fixed by #1391
Closed

removal of proc mechanism for middleware #1389

notEthan opened this issue Jan 29, 2022 · 4 comments · Fixed by #1391

Comments

@notEthan
Copy link

following PR #1301: Autoloading, dependency loading and middleware registry cleanup

I can no longer register middleware like:

Faraday::Request.register_middleware(:my_middleware => proc { MyMiddleware })

this removal is unfortunate. that PR says "This change should not affect you directly" - but this has been a publicly documented method of registering middleware for a very long time. I have been using this mechanism for at least 8 years across a number of applications and gems. I'm sure I am far from the only one.

I understand refactorings made this method of registration no longer needed by faraday itself, but this was a very useful mechanism for me. I could offer faraday middleware, but avoid the overhead of loading the associated implementation unless/until the application requires it.

deferring also helped ease dependency resolution - I'll have to refactor to put faraday code later in a number of places. this part is not a huge deal, but was a convenient thing that has been lost.

I really hope you'll consider restoring support for this. it's a better way to register middleware, in my opinion.

it's also surprising to see no warnings about deprecation or anything. on upgrading, I just get a meaningless message from faraday's internals:

NoMethodError: undefined method `<=' for #<Proc:0x00005576b950d4c8>
  Did you mean?  <=>
    /var/lib/gems/2.7.0/gems/faraday-2.1.0/lib/faraday/rack_builder.rb:238:in `is_adapter?'
    /var/lib/gems/2.7.0/gems/faraday-2.1.0/lib/faraday/rack_builder.rb:224:in `raise_if_adapter'
    /var/lib/gems/2.7.0/gems/faraday-2.1.0/lib/faraday/rack_builder.rb:97:in `use'
    /var/lib/gems/2.7.0/gems/faraday-2.1.0/lib/faraday/rack_builder.rb:242:in `use_symbol'
    /var/lib/gems/2.7.0/gems/faraday-2.1.0/lib/faraday/rack_builder.rb:103:in `request'

I looked in the CHANGELOG.md but faraday 2 isn't even mentioned. after eventually finding the PR with this change I realized there's another file, UPGRADING.md, that I had not noticed, not linked from the readme or anywhere. this change is not very well communicated.

iMacTia added a commit that referenced this issue Feb 2, 2022
…rings or procs

* Update UPGRADING.md to explain the original removal from v2.0
* Update CHANGELOG.md to point to the `releases` page

Fixes #1389
@iMacTia
Copy link
Member

iMacTia commented Feb 2, 2022

@notEthan thanks a lot for raising this and for sharing your concerns.
I have to say, I was surprised as well when I noticed the extent of the changes in that PR.
I agree the possibility of providing a string, symbol or proc don't really add too much complexity and could have been kept, I'm sorry if I got taken away with the cleanup.

I'm also sorry if the UPGRADING.md file didn't mention this, I suspect this removal was not really thought out properly to begin with.

I've opened #1391 to address both this issues, and I've also pointed out in the CHANGELOG.md file that people should now check the GitHub releases page for changelogs.

Thanks for all the useful feedback, and I hope this will let you keep the existing code.
As soon as the PR is merged I'll release it as v2.2

olleolleolle pushed a commit that referenced this issue Feb 2, 2022
Reintroduce the possibility to register middleware with symbols, strings or procs.

Fixes #1389
olleolleolle pushed a commit that referenced this issue Feb 2, 2022
Reintroduce the possibility to register middleware with symbols, strings or procs.

Fixes #1389
@notEthan
Copy link
Author

notEthan commented Feb 3, 2022

fantastic, I really appreciate the positive response. everything seems to be compatible again, pointing my code at that PR branch. thanks very much for all your good work on this gem and the whole ecosystem.

@olleolleolle
Copy link
Member

@notEthan Thanks for making sure the solution worked for you! Cheers!

@iMacTia
Copy link
Member

iMacTia commented Feb 3, 2022

Thanks for confirming @notEthan, I'm shipping v2.2 now 🎉

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 a pull request may close this issue.

3 participants