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

Bump Active Support dependency to version >= 2 #257

Merged
merged 5 commits into from
Jul 11, 2016
Merged

Conversation

JuanitoFatas
Copy link
Contributor

@JuanitoFatas JuanitoFatas commented Jun 28, 2016

Closes #252

This Pull Request unlocks Active Support dependency to >= 2, so that App used HTML::Pipeline can upgrade to Rails 5.

Changes

Appraisal Usage

  • Run tests with Rails 3: $ appraisal rails-3 rake
  • Run tests with Rails 4: $ appraisal rails-4 rake
  • Run tests with Rails 5: $ appraisal rails-5 rake
  • Run above all combined: $ appraisal rake

Exclude builds

Allowed Failures

screenshot 2016-07-03 16 25 17

ruby-head is nice to pass, but failed is fine.

Test with Rails 5 apps

HTML::Pipeline works great with these Rails 5.0.0 apps:

@JuanitoFatas
Copy link
Contributor Author

@jch @simeonwillbanks 👍 ?

@jch
Copy link
Contributor

jch commented Jun 28, 2016

Sorry, I haven't had a chance to test this on Rails 5 yet. In addition to the tests, has this been used in a production app yet? We're still using a patched version of Rails 3 and moving towards 4.

@JuanitoFatas
Copy link
Contributor Author

JuanitoFatas commented Jun 29, 2016

has this been used in a production app yet?

Yes, the RedDotRubyConf webiste uses HTML::Pipeline. And I created a Rails 5 app with some specs.

For tests: I can setup a test suite with https://github.com/thoughtbot/appraisal. But I am also thinking create some real apps and put them under github.com/html-pipeline organization, what do you think?

@JuanitoFatas JuanitoFatas force-pushed the bump-rails-dependency branch 3 times, most recently from 2f93f2f to 663a4ae Compare July 1, 2016 09:24
@JuanitoFatas JuanitoFatas force-pushed the bump-rails-dependency branch 2 times, most recently from 8eb4f1d to 08915cf Compare July 1, 2016 09:41
@JuanitoFatas JuanitoFatas mentioned this pull request Jul 1, 2016
@JuanitoFatas
Copy link
Contributor Author

JuanitoFatas commented Jul 1, 2016

@jch Please review and 🚢:shipit::heart:

Because Rails 3's `try` method behavior does not dance with its
successors:

Rails 3:

"".try(:call)
=> NoMethodError: undefined method `call' for "emoji":String

Rails 4+:

"".try(:call)
=> nil
@bdewater
Copy link

bdewater commented Jul 3, 2016

Instead of allow_failures for Rails 5 and Ruby versions < 2.2.2, use exclude to save CPU time on Travis.

@JuanitoFatas
Copy link
Contributor Author

Instead of allow_failures for Rails 5 and Ruby versions < 2.2.2, use exclude to save CPU time on Travis.

Thanks Bart! ❤️ Updated .travis.yml in a31fa29.

@glebm
Copy link

glebm commented Jul 4, 2016

has this been used in a production app yet?

Also now running on https://thredded.org/ with Rails 5.0.0 and this PR as of this comment.

@n-rodriguez
Copy link

hi there! any news?

@jch jch merged commit 7063561 into master Jul 11, 2016
@jch jch deleted the bump-rails-dependency branch July 11, 2016 23:08
@parkr
Copy link
Contributor

parkr commented Jul 15, 2016

@jch
Copy link
Contributor

jch commented Jul 21, 2016

@parkr good catch. This has been a frustration each time html-pipeline upgrades it's dependencies be because of it's loose constraints on versions. I proposed splitting up the gem into smaller gems to narrow down these problems in #182, but did not make any changes at the time.

Could you talk more about why this is breaking for specific versions of Ruby? What workaround have you taken for it?

@parkr
Copy link
Contributor

parkr commented Jul 22, 2016

Could you talk more about why this is breaking for specific versions of Ruby?

Absolutely. The issue affects anyone installing jemoji or jekyll-mentions, two gems which rely on this gem (bindings for the emoji filter and mentions filter, respectively). When one specifies gem "jekyll-mentions" in one's Gemfile and executes bundle install, they see the error saying that activesupport 5.0.0 requires Ruby 2.2.2 or higher to install. Instead of gracefully downgrading to activesupport 4.2.7, it simply fails. The two plugins I mentioned above simply specify their dependency on html-pipeline. Neither bundler nor RubyGems for Ruby 2.1 appear to be able to gracefully downgrade a dependency until they all resolve to installable versions.

What workaround have you taken for it?

The workaround we went for is to lock down activesupport to ~> 4.0 (e.g. jekyll/jemoji#47) as it is compatible with all Ruby versions we test with (2.0, 2.1, 2.2, 2.3). I looked at the emoji filter and all activesupport is used for is HashWithIndifferentAccess. 😦 Perhaps we could remove that dependency on the emoji filter?

@JuanitoFatas
Copy link
Contributor Author

The workaround we went for is to lock down activesupport to ~> 4.0 (e.g. jekyll/jemoji#47) as it is compatible with all Ruby versions we test with (2.0, 2.1, 2.2, 2.3).

How about release a patch version of Jemoji that stays with Active Support 4.x and a new major version that supports Active Support 5+? 😅

I looked at the emoji filter and all activesupport is used for is HashWithIndifferentAccess. 😦 Perhaps we could remove that dependency on the emoji filter?

If we want to remove Active Support, we need to remove all usage of Active Support across the html-pipeline gem. Otherwise html-pipeline still depend on Active Support. The barrier is the use of coverting document to hash. 😓

@parkr
Copy link
Contributor

parkr commented Jul 25, 2016

How about release a patch version of Jemoji that stays with Active Support 4.x

We did this, actually. GitHub Pages uses Ruby 2.1.7 in production so we locked the downgrade and all was well.

It's not the end of the world, but there were a couple days where anyone using Ruby < 2.2.2 couldn't install this gem in our projects.

@Taher-Ghaleb
Copy link

Hello,

Sorry, I know it's been a long time ago, but I am curious why caching hasn't been enabled before this (i.e., since December 17, 2014, when it became available to open source projects).

Thank you.

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.

7 participants