Skip to content

Commit

Permalink
Fix asset pipeline errors for plugin dummy apps
Browse files Browse the repository at this point in the history 
To fix #43920, f292daa added
`sprockets-rails` to the generated `Gemfile` for engine plugins because
their dummy apps use Sprockets.  However, non-engine plugins exhibit the
same issue because their dummy apps also use Sprockets.

This commit forces `skip_asset_pipeline` to be true when a plugin is not
an engine, and fixes several tests that failed to detect these issues
because they were accidentally using the `rails/rails` `Gemfile` instead
of the generated plugin `Gemfile`.

This commit also refactors the plugin `Gemfile.tt` to abstract much of
the logic into the `gemfile_entries` helper.  Doing so also avoids
adding application-only `Gemfile` entries (e.g. `puma`) when generating
a plugin with a prerelease flag (e.g. `--dev`).
  • Loading branch information
@jonathanhefner
jonathanhefner committed Dec 28, 2021
1 parent 94c28ac commit cc3594a
Show file tree
Hide file tree
Showing 9 changed files with 181 additions and 146 deletions.
61 changes: 28 additions & 33 deletions railties/lib/rails/generators/app_base.rb
View file
Expand Up @@ -108,16 +108,18 @@ def initialize(positional_argv, option_argv, *)

private
def gemfile_entries # :doc:
[rails_gemfile_entry,
asset_pipeline_gemfile_entry,
database_gemfile_entry,
web_server_gemfile_entry,
javascript_gemfile_entry,
hotwire_gemfile_entry,
css_gemfile_entry,
jbuilder_gemfile_entry,
psych_gemfile_entry,
cable_gemfile_entry].flatten.find_all(&@gem_filter)
[
rails_gemfile_entry,
asset_pipeline_gemfile_entry,
database_gemfile_entry,
web_server_gemfile_entry,
javascript_gemfile_entry,
hotwire_gemfile_entry,
css_gemfile_entry,
jbuilder_gemfile_entry,
psych_gemfile_entry,
cable_gemfile_entry,
].flatten.compact.select(&@gem_filter)
end

def builder # :doc:
Expand Down Expand Up @@ -159,7 +161,8 @@ def set_default_accessors! # :doc:
end

def database_gemfile_entry # :doc:
return [] if options[:skip_active_record]
return if options[:skip_active_record]

gem_name, gem_version = gem_for_database
GemfileEntry.version gem_name, gem_version,
"Use #{options[:database]} as the database for Active Record"
Expand Down Expand Up @@ -262,20 +265,13 @@ def self.path(name, path, comment = nil)
new(name, nil, comment, path: path)
end

def version
version = super

if version.is_a?(Array)
version.join('", "')
else
version
end
end

