Only require the necessary Rails frameworks

[elia]

Description

By requiring rails/all Solidus was loading a bunch of unnecessary code
taking more time and consuming more memory. This change allows apps
that want to cherry-pick Rails frameworks to benefit from the more
attentive choice.

The list of frameworks I enabled for each component of Solidus comes from searching the subdir (e.g. core/) with this regular expression: /\b(Action|Active|Application)[A-Z]/.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[kennyadsl]

Hey @elia, this is interesting. A couple of things:

1. I'd remove the commented lines: I see some value in them but they could change across different Rails versions and we may not be able to maintain the list of comments in the future.
2. How do we know it's working? I think the dummy app requires rails using the Gemfile so it would be interesting to know how you tested it in a real application.

[elia]

@kennyadsl I've updated the code to remove the comments and include some specs.

There's a remote risk that another spec in the full suite is loading one of the optional Rails frameworks breaking it if it's executed first. The only truly safe alternative would be to always run those specs in isolation, but that seems overkill given it's unlikely that any specs will need to require "rails/all" in order to function.

[filippoliverani]

I'm rebasing and updating the PR with @elia's blessing ✋

[spaghetticode]

@elia @filippoliverani thank you 👍

[tvdeyen]

Very nice

[kennyadsl]

@elia @filippoliverani

There's a remote risk that another spec in the full suite is loading one of the optional Rails frameworks breaking it if it's executed first. The only truly safe alternative would be to always run those specs in isolation, but that seems overkill given it's unlikely that any specs will need to require "rails/all" in order to function.

What about attempting to run all spec files individually with something like this?

find spec -name '*_spec.rb' -exec bin/rspec {} \;

It will take forever ⌛ but I think it could give us some peace of mind.

[elia]

@kennyadsl makes sense to me! And I guess you man just locally, not as part of the CI right?

