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

Drop activesupport dependency from sinatra-contrib #1448

Merged
merged 1 commit into from
Oct 27, 2018

Conversation

yob
Copy link
Contributor

@yob yob commented Jun 15, 2018

ActiveSupport was being loaded to provide Object#try. It's a very large dependency that can monkey patch many core classes, and although the require in sinatra-contrib is targeted and doesn't load all of ActiveSupport, once the gem is on the load path some other libraries will opportunistically load more of it.

Object#try is only used twice, maybe it's ok to live with a manual check for nil for now? sinatra-contrib currently requires ruby 2.2 or better - once the minimum is lifted to 2.3, Object#try will be provided by ruby core.

ActiveSupport was being loaded to provide Object#try. It's a very large
dependency that can monkey patch many core classes, and although the
require in sinatra-contrib is targeted and doesn't load all of
ActiveSupport, once the gem is on the load path some other libraries
will opportunistically load more of it.

Object#try is only used twice, maybe it's ok to live with a manual check
for nil for now? sinatra-contrib currently requires ruby 2.2 or better -
once the minimum is lifted to 2.3, Object#try will be provided by ruby
core.
@yob yob changed the title Drop activesupport dependency Drop activesupport dependency from sinatra-contrib Jun 15, 2018
@namusyaka
Copy link
Member

I like this patch, but I'm worried about implicit backword compatibility destruction.
See #1410 #1208

@namusyaka namusyaka added this to the v2.1.0 milestone Jun 19, 2018
@yob
Copy link
Contributor Author

yob commented Jun 19, 2018

Oh, thanks for the pointer to #1208 - I hadn't noticed that someone else had tried almost the same changes as me (buffer.clear if buffer.respond_to?(:clear), etc).

My reading of the earlier PR is that the discussion about breaking backwards compatibility is centered around changing the method signature of Sinatra::Contrib::Loader#register.

This PR is more targeted in scope than #1208 and doesn't attempt to remove backports (although I'm also broadly supportive of that in the long term), and I don't think there are any backwards compatibility issues with the subset of changes proposed here. It's possible I'm wrong though, it happens occasionally.

@KrauseFx
Copy link

Thanks @yob for putting in the work. Just wanted to voice our support as the fastlane team that we'd love to not have the active_support dependency (more context in #1451) 🎉

@namusyaka
Copy link
Member

okay, I'm going to revisit this patch next week.
Thanks for your contribution!

@mdub
Copy link

mdub commented Jun 21, 2018

I agree, it's a shame that #1208 was reverted in it's entirety, as the changes to capture.rb looked kosher, and removed dependency on #try.

@yob: Might the respond_to? approach taken in #1208 be safer than just checking for nil, from a backward compatibility point of view?

@yob
Copy link
Contributor Author

yob commented Jun 21, 2018

Might the respond_to? approach taken in #1208 be safer than just checking for nil, from a backward compatibility point of view?

I don't have a strong feeling either way - I'd be happy to change to the respond_to? approach if others prefer it.

@tibbon
Copy link

tibbon commented Jul 30, 2018

Can we also just use the safe navigation operator? ActiveSupport seems really big to pull in here just for try. I commit to doing whatever work is needed to make that happen.

@raxoft
Copy link
Contributor

raxoft commented Aug 1, 2018

OK, guys, what's the status of this? Can we finally get rid off of the #try with either nil check or respond_to? check? It was real let down for our team to see activesupport brought into the list of dependencies. One of the reasons for choosing sinatra in the first place was to keep the bloat at minimum and the auditing scope sane, so I would really appreciate if we can see this dependency gone. Thanks.

@dentarg dentarg mentioned this pull request Aug 18, 2018
@jpaulgs
Copy link

jpaulgs commented Sep 25, 2018

Is there movement here? I'd like to pull out active support from my project..

@raxoft
Copy link
Contributor

raxoft commented Oct 4, 2018

Hmm, 2.0.4 is out and this has not been included? That's sad. For our team, sinatra is on hold until the activesupport goes away, so any releases not addressing this are kind of moot.

@npickens
Copy link

What are the outstanding concerns? If there are none, let's get this merged.

@mdub
Copy link

mdub commented Oct 14, 2018

I withdraw my earlier suggestion re backward compatibility, as @yob's version is more consistent with the ActiveSupport implementation of #try. 👍

It would be really nice to see this merged and released.

Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Sorry for the delay, but thanks for your contribution!

@namusyaka namusyaka merged commit e0aa37d into sinatra:master Oct 27, 2018
@npickens
Copy link

Thank you!! 👍👍👍

@BuonOmo
Copy link
Contributor

BuonOmo commented Nov 8, 2018

If the support for active_support has been removed here. Why is there still a lock for active_support 5.1.6 in Sinatra's Gemfile?

gem "activesupport", "~> 5.1.6"

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 6, 2019
### sinatra-contrib

* Add `flush` option to `content_for` [#1225](sinatra/sinatra#1225) by Shota Iguchi

* Drop activesupport dependency from sinatra-contrib [#1448](sinatra/sinatra#1448)

* Update `yield_content` to append default to ERB template buffer [#1500](sinatra/sinatra#1500) by Jordan Owens
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.

9 participants