[RFC] Pull rails dependency out of core into solidus_rails #2236

cbrunsdon · 2017-09-22T22:54:53Z

Solidus core doesn't need to be a mountable rails engine.

Core only provides its rails engine for use in solidus_frontend, solidus_backend, and solidus_api. By creating a new gem (solidus_rails), and pushing the rails dependencies into it, we can lesson the surface area of core.

This would allow us more room to try other technologies, build smaller modules to enhance core, and allow us to develop significantly faster (without constantly needing a full rails env to do anything). This removes the requirement to run test_app or use the spec/dummy to run tests.

It will also give us a new layer to keep compatibility in with existing _frontend, and _backend while continuing to move core forward (think: pulling out pre-complete-states)

You can see from the db_helper.rb, and the inclusion of standalone_migrations how simple it is to continue to maintain activerecord migrations in core.

This transition will take a while as we move all dependencies to solidus_rails, but should be pretty seamless for anyone currently relying on frontend or backend.

Big TODOs:

Pull mailers out of core
Move the rest of app/models to /lib (and fix requires)
Move Spree::Core::Engine's work into solidus_rails (or continue to deprecate)
Move the few assets in core to solidus_rails (spree.js.erb)

Happy for feedback or to discuss this change here, on slack, or over a hangout if anyone wanted to talk face to face.

mamhoff

I have two minor nits which I don't care whether they're addressed. Great work, this will greatly speed up development!

mamhoff · 2017-09-23T19:45:22Z

solidus_rails/solidus_rails.gemspec

+  s.platform    = Gem::Platform::RUBY
+  s.name        = 'solidus_rails'
+  s.version     = Spree.solidus_version
+  s.summary     = 'Provides the rails glue for the legacy solidus frontend/backend/api'


Do we want to call these legacy just yet? :)

mamhoff · 2017-09-23T19:48:50Z

core/lib/spree.rb

+  #   end
+  #
+  # This method is defined within the core gem on purpose.
+  # Some people may only wish to use the Core part of Spree.


s/Spree/Solidus ?

Was just a move, but updated.

BenMorganIO · 2017-09-27T20:38:53Z

solidus_rails/solidus_rails.gemspec

+
+  s.add_dependency 'solidus_core', s.version
+
+  s.add_dependency 'rails', '~> 5.1.0'


This can be split out to just actionmailer and activerecord.

I will once the deps are actually removed, aye. There are still lots of rails calls inside the codebase (and we still have the engine in there).

jhawthorn · 2017-10-05T20:26:33Z

My initial feeling towards this is 👎

I don't see any benefit to making these changes. Furthermore I expect this will be confusing to users as it is already confusing to me.

Even after all these changes, solidus_core still depends on rails. Making the split pretty ineffective. solidus_core is always going to depend on activerecord, so it seems like a better first step would be reducing the requirements of solidus_core. Since solidus_core will never be without activerecord it might as well continue being an engine. If this is just for spec speed purposes we can just avoid requiring the engine if that's the goal here; there's no need for the complexity of an extra gem.

As for the controller_helpers, it would be simpler for everyone to keep them in solidus_core, but simply not require them by default. I think that would be a good change.

jhawthorn

I don't think this should be done. Let's not approve PRs before they can even build.

cbrunsdon · 2017-10-05T22:58:54Z

Yea, it'll be a fair number of more changes before I can actually remove the rails dep from solidus_core, but without this work that'll never be possible of course.

houndci-bot · 2017-10-18T02:58:10Z

core/spec/rails_helper.rb

-  config.infer_spec_type_from_file_location!
+# set paperclip to interpolate rails_root, since we don't have one
+# sans rails
+Paperclip.interpolates :rails_root do |attachment, style|


Unused block argument - attachment. You can omit all the arguments if you don't care about them.
Unused block argument - style. You can omit all the arguments if you don't care about them.

houndci-bot · 2017-10-23T15:04:00Z

core/app/models/spree/calculator/shipping/flexi_rate.rb

