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

--skip generator options reduce included gems #24997

Closed
wants to merge 1 commit into from

Conversation

claudiob
Copy link
Member

(Note: this PR stems from a Basecamp chat where @rafaelfranca said "yes, I'd love to --skip-* change the gemfile instead of not loading the railtie", @SamSaffron expressed some concerns and @matthewd suggested a possible implementation. This PR needs a CHANGELOG and a deprecation policy… but I'd like to know if the team agrees with the concept behind this PR before going any further)


Before this commit, every generated Rails app always included all the component gems of Rails (activerecord, actionmailer, etc.), whether or not those components were explicitly skipped (e.g.: with --skip-active-record).

After this commit, using --skip-active-record, --skip-action-mailer or --skip-action-cable when generating a Rails app/plugin also affects the Gemfile.

Rather than having a single, "include-all" gem rails line, the Gemfile will have the specific Rails component listed and commented out if specified.

For instance, rails new app_name --skip-active-record will generate a Gemfile that looks like this:

# gem 'rails', '>= 5.1.0.alpha', '< 5.2'
gem 'actioncable', '>= 5.1.0.alpha', '< 5.2'
gem 'actionmailer', '>= 5.1.0.alpha', '< 5.2'
gem 'activejob', '>= 5.1.0.alpha', '< 5.2'
gem 'activemodel', '>= 5.1.0.alpha', '< 5.2'
# gem 'activerecord', '>= 5.1.0.alpha', '< 5.2'
gem 'railties', '>= 5.1.0.alpha', '< 5.2'

As a result, the activerecord gem and its dependencies won't be bundled.


As of 6007e58, the number of gems bundled by rails new is 61.
The benefit of this commit is that the number of bundled gems is reduced to:

  • 57 when running --skip-active-record (arel and sqlite3 are not installed)
  • 56 when running --skip-action-cable (nio4r, websocket-driver and websocket-extensions are not installed)
  • 56 when running --skip-action-mailer (mail, mime-types and mime-types-data are not installed)

This PR honors the spirit of Rails as a "modular framework" that allows users to cherry-pick the components that are required for each application. Removing unnecessary gems keeps the size of the application small and reduces the number of possible third-party issues.

Before this commit, every generated Rails app always included all the component
gems of Rails (`activerecord`, `actionmailer`, etc.), whether or not those
components were explicitly skipped (e.g.: with `--skip-active-record`).

After this commit, using `--skip-active-record`, `--skip-action-mailer` or
`--skip-action-cable` when generating a Rails app/plugin also has an effect
on the Gemfile.

Rather than having a single, "include-all" `gem rails` line, the Gemfile will
have the specific Rails component listed and commented out if specified.

For instance, `rails new app_name --skip-active-record` will generate a Gemfile
that looks like this:

> # gem 'rails', '>= 5.1.0.alpha', '< 5.2'
> gem 'actioncable', '>= 5.1.0.alpha', '< 5.2'
> gem 'actionmailer', '>= 5.1.0.alpha', '< 5.2'
> gem 'activejob', '>= 5.1.0.alpha', '< 5.2'
> gem 'activemodel', '>= 5.1.0.alpha', '< 5.2'
> # gem 'activerecord', '>= 5.1.0.alpha', '< 5.2'
> gem 'railties', '>= 5.1.0.alpha', '< 5.2'

As a result, the `activerecord` gem and its dependencies won't be bundled.
@rafaelfranca
Copy link
Member

@dhh are you fine with this? I proposed the same change in #5703 and you had some concerns on that confusing our users and making harder to add gems back.

@dhh
Copy link
Member

dhh commented May 12, 2016

This works for me. If you're calling skip yourself, you're already off the beaten path. Compass and backpack required!

@rafaelfranca rafaelfranca self-assigned this May 12, 2016
@claudiob
Copy link
Member Author

Thanks! Then I'll work on improving the code itself.

When talking with @matthewd about this at RailsConf, we were wondering whether this change would need a deprecation policy.

It's a little "meta" because we are changing the generator, but theoretically anyone using --skip-active-record was expecting the Gemfile to include gem 'rails', and that won't be the case after this commit.

So in case gems depend on the Rails generator for other behavior, this would be a breaking change.

Another option is to add separate options, for instance --skip-active-record would maintain the current behavior and --really-really-skip-active-record 😉 would implement the current PR.

@claudiob
Copy link
Member Author

On a separate note (and eventually in a separate PR), would you be okay if I changed the API interface of the GemfileEntry class? Currently, it's defined as

class GemfileEntry < Struct.new(:name, :version, :comment, :options, :commented_out)

and its methods accept parameters like:

def self.github(name, github, branch = nil, comment = nil)

and the resulting code is verbose and full of duplication (see my code change in this PR).

Maybe we could start using optional parameters, and the entire class would be more readable.

Let me know… it's a separate conversation and it's another possible breaking change, so I would understand if we want to keep it as it is.

@dhh
Copy link
Member

dhh commented May 12, 2016

No deprecation needed since it only affects new applications.

On Thu, May 12, 2016 at 6:28 PM, Claudio B. notifications@github.com
wrote:

On a separate note (and eventually in a separate PR), would you be okay if
I changed the API interface of the GemfileEntry class? Currently, it's
defined as

class GemfileEntry < Struct.new(:name, :version, :comment, :options, :commented_out)

and its methods accept parameters like:

def self.github(name, github, branch = nil, comment = nil)

and the resulting code is verbose and full of duplication (see my code
change in this PR).

Maybe we could start using optional parameters, and the entire class would
be more readable.

Let me know… it's a separate conversation and it's another possible
breaking change, so I would understand if we want to keep it as it is.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#24997 (comment)

@matthewd
Copy link
Member

My concern wasn't about deprecation, but the fact that bundle update rails would no longer Do The Thing.

@claudiob
Copy link
Member Author

@matthewd oops, sorry, I forgot! 😅

You are right, bundle update rails wouldn't work but

If you're calling skip yourself, you're already off the beaten path.

If you generate your own app skipping some gems, it will be your responsibility to bundle update it appropriately. It seems fair.

@dhh
Copy link
Member

dhh commented May 12, 2016

I think if you're off in custom land, then that's the choice you make.

On Thu, May 12, 2016 at 6:45 PM, Matthew Draper notifications@github.com
wrote:

My concern wasn't about deprecation, but the fact that bundle update rails
would no longer Do The Thing.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#24997 (comment)

@prathamesh-sonpatki
Copy link
Member

@claudiob Will rails:update task will run properly after this? I don't mean it will work as default app but it should not throw an error.

@claudiob
Copy link
Member Author

@prathamesh-sonpatki I ran bin/rails app:update and I found something weird that is unrelated to this PR… might be a separate issue.

In short, if you create a Rails app with --skip-action-mailer (even without this PR), then your config/application.rb will look like this:

require "rails"
# Pick the frameworks you want:
require "active_model/railtie"
require "active_job/railtie"
require "active_record/railtie"
require "action_controller/railtie"
# require "action_mailer/railtie"
require "action_view/railtie"
require "action_cable/engine"
require "sprockets/railtie"
require "rails/test_unit/railtie"

but then, if you run bin/rails app:update, your config/application.rb will look like this:

require 'rails/all'

In short, it looks like rails:update is already not respecting the fact that you might have skipped some Rails components when generating the app. Whether this is intended or a bug, it's unrelated to this PR (and we can continue the discussion in a separate issue).

@prathamesh-sonpatki
Copy link
Member

@claudiob I remember now :)

