removal of proc mechanism for middleware
notEthan opened this issue · 4 comments
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.
@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
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.
@notEthan Thanks for making sure the solution worked for you! Cheers!