+          max = preferred_max_items.to_i
+          quantity.times do |i|
+            # check max value to avoid divide by 0 errors
+            if (max == 0 && i == 0) || (max > 0) && (i % max == 0)


Use max.zero? instead of max == 0.
Use i.zero? instead of i == 0.
Use (i % max).zero? instead of i % max == 0.

Addressed this rubocop annoyance with #2314

I'll end up pulling this commit out, as I sent it separably as #2312

houndci-bot · 2017-10-23T15:04:00Z

core/app/models/spree/calculator/flexi_rate.rb


-      return BigDecimal.new(0) if items_count == 0
+        return BigDecimal.new(0) if items_count == 0


Use items_count.zero? instead of items_count == 0.

BenMorganIO · 2017-10-23T15:07:34Z

core/script/rails

-ENGINE_PATH = File.expand_path('../../lib/spree/core/engine', __FILE__)
-
-require 'rails/all'
-require 'rails/engine/commands'


We should probably cherry-pick this out into another PR.

Just pulled this out entirely, can be dealt with later

cbrunsdon · 2017-11-01T01:12:51Z

Here is a quick diagram to help illustrate what this PR is working to achieve:

The goal is for solidus_core to provide ecommerce functionality independently from what web framework we're using to deliver the content.

Anecdotally, this removes 20 more gems from the requirements of solidus_core.

More importantly, it lets me use and work with the solidus models without also requiring all of rails, a huge benefit to me when I'm developing stores and libraries locally.

houndci-bot · 2017-11-01T01:55:54Z

solidus_rails/spec/rails_helper.rb

+ActiveJob::Base.queue_adapter = :test
+
+RSpec.configure do |config|
+  config.fixture_path = File.join(File.expand_path(File.dirname(__FILE__)), "fixtures")


Use dir to get an absolute path to the current file's directory.

houndci-bot · 2017-11-01T01:55:54Z

solidus_rails/spec/rails_helper.rb

+begin
+  require File.expand_path("../dummy/config/environment", __FILE__)
+rescue LoadError
+  $stderr.puts "Could not load dummy application. Please ensure you have run `bundle exec rake test_app`"


Use warn instead of $stderr.puts.

houndci-bot · 2017-11-01T01:55:54Z

core/spec/rails_helper.rb

-  config.use_transactional_fixtures = true
+# set paperclip to interpolate rails_root, since we don't have one
+# sans rails
+Paperclip.interpolates :rails_root do |attachment, style|


Unused block argument - attachment. You can omit all the arguments if you don't care about them.
Unused block argument - style. You can omit all the arguments if you don't care about them.

BenMorganIO · 2017-11-01T15:36:31Z

@cbrunsdon oh man, looking at that, I feel like Solidus Core should be ActiveSolidus and Solidus Rails should be ActionSolidus XD

We use mattr_accessor, defined in active support here, so we may as well declare that we use it.

It doesn't take much for us to require our dependencies, so lets do it.

A small change to add more explicit requires in the stack

This is a transitionary step to remove rails_helper entirely, but it sets up the test env for rails-like settings, without needing to run test_app or boot an unused rails app.

As these are rendering full messages, they need a view path in order to render a mailer, so we need more to set up for testing.

Our models use activejob, which requires a globalid setup in order to create jobs

This makes our runs look more like rails defaults, sets default settings for test.

We don't have rails root set as we're not running a full rails app, so we'll need to set it ourselves.

These tests are heavily dependent on the default config getting set up in the rails initializer.

really_destroy! does weird things, this should maybe still be flagged as persisted? For sure, though, trying to reload should cause an activerecord not found.

We don't have much work to do for test, app, just need to run the migrations. We are re-using test_app for compatability, but might want to change this in the future.

Need to test the code I'm pushing out of core.

As core no longer runs (most) of rails, we can pull these out now and just add them to the rails projects

None of this is needed/used by core, but is required by our rails apps, so pushing it into solidus_rails.

As all these sub-gems require solidus_rails, we load it early.