(I'll try it and keep you posted on the results)

[elia]

@kennyadsl @filippoliverani got just one failure:

$ cd core; bundle exec rspec /Users/elia/Code/Nebulab/solidus/core/spec/lib/spree/event/subscriber_spec.rb; cd -
Run options: include {:focus=>true}

All examples were filtered out; ignoring {:focus=>true}

Randomized with seed 49674
FFFFFFF

Failures:

  1) Spree::Event::Subscriber::unsubscribe removes the subscription
     Failure/Error: ActiveSupport::Notifications.unsubscribe(subscriber_or_name)
     
     NoMethodError:
       undefined method `try' for nil:NilClass
     # ./lib/spree/event/adapters/active_support_notifications.rb:25:in `unsubscribe'
     # ./lib/spree/event.rb:96:in `unsubscribe'
     # ./lib/spree/event/subscriber.rb:81:in `block in unsubscribe!'
     # ./lib/spree/event/subscriber.rb:80:in `each'
     # ./lib/spree/event/subscriber.rb:80:in `unsubscribe!'
     # ./lib/spree/event/subscriber.rb:67:in `subscribe!'
     # ./spec/lib/spree/event/subscriber_spec.rb:49:in `block (3 levels) in <top (required)>'

  2) Spree::Event::Subscriber::subscribe! does not subscribe event actions more than once
     Failure/Error: ActiveSupport::Notifications.unsubscribe(subscriber_or_name)
     
     NoMethodError:
       undefined method `try' for nil:NilClass
     # ./lib/spree/event/adapters/active_support_notifications.rb:25:in `unsubscribe'
     # ./lib/spree/event.rb:96:in `unsubscribe'
     # ./lib/spree/event/subscriber.rb:81:in `block in unsubscribe!'
     # ./lib/spree/event/subscriber.rb:80:in `each'
     # ./lib/spree/event/subscriber.rb:80:in `unsubscribe!'
     # ./spec/lib/spree/event/subscriber_spec.rb:22:in `block (3 levels) in <top (required)>'

  3) Spree::Event::Subscriber::subscribe! adds new listeners to Spree::Event
     Failure/Error: ActiveSupport::Notifications.unsubscribe(subscriber_or_name)
     
     NoMethodError:
       undefined method `try' for nil:NilClass
     # ./lib/spree/event/adapters/active_support_notifications.rb:25:in `unsubscribe'
     # ./lib/spree/event.rb:96:in `unsubscribe'
     # ./lib/spree/event/subscriber.rb:81:in `block in unsubscribe!'
     # ./lib/spree/event/subscriber.rb:80:in `each'
     # ./lib/spree/event/subscriber.rb:80:in `unsubscribe!'
     # ./spec/lib/spree/event/subscriber_spec.rb:22:in `block (3 levels) in <top (required)>'

  4) Spree::Event::Subscriber::subscribe! subscribes event actions
     Failure/Error: ActiveSupport::Notifications.unsubscribe(subscriber_or_name)
     
     NoMethodError:
       undefined method `try' for nil:NilClass
     # ./lib/spree/event/adapters/active_support_notifications.rb:25:in `unsubscribe'
     # ./lib/spree/event.rb:96:in `unsubscribe'
     # ./lib/spree/event/subscriber.rb:81:in `block in unsubscribe!'
     # ./lib/spree/event/subscriber.rb:80:in `each'
     # ./lib/spree/event/subscriber.rb:80:in `unsubscribe!'
     # ./spec/lib/spree/event/subscriber_spec.rb:22:in `block (3 levels) in <top (required)>'

  5) Spree::Event::Subscriber::subscribe! when subscriptions are not registered does not trigger the event callback
     Failure/Error: ActiveSupport::Notifications.unsubscribe(subscriber_or_name)
     
     NoMethodError:
       undefined method `try' for nil:NilClass
     # ./lib/spree/event/adapters/active_support_notifications.rb:25:in `unsubscribe'
     # ./lib/spree/event.rb:96:in `unsubscribe'
     # ./lib/spree/event/subscriber.rb:81:in `block in unsubscribe!'
     # ./lib/spree/event/subscriber.rb:80:in `each'
     # ./lib/spree/event/subscriber.rb:80:in `unsubscribe!'
     # ./spec/lib/spree/event/subscriber_spec.rb:22:in `block (3 levels) in <top (required)>'

  6) Spree::Event::Subscriber::event_action when the action is declared subscribe the action
     Got 0 failures and 2 other errors:

     6.1) Failure/Error: ActiveSupport::Notifications.unsubscribe(subscriber_or_name)
          
          NoMethodError:
            undefined method `try' for nil:NilClass
          # ./lib/spree/event/adapters/active_support_notifications.rb:25:in `unsubscribe'
          # ./lib/spree/event.rb:96:in `unsubscribe'
          # ./lib/spree/event/subscriber.rb:81:in `block in unsubscribe!'
          # ./lib/spree/event/subscriber.rb:80:in `each'
          # ./lib/spree/event/subscriber.rb:80:in `unsubscribe!'
          # ./lib/spree/event/subscriber.rb:67:in `subscribe!'
          # ./spec/lib/spree/event/subscriber_spec.rb:71:in `block (4 levels) in <top (required)>'

     6.2) Failure/Error: ActiveSupport::Notifications.unsubscribe(subscriber_or_name)
          
          NoMethodError:
            undefined method `try' for nil:NilClass
          # ./lib/spree/event/adapters/active_support_notifications.rb:25:in `unsubscribe'
          # ./lib/spree/event.rb:96:in `unsubscribe'
          # ./lib/spree/event/subscriber.rb:81:in `block in unsubscribe!'
          # ./lib/spree/event/subscriber.rb:80:in `each'
          # ./lib/spree/event/subscriber.rb:80:in `unsubscribe!'
          # ./spec/lib/spree/event/subscriber_spec.rb:75:in `block (4 levels) in <top (required)>'

  7) Spree::Event::Subscriber::event_action when the action has not been declared does not subscribe the action
     Failure/Error: ActiveSupport::Notifications.unsubscribe(subscriber_or_name)
     
     NoMethodError:
       undefined method `try' for nil:NilClass
     # ./lib/spree/event/adapters/active_support_notifications.rb:25:in `unsubscribe'
     # ./lib/spree/event.rb:96:in `unsubscribe'
     # ./lib/spree/event/subscriber.rb:81:in `block in unsubscribe!'
     # ./lib/spree/event/subscriber.rb:80:in `each'
     # ./lib/spree/event/subscriber.rb:80:in `unsubscribe!'
     # ./lib/spree/event/subscriber.rb:67:in `subscribe!'
     # ./spec/lib/spree/event/subscriber_spec.rb:60:in `block (4 levels) in <top (required)>'

Finished in 0.00208 seconds (files took 0.60128 seconds to load)
7 examples, 7 failures

Failed examples:

rspec ./spec/lib/spree/event/subscriber_spec.rb:51 # Spree::Event::Subscriber::unsubscribe removes the subscription
rspec ./spec/lib/spree/event/subscriber_spec.rb:41 # Spree::Event::Subscriber::subscribe! does not subscribe event actions more than once
rspec ./spec/lib/spree/event/subscriber_spec.rb:24 # Spree::Event::Subscriber::subscribe! adds new listeners to Spree::Event
rspec ./spec/lib/spree/event/subscriber_spec.rb:35 # Spree::Event::Subscriber::subscribe! subscribes event actions
rspec ./spec/lib/spree/event/subscriber_spec.rb:29 # Spree::Event::Subscriber::subscribe! when subscriptions are not registered does not trigger the event callback
rspec ./spec/lib/spree/event/subscriber_spec.rb:79 # Spree::Event::Subscriber::event_action when the action is declared subscribe the action
rspec ./spec/lib/spree/event/subscriber_spec.rb:62 # Spree::Event::Subscriber::event_action when the action has not been declared does not subscribe the action

Randomized with seed 49674

Here's the full log https://gist.githubusercontent.com/elia/78da423cb3ca778045d06da773aecdff/raw/a448024083a37b8964b03215e3cdcc8655e99d08/gistfile1.txt

[filippoliverani]

@kennyadsl @filippoliverani got just one failure:

This is due to a missing require in the spec, it only requires:

require 'spec_helper'

but relies on ActiveSupport stuff. I'm pushing the updated requires

[kennyadsl]

Thanks @elia and @filippoliverani!