search

AaronLasseigne / active_interaction

:briefcase: Manage application specific business logic.
MIT License
2.07k stars 137 forks source link

Only depend on the specific gems that are required #533

Closed stevecrozz closed 2 years ago

stevecrozz commented 2 years ago

active_interaction currently declares a dependency on all of rails, but it really only needs a few gems from the rails family. I am suggesting we specify those gems specifically as some apps may not want to depend on all of rails

AaronLasseigne commented 2 years ago

I specifically shifted away from picking and choosing because I can't imagine anyone is actually using this outside of Rails. Is there a meaningful reason to making this shift?

stevecrozz commented 2 years ago

I think there is a meaningful reason. Although most people don't, you can actually use rails itself without depending on all of rails. Some folks may not want to depend on all the Rails gems in order to reduce the complexity of unnecessary dependencies like this (although not documented here is the fact that this lets you trim the actual Gemfile too): https://andycroll.com/ruby/turn-off-the-bits-of-rails-you-dont-use/

Others may want to reduce dependencies in order to decrease their image/slug sizes.

And there are other weird cases like this famous one from last year: https://twitter.com/nateberkopec/status/1374724767593336835

This also seems to be the way most other gems (at least the ones in the Gemfile I'm currently looking at) work. I plucked a few interesting examples from my current dependency list. I only found one non-Rails gem in my Gemfile that directly depends on rails (not counting active_interaction which we're currently adding). https://github.com/ActiveCampaign/postmark-rails/blob/a8ed2de93cc9b94b3d8f6b9c585aa5d18155250d/postmark-rails.gemspec#L22 https://github.com/Dynamoid/dynamoid/blob/fdcfa23d189583556009a9478c35b69b5773b727/dynamoid.gemspec#L46 https://github.com/rails/rails-controller-testing/blob/932aef15e4539977768bb220d7f4c7dbae8d8924/rails-controller-testing.gemspec#L21-L23 https://github.com/ClosureTree/with_advisory_lock/blob/851322b91a435fc68b45c989b931be1ea77a3f97/with_advisory_lock.gemspec#L20 https://github.com/ankane/strong_migrations/blob/8a025dabdbbffe1a3a3bf3ca4a6d635dcceb4a6c/strong_migrations.gemspec#L18

wulffeld commented 2 years ago

Why is ActiveRecord required? I really think it should ORM agnostic. I'm personally using v4 with Mongoid in several projects.

stevecrozz commented 2 years ago

@wulffeld There are some direct references to ActiveRecord in ActiveIntegration::ArrayFilter. This reference could probably be made optional or more flexible somehow. ActiveRecord is also used in specs. That reference could definitely be made into a dev dependency if we were so inclined.

This is beyond the scope of what I'm trying to achieve here, but happy to work with you on a PR for that if you can get some buy-in from @AaronLasseigne.

AaronLasseigne commented 2 years ago

I'm fine with rolling back to more bare requirements. There will have to be a change to the gemspec files as well. It'll basically mean undoing https://github.com/AaronLasseigne/active_interaction/commit/68b860273583668ddb5eb045969d06c0bf290bea and then adding checks for the existence of classes from things like ActiveRecord.

AaronLasseigne commented 2 years ago

Closing in favor of 78f62fd30de7bddaad8a9d5f2660c345b0fc4962.