We no longer run test_app, so there isn't a clearly great place to run migrations, but this will do for now.

jhawthorn · 2017-11-09T22:23:48Z

core/lib/spree.rb

@@ -0,0 +1,34 @@
+require 'active_support/core_ext/module/attribute_accessors'


We can't name a file spree.rb, because of the other file of the same name. Even with the require_relative it has potential to cause issues.

Hmmm, I like the idea of a spree.rb defining the Spree module. (independent of the rest of these changes). Can we look at killing the other one?

I am not a big fan of the "solidus" gem itself doing much.

That file doesn't do much, but we should definitely deprecate it before removing it, various extensions and likely stores, which have been upgraded from spree, are expecting that require to require all of Solidus.

I am not super worried about deleting the original without deprecating as I don't think it would create any runtime errors in stores, and could be caught and fixed very easily by anyone reading the changelog.

But now that i sit down and think about it, I'm not 100% sure how/when solidus is usually required into a project.

Anecdotally I grep'd my code folder and we don't have any projects, either inherited or greenfield that do a require 'spree'.

Yea, I just checked and it looks like we usually get solidus required as part of:
Bundler.require(*Rails.groups) in config/application.rb's.

I doubt anyone is going to get caught by this, even if they are an old spree upgrade.

kennyadsl · 2020-05-22T14:27:23Z

Closing this one since there seems to be no one anymore interested and it's a big change that requires some ownership at least.

mamhoff approved these changes Sep 23, 2017

View reviewed changes

tvdeyen mentioned this pull request Sep 27, 2017

[core] remove rails and use only actionmailer and activerecord #2239

Closed

BenMorganIO reviewed Sep 27, 2017

View reviewed changes

jhawthorn suggested changes Oct 5, 2017

View reviewed changes

BenMorganIO mentioned this pull request Oct 5, 2017

Make solidus_core depend on actionmailer and activerecord instead of rails #2272

Merged

cbrunsdon force-pushed the solidus_rails branch from dcf85f4 to 0a1011c Compare October 18, 2017 02:56

houndci-bot reviewed Oct 18, 2017

View reviewed changes

cbrunsdon force-pushed the solidus_rails branch 11 times, most recently from 31b56f4 to afba33f Compare October 23, 2017 06:39

houndci-bot reviewed Oct 23, 2017

View reviewed changes

BenMorganIO reviewed Oct 23, 2017

View reviewed changes

cbrunsdon force-pushed the solidus_rails branch from ee54392 to 9ce513c Compare November 1, 2017 01:54

houndci-bot reviewed Nov 1, 2017

View reviewed changes

cbrunsdon force-pushed the solidus_rails branch 4 times, most recently from 10c9519 to f2877a1 Compare November 1, 2017 04:10

Clarke Brunsdon added 22 commits November 7, 2017 19:01

Explicitely require activesupport/accessors

7c2c549

We use mattr_accessor, defined in active support here, so we may as well declare that we use it.

Explicitely require paranoia/acts_as_list

1d3d0bd

It doesn't take much for us to require our dependencies, so lets do it.

Require variants in product

d0b2588

A small change to add more explicit requires in the stack

Remove rails init from rails_helper

87bd23b

This is a transitionary step to remove rails_helper entirely, but it sets up the test env for rails-like settings, without needing to run test_app or boot an unused rails app.

Require mailer_helper for mailer specs

e91e74d

As these are rendering full messages, they need a view path in order to render a mailer, so we need more to set up for testing.

Add a logger to activerecord

d3291bd

Add globalid to the db setup

c31ec6e

Our models use activejob, which requires a globalid setup in order to create jobs

Throw in some more "rails defaults" for test env

6b31af1

This makes our runs look more like rails defaults, sets default settings for test.

Interpolate rails_root in our spec helper

fb615c9

We don't have rails root set as we're not running a full rails app, so we'll need to set it ourselves.

Set up default roles in ability spec

628561d

These tests are heavily dependent on the default config getting set up in the rails initializer.

Reload error instead of checking persisted?

9f697a0

really_destroy! does weird things, this should maybe still be flagged as persisted? For sure, though, trying to reload should cause an activerecord not found.

FXUP: into improve rails helper

24424c7

Use mailer helper in specs that send mail

6b06090

Use standalone_migrations in our test_app

ff279be

We don't have much work to do for test, app, just need to run the migrations. We are re-using test_app for compatability, but might want to change this in the future.

Include new solidus_rails gem on CI build

ced7bd3

Need to test the code I'm pushing out of core.

Move rails deps into common_rails_dependencies

9e08306

As core no longer runs (most) of rails, we can pull these out now and just add them to the rails projects

Move assets into solidus_rails

e3b79c7

None of this is needed/used by core, but is required by our rails apps, so pushing it into solidus_rails.

Require solidus_rails in gems that use it

b6e6378

As all these sub-gems require solidus_rails, we load it early.

Require the 'spree' model when we boot up core

7fb65bd

Require rails for solidus_rails engine

80c15dc

Set up database on circleci

0cc612b

We no longer run test_app, so there isn't a clearly great place to run migrations, but this will do for now.

Update solidus_rails for test_app changes

863db95

cbrunsdon force-pushed the solidus_rails branch from 084f4a4 to 863db95 Compare November 8, 2017 03:03

Clarke Brunsdon added 2 commits November 7, 2017 19:35

Don't reset preferences

67e0b44

Remove ffaker from solidus_rails

6494ad4

jhawthorn reviewed Nov 9, 2017

View reviewed changes

tvdeyen added this to the 3.0 milestone Jun 13, 2018

kennyadsl mentioned this pull request Dec 19, 2018

Fix autoload issue. Replace require/load with require_dependency. #3008

Merged

kennyadsl closed this May 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Pull rails dependency out of core into solidus_rails #2236

[RFC] Pull rails dependency out of core into solidus_rails #2236

cbrunsdon commented Sep 22, 2017

mamhoff left a comment

mamhoff Sep 23, 2017

cbrunsdon Nov 1, 2017

mamhoff Sep 23, 2017

cbrunsdon Nov 1, 2017

BenMorganIO Sep 27, 2017

cbrunsdon Sep 28, 2017

jhawthorn commented Oct 5, 2017

jhawthorn left a comment

cbrunsdon commented Oct 5, 2017

houndci-bot Oct 18, 2017

houndci-bot Oct 23, 2017

tvdeyen Oct 23, 2017

cbrunsdon Oct 23, 2017

houndci-bot Oct 23, 2017

BenMorganIO Oct 23, 2017

cbrunsdon Nov 1, 2017

cbrunsdon commented Nov 1, 2017

houndci-bot Nov 1, 2017

houndci-bot Nov 1, 2017

houndci-bot Nov 1, 2017

BenMorganIO commented Nov 1, 2017

jhawthorn Nov 9, 2017 •

edited

Loading

cbrunsdon Nov 9, 2017

jhawthorn Nov 9, 2017

cbrunsdon Nov 9, 2017

cbrunsdon Nov 9, 2017

kennyadsl commented May 22, 2020


		s.add_dependency 'solidus_core', s.version

		s.add_dependency 'rails', '~> 5.1.0'


		return BigDecimal.new(0) if items_count == 0
		return BigDecimal.new(0) if items_count == 0

		@@ -0,0 +1,34 @@
		require 'active_support/core_ext/module/attribute_accessors'

[RFC] Pull rails dependency out of core into solidus_rails #2236

[RFC] Pull rails dependency out of core into solidus_rails #2236

Conversation

cbrunsdon commented Sep 22, 2017

mamhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhawthorn commented Oct 5, 2017

jhawthorn left a comment

Choose a reason for hiding this comment

cbrunsdon commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbrunsdon commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenMorganIO commented Nov 1, 2017

jhawthorn Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented May 22, 2020

jhawthorn Nov 9, 2017 •

edited

Loading