-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Make solidus_core depend on actionmailer and activerecord instead of rails #2272
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rails is still being used and called directly in many places in core, (for instance:
@cache = Rails.cache |
Just removing the gems doesn't remove our dependence on it, we're just counting on other places to include it for us, which isn't a good practice IMO.
@cbrunsdon nope... Wait, isn't that from railties? Rails only includes the https://github.com/rails/rails/blob/master/rails.gemspec#L21 And for the https://github.com/rails/rails/blob/master/railties/lib/rails.rb#L38 |
At the very least I think we'll need to add activesupport, activejob, and railties, which are the three I know we depend on inside of core. |
@BenMorganIO aye, |
We do get these from other gems, but it would make sense to add them to our deps |
core/solidus_core.gemspec
Outdated
%w[activerecord actionmailer].each do |rails_dep| | ||
%w[ | ||
actionmailer actionpack actionview activejob activemodel activerecord | ||
activesupport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
railties too please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a comprehensive list now of everything we need 👍
We originally closed #2239 because we thought we were going to get it in from #2236 but never did. So... Here's a new PR!
Before:
$ bundle Bundle complete! 30 Gemfile dependencies, 126 gems now installed.
After:
$ bundle Bundle complete! 30 Gemfile dependencies, 123 gems now installed.