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

Fix Dependency Resolution for Rails 5 #22

Closed
wants to merge 5 commits into from
Closed

Conversation

freerobby
Copy link
Member

@freerobby freerobby commented Sep 1, 2016

This fixes #20. The fundamental problem is that we require any version of ActiveResource beyond 2.3.8; but Bundler's versioning rules do not permit such a broad requirement. All of their versioning inequalities have implied boundaries around the major version of a dependency. In practice, this renders it impossible to have a single version of our gem that supports all of [Rails 2, Rails 3, Rails 4, Rails 5].

One way to work around this would be to maintain separate branches, and release separate gem versions, one for each Rails major version. However that has two downsides. First, it requires every change to this gem to be applied to four separate branches, which is an annoying workflow for all maintainers. Second it requires our users to break Gemfile conventions in that anybody who is not on latest Rails would have to manually specify an old version of our gem (rather than letting Bundler deduce a reasonable version) when they first install it.

Instead, this fix vendors ActiveResource and ActiveSupport (a dependency of ActiveResource) so that neither is a gem dependency. In order to be safe, I have completely re-namespaced these libraries to WistiaActiveResource and WistiaActiveSupport, respectively. This way, if a host application uses both our gem and a specific version of one of these libraries, there is no conflict. This has an added benefit that if Rails ever changes the APIs we consume, we will be insulated from those changes.

I am not happy with having such a large footprint for such a small gem. Any use of this gem in a Rails app will now cause these libraries to load twice rather than once. I don't do that lightly, but accepting that cost allows us to maintain a simple, conventional workflow, and make things as easy as possible for both our users and our maintainers.

@bschwartz @emb78 @ryanartecona @davejachimiak

cc @paublyrne @sublimecoder @DanielKehoe @tute

…is causing problems supporting multiple versions of Rails. Develop against Ruby 1.9.3 to avoid IConv requirement for now.
…tiaActiveResource to avoid any conflicts with versions of those libraries that other gems may load.
@bschwartz
Copy link
Member

@freerobby Given the alternatives, this seems like a wise approach! Nice work! And like we talked about in person today, when we version up to API v2, we'll be sure to not have any external dependencies in any client libs to avoid this situation again.

@rocketbop
Copy link

This is great, @freerobby, @bschwartz thanks a lot for this.

Appreciate the fast response. 👍

@tute
Copy link

tute commented Sep 1, 2016

Thanks for the proposed fix!

Why do we need to include the whole libraries? Can't we just include the pieces of code we use?

Also, if this is a stable gem, what do you think of releasing a new major version that targets the newer versions of Rails?

Thank you!

@freerobby
Copy link
Member Author

@tute I looked briefly at bringing in only specific pieces of ActiveResource but we use a great deal of it through its fuzzy finder metaprogramming. ActiveSupport is a better candidate but with all of its core extensions (many of which are used by ActiveResource), there is a lot to go through to deduce what is truly safe to remove. What we probably could do is keep all of the core extensions and replace the ActiveSupport JSON Serializer with the native Ruby one. While I think that's a great idea, I would rather get this out now and leave that as a project for the future, as it does increase the scope and complexity of this change. Pull requests are accepted. :)

We will definitely release a new version of the gem after this merge. I was planning to make this only a patch level bump, so that users would know it is safe to upgrade without having to worry about any API changes and without going hunting for new functionality (per semver spec).

@tute
Copy link

tute commented Sep 1, 2016

What we probably could do is keep all of the core extensions and replace the ActiveSupport JSON Serializer with the native Ruby one. While I think that's a great idea, I would rather get this out now and leave that as a project for the future, as it does increase the scope and complexity of this change. Pull requests are accepted. :)

You are a good maintainer. :) This looks good enough, indeed!

If I might, I'd only add the good explanation you have in the PR description into the commit messages, so that it's clear to future developers while digging into the project's history.

Thank you for your work!