def to_s
[ ("# #{comment}\n" if comment),
("# " if commented_out), "gem \"#{name}\"", (", \"#{version}\"" if version),
*options.map { |key, val| ", #{key}: #{val.inspect}" }
[
(comment.gsub(/^/, "# ").chomp + "\n" if comment),
("# " if commented_out),
"gem \"#{name}\"",
*Array(version).map { |constraint| ", \"#{constraint}\"" },
*options.map { |key, value| ", #{key}: #{value.inspect}" },
].compact.join
end
end
Expand Down Expand Up @@ -312,12 +308,12 @@ def rails_version_specifier(gem_version = Rails.gem_version)
end

def jbuilder_gemfile_entry
return [] if options[:skip_jbuilder]
return if options[:skip_jbuilder]
GemfileEntry.new "jbuilder", nil, "Build JSON APIs with ease [https://github.com/rails/jbuilder]", {}, options[:api]
end

def javascript_gemfile_entry
return [] if options[:skip_javascript]
return if options[:skip_javascript]

if adjusted_javascript_option == "importmap"
GemfileEntry.floats "importmap-rails", "Use JavaScript with ESM import maps [https://github.com/rails/importmap-rails]"
Expand All @@ -327,7 +323,7 @@ def javascript_gemfile_entry
end

def hotwire_gemfile_entry
return [] if options[:skip_javascript] || options[:skip_hotwire]
return if options[:skip_javascript] || options[:skip_hotwire]

turbo_rails_entry =
GemfileEntry.floats "turbo-rails", "Hotwire's SPA-like page accelerator [https://turbo.hotwired.dev]"
Expand All @@ -353,7 +349,7 @@ def adjusted_javascript_option
end

def css_gemfile_entry
return [] unless options[:css]
return unless options[:css]

if !using_node? && options[:css] == "tailwind"
GemfileEntry.floats "tailwindcss-rails", "Use Tailwind CSS [https://github.com/rails/tailwindcss-rails]"
Expand All @@ -363,19 +359,18 @@ def css_gemfile_entry
end

def psych_gemfile_entry
return [] unless defined?(Rubinius)
return unless defined?(Rubinius)

comment = "Use Psych as the YAML engine, instead of Syck, so serialized " \
"data can be read safely from different rubies (see http://git.io/uuLVag)"
GemfileEntry.new("psych", "~> 2.0", comment, platforms: :rbx)
end

def cable_gemfile_entry
return [] if options[:skip_action_cable]
return if options[:skip_action_cable]

comment = "Use Redis adapter to run Action Cable in production"
gems = []
gems << GemfileEntry.new("redis", "~> 4.0", comment, {}, true)
gems
GemfileEntry.new("redis", "~> 4.0", comment, {}, true)
end

def bundle_command(command, env = {})
Expand Down
31 changes: 31 additions & 0 deletions railties/lib/rails/generators/rails/plugin/plugin_generator.rb
View file
Expand Up @@ -226,6 +226,10 @@ class PluginGenerator < AppBase # :nodoc:
def initialize(*args)
@dummy_path = nil
super

if !engine? || !with_dummy_app?
self.options = options.merge(skip_asset_pipeline: true).freeze
end
end

public_task :set_default_accessors!
Expand Down Expand Up @@ -309,6 +313,33 @@ def namespaced_name
end

private
def gemfile_entries
[
rails_gemfile_entry,
simplify_gemfile_entries(
database_gemfile_entry,
asset_pipeline_gemfile_entry,
),
].flatten.compact
end

def rails_gemfile_entry
if options[:skip_gemspec]
super
elsif rails_prerelease?
super.dup.tap do |entry|
entry.comment = <<~COMMENT
Your gem is dependent on a prerelease version of Rails. Once you can lock this
dependency down to a specific version, move it to your gemspec.
COMMENT
end
end
end

def simplify_gemfile_entries(*gemfile_entries)
gemfile_entries.flatten.compact.map { |entry| GemfileEntry.floats(entry.name) }
end

def create_dummy_app(path = nil)
dummy_path(path) if path

Expand Down
31 changes: 7 additions & 24 deletions railties/lib/rails/generators/rails/plugin/templates/Gemfile.tt
View file
@@ -1,36 +1,19 @@
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
<% unless options[:skip_gemspec] -%>

<% if options[:skip_gemspec] -%>
<%= "# " if rails_prerelease? -%>gem "rails", "<%= Array(rails_version_specifier).join("', '") %>"
<% else -%>
# Specify your gem's dependencies in <%= name %>.gemspec.
gemspec
<% end -%>
<% unless options[:skip_active_record] -%>

group :development do
gem "<%= gem_for_database[0] %>"
end
<% end -%>

<% if engine? && !skip_sprockets? -%>
<%= asset_pipeline_gemfile_entry %>

<% end -%>
<% if rails_prerelease? -%>
# Your gem is dependent on a prerelease version of Rails. Once you can lock this
# dependency down to a specific version, move it to your gemspec.
<% gemfile_entries.each do |gemfile_entry| -%>
<% gemfile_entries.each do |gemfile_entry| %>
<%= gemfile_entry %>
<% end -%>

<% end -%>
<% if RUBY_ENGINE == "ruby" -%>
# Start debugger with binding.b -- Read more: https://github.com/ruby/debug
# gem "debug", ">= 1.0.0", group: %i[ development test ]

# Start debugger with binding.b [https://github.com/ruby/debug]
# gem "debug", ">= 1.0.0"
<% end -%>
<% if RUBY_PLATFORM.match(/bccwin|cygwin|emx|mingw|mswin|wince|java/) -%>
<% if RUBY_PLATFORM.match?(/bccwin|cygwin|emx|mingw|mswin|wince|java/) -%>

gem "tzinfo-data", platforms: %i[ mingw mswin x64_mingw jruby]
gem "tzinfo-data", platforms: %i[ mingw mswin x64_mingw jruby ]
<% end -%>
39 changes: 12 additions & 27 deletions railties/test/generators/app_generator_test.rb
View file
Expand Up @@ -944,26 +944,25 @@ def test_master_key_is_only_readable_by_the_owner
end

def test_minimal_rails_app
app_root = File.join(destination_root, "myapp")
run_generator [app_root, "--minimal"]
run_generator [destination_root, "--minimal"]

assert_no_file "#{app_root}/config/storage.yml"
assert_no_file "#{app_root}/config/cable.yml"
assert_no_file "#{app_root}/views/layouts/mailer.html.erb"
assert_no_file "#{app_root}/app/jobs/application.rb"
assert_file "#{app_root}/app/views/layouts/application.html.erb" do |content|
assert_no_file "config/storage.yml"
assert_no_file "config/cable.yml"
assert_no_file "views/layouts/mailer.html.erb"
assert_no_file "app/jobs/application.rb"
assert_file "app/views/layouts/application.html.erb" do |content|
assert_no_match(/data-turbo-track/, content)
end
assert_file "#{app_root}/config/environments/development.rb" do |content|
assert_file "config/environments/development.rb" do |content|
assert_no_match(/config\.active_storage/, content)
end
assert_file "#{app_root}/config/environments/production.rb" do |content|
assert_file "config/environments/production.rb" do |content|
assert_no_match(/config\.active_job/, content)
end
assert_file "#{app_root}/config/boot.rb" do |content|
assert_file "config/boot.rb" do |content|
assert_no_match(/require "bootsnap\/setup"/, content)
end
assert_file "#{app_root}/config/application.rb" do |content|
assert_file "config/application.rb" do |content|
assert_match(/#\s+require\s+["']active_job\/railtie["']/, content)
assert_match(/#\s+require\s+["']active_storage\/engine["']/, content)
assert_match(/#\s+require\s+["']action_mailer\/railtie["']/, content)
Expand All @@ -972,8 +971,8 @@ def test_minimal_rails_app
assert_match(/#\s+require\s+["']action_cable\/engine["']/, content)
end

assert_no_gem "jbuilder", app_root
assert_no_gem "web-console", app_root
assert_no_gem "jbuilder"
assert_no_gem "web-console"
end

private
Expand All @@ -985,18 +984,4 @@ def stub_rails_application(root = destination_root, &block)
def action(*args, &block)
capture(:stdout) { generator.send(*args, &block) }
end

def assert_gem(gem, constraint = nil, app_path = ".")
if constraint
assert_file File.join(app_path, "Gemfile"), /^\s*gem\s+["']#{gem}["'], #{constraint}$*/
else
assert_file File.join(app_path, "Gemfile"), /^\s*gem\s+["']#{gem}["']$*/
end
end

def assert_no_gem(gem, app_path = ".")
assert_file File.join(app_path, "Gemfile") do |content|
assert_no_match(gem, content)
end
end
end

0 comments on commit cc3594a

Please sign in to comment.