search

miyagawa / faraday-cookie_jar

Client-side cookie management for Faraday
MIT License
76 stars 12 forks source link

Avoid checking (and preloading) Faraday only to support 0.7.x #14

Closed chesterbr closed 4 years ago

chesterbr commented 4 years ago

Context

An earlier update on this project added support to Faraday middleware API (which is the only supported way as of now). Unfortunately, the fallback check (Faraday.responds_to? in the registration code - that is, the if in:

if Faraday.respond_to? :register_middleware
  Faraday.register_middleware :cookie_jar => lambda { Faraday::CookieJar }
elsif Faraday::Middleware.respond_to? :register_middleware
  Faraday::Middleware.register_middleware :cookie_jar => Faraday::CookieJar
end

triggers a load of Faraday that messes up with its configurations configurations in some specific classloader/forking situations (happened in our CI system).

Proposal

We considered flipping the checks, but verified that Faraday::Middleware#register_middleware works as far back as 0.8.0. So by slightly bumping the minimum version (still much below what most people should be using in recent years), we can just drop the initial test, fixing the issue for us (and potentially others) while DRYing the code a bit.

In fact, this seems to be what other middleware do these days. For example, registration code in faraday_middleware goes like:

  if Faraday::Middleware.respond_to? :register_middleware
    ...
    Faraday::Middleware.register_middleware \
      ...
  end

Tests

Did not find an easy way to write a test, but given that a failure on registration will break the entire suite, it can be used as a test on itself, so I ran the specs on different Faraday versions.

They ran successfully with Faraday 1.0.1 (the current one), so I went back and verified that even Faraday 0.8.0 works, so I reduced the impact by only bumped the minimum to it (instead of 0.9.0, which was the version that motivated @cameron-martin to add the support in first place).

Version and Changelog

I could not find instructions on whether you'd like to just receive the change and do the changelog/version bump yourself, so I made those in a separate commit (which I can revert/squash if you prefer, just let me know).

Even though it is a theoretical compatibility change, projects with Faraday versions that don't support the API will not update via Bundler, so I found it safe to just bump the patch component.

miyagawa commented 4 years ago

LGTM, thanks!