@freerobby
Copy link
Member Author

@tute Thanks for the kind words. Good point about adding that explanation to the git comments -- will do.

Unfortunately, in manual testing I hit an error with this approach in Rails 5. The issue has to do with ActiveSupport 2 core extensions and ActiveSupport 5 cor extensions clobbering each other (they are, by design, modifying native Ruby objects in the global namespace, not their own). I'm going to look at this further today.

@DanielKehoe
Copy link

You're trying to support deprecated versions of Rails. Would it be better to only target supported versions of Rails and expect anyone using deprecated versions of Rails to lock to earlier releases of the Wistia gem? Am I missing something here.

@sublimecoder
Copy link

@DanielKehoe That's the way a ton of other projects work. They even include those instructions in the readme.

@freerobby
Copy link
Member Author

@DanielKehoe @sublimecoder wistia-api is a Ruby library, not a Rails library; we "support" rails only to the extent that a user should be able to use both wistia-api and Rails simultaneously. We don't integrate or interface with Rails, be it version 2, 3, 4, or 5.

The issue here is that Rails 5 discontinued ActiveResource without releasing a version that is forwards-compatible with ActiveSupport, and the most recent release of ActiveResource (version 4) requires that ActiveResource's version must match ActiveSupport's version. So we wind up with these requirements:

  1. Rails version == ActiveSupport version
  2. ActiveSupport version == ActiveResource version.

There is no ActiveResource 5, so if you require Rails 5, this will never resolve. As a result, any gem that depends on any released version of ActiveResource will fail to resolve in a Rails 5 app. Even if we wanted to support only Rails 5, we'd have to abandon ActiveResource completely in order for dependencies to resolve.

This issue stems from a combination of (1) Rails' historic requirement that dependency versions matches exactly, and (2) Rails' choice not to maintain future versions of all of those dependencies. This is frustrating, but we're not going to break support for all Rails users of this gem over that, so this is my best attempt at a workaround.

@freerobby freerobby changed the title Fix Dependency Resolution for Rails 3, 4 and 5 Fix Dependency Resolution for Rails 5 Sep 6, 2016
@freerobby
Copy link
Member Author

@paublyrne @tute I have this working locally in a Rails 5 project. However, it's tricky to test extensively because ActiveSupport loads so many different things dynamically based on what is and isn't available, which varies from application to application.

Before I merge this into master, would either of you be willing to be a guinea pig? If you could try using this line in your Gemfile for wistia-api, and tell me if it works as expected, it'd be much appreciated!

gem 'wistia-api', git: 'git@github.com:wistia/wistia-api.git', branch: 'rails-5'

@excid3
Copy link

excid3 commented Sep 16, 2016

@freerobby I gave this a test today in Rails 5 and ran into:

Gem Load Error is: undefined method `attr_accessor_with_default' for #<Class:WistiaActiveResource::Base>
Did you mean?  attr_internal_accessor
Backtrace for gem load error is:
/Users/chris/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/bundler/gems/wistia-api-867f73eb7e7e/lib/vendor/activeresource-2.3.18/lib/wistia_active_resource/base.rb:405:in `singleton class'

@jbodah
Copy link

jbodah commented Sep 22, 2016

All of their versioning inequalities have implied boundaries around the major version of a dependency

I'm not sure that is intended. Could this be a bundler resolution bug? For what it's worth Bundler does properly resolve activeresource to 4.2 in the wistia-api repo when you remove the Gemfile.lock

@freerobby
Copy link
Member Author

freerobby commented Sep 22, 2016

@jbodah Yeah, you're correct, that piece was incorrectly documented in the Rubygem guide. (Aside: I submitted this clarification to fix it: rubygems/guides#162)

The actual issue has to do with the versioning constraints that Rails imposes combined with its discontinuing ActiveResource. I outlined it a bit more in #22 (comment)

@jbodah
Copy link

jbodah commented Sep 22, 2016

@freerobby
Copy link
Member Author

@jbodah Exactly, and additionally, every other release of activeresource is version-locked to its respective version of activesupport.

@ThomasBush
Copy link

Just wanted to check on status as I am working on an upgrade to rails 5. I get the same error as @excid3 when using the 'rg-fix-dependencies' branch.

@freerobby
Copy link
Member Author

@excid3 @ThomasBush Thank you both for the feedback. The autoloading behavior in ActiveSupport makes it very difficult to pinpoint what code will and what code won't execute when placed in a new environment. I don't think I can infer all of the possibilities and deal with them responsibly.

Given that, and that this gem is built on ActiveResource, and that ActiveResource cannot be loaded in a Rails 5 app, I think the best course of action for Rails 5 users is to manage your own HTTP calls to Wistia rather than rely on our client library.

I'm wish I had a better answer, but based on what I've learned, this is the optimal path to accessing the Wistia API in a Rails 5 application.

@freerobby freerobby closed this Oct 17, 2016
@freerobby freerobby mentioned this pull request Oct 17, 2016
@tute
Copy link

tute commented Oct 17, 2016

👍

Thanks for your work.

@excid3
Copy link

excid3 commented Oct 17, 2016

How would you feel about doing a new major release that uses rest-client
instead of active resource? That's what the stripe gem uses and would be a
pretty solid solution moving forward. Some of the Ruby interface will
change so it would require a major version change but I see it being worth
it.

I'd be happy to start a fork for that implementation. I'm going to need to
build this anyways so might as well make it reusable for other people at
the same time. Thoughts?

On Monday, October 17, 2016, Robby Grossman notifications@github.com
wrote:

@excid3 https://github.com/excid3 @ThomasBush
https://github.com/ThomasBush Thank you both for the feedback. The
autoloading behavior in ActiveSupport makes it very difficult to pinpoint
what code will and what code won't execute when placed in a new
environment. I don't think I can infer all of the possibilities and deal
with them responsibly.

Given that, and that this gem is built on ActiveResource, and that
ActiveResource cannot be loaded in a Rails 5 app, I think the best course
of action for Rails 5 users is to manage your own HTTP calls to Wistia
rather than rely on our client library.

I'm wish I had a better answer, but based on what I've learned, this is
the optimal path to accessing the Wistia API in a Rails 5 application.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#22 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEGFWcp2ifoeQFuFHQX8osdICJ9wvNUks5q04ImgaJpZM4JyOO2
.

Chris Oliver
http://excid3.com | @excid3 http://twitter.com/excid3 |
http://github.com/excid3

@freerobby
Copy link
Member Author

freerobby commented Oct 17, 2016

@excid3 We probably won't build that in the near term, as we are working on a v2 API based on JSON-API. However, if you want to fork this repo and build a rest-client (or httparty, etc)-based version on the current API, we'd certainly be open to releasing that as a new major version. Hopefully the Ruby interface is straightforward, but feel free to open up issues and/or email me (robby@wistia) if you want to talk anything through.

@esbanarango
Copy link

Hello @excid3 didi you end up working on that fork? Thank you in advance.

@excid3
Copy link

excid3 commented Feb 23, 2017

I just made a simple HTTParty class for the Medias only. Happy to share that if it's useful for anyone.

class Wistia
  cattr_accessor :password

  class Media
    include HTTParty
    base_uri "api.wistia.com/v1"
    #debug_output $stdout

    def all
      results = []
      num = 1

      while medias = page(page: num).to_a
        break if medias.empty?

        results += medias
        num += 1
      end

      results
    end

    def page(options={})
      options[:page] ||= 1
      options[:per_page] ||= 100
      options.merge!(api_password: Wistia.password)
      self.class.get("/medias.json", query: options)
    end
  end
end

@esbanarango
Copy link

Thank you @excid3

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.

Rails 5 support?
10 participants