We had some discussion about it in #22790 but could not come to a decision. I think we should be fine in that case.

@maclover7
Copy link
Contributor

I'm 👍 for having skip-* not force you to require gems, but I think this drives users too far off the beaten path. As @matthewd already mentioned, bundle update rails will no longer work. Also, I think this breaks the natural flow of having just a single gem 'rails' entry in the Gemfile.

Side note: I remember @sgrif talking (on 🚲 🏠) about having optional dependencies with Rubygems be a thing -- that would be super helpful here 😞

@SamSaffron
Copy link
Contributor

I feel like this is the wrong place to make the change, essentially rubygems and bundler need the change first. In particular, gemspec should allow for optional opt-in and optional opt-out dependencies, these issues are pretty huge in our ecosystem.

For example, active_record has optional opt-in dependencies on particular versions of the pg gem. This dependency can not be codified in the gemfile.

Similarly the rails meta gem has optional opt-out dependencies on various bits that make up rails.

I feel that within the current constraints this change does not really help much... what I want is this in my gemfile.

gem 'rails', without: ['action_cable']

That way, bundle update rails continues to work and so on.

cc @indirect

@claudiob
Copy link
Member Author

@SamSaffron That would be great! I agree with everything you said 😁

@nateberkopec
Copy link
Contributor

+1 for closing this. The benefits (reducing gem install count by ~10%) don't outweigh the costs of making maintaining a modular Rails application more difficult (breaking bundle update rails).

I have a number of modular Rails apps inside of test suites (for testing how a gem works with Rails, etc), and I always use gem 'rails', then require the needed railties.

@claudiob claudiob closed this Jun 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants