Skip to content

Commit 14f4a45

Browse files
justin808claude
andcommitted
fix: address PR B review feedback for unified renderer cache staging
- Doctor check now tailors the suggested migration command per matched file type: Dockerfile entries get the bare 'rake pre_seed_renderer_cache' (MODE=copy is the default); Procfile/bin entries get 'MODE=symlink'. - The copy-mode env-var error now mentions 'MODE=symlink' as an escape hatch for CI users who don't need an immutable artifact. - make_relative_symlink rescues Errno::ENOENT from Pathname#realpath and surfaces a clearer ReactOnRailsPro::Error when a bundle/asset vanished between existence check and staging (e.g. webpack output rotating mid-stage, dangling symlinks in the source tree). - PrepareNodeRenderBundles.reset_deprecation_warned! is now private_class_method; specs invoke it via send. - Rake task now downcases ENV['MODE'] and validates against PreSeedRendererCache::VALID_MODES before to_sym, accepting 'MODE=Copy', 'MODE=COPY', etc. Unknown values abort with a clear error listing valid modes. - Added a :symlink-mode spec that exercises asset symlinking (previously only covered via the PrepareNodeRenderBundles shim). - env-var-bypass spec now uses ensure block with a local tmpdir variable to guarantee cleanup even if Dir.mktmpdir or an intermediate assertion raises. Declined: thread-safety wrapper around @deprecation_warned. This code runs only from rake tasks and AssetsPrecompile.call (single-threaded contexts); a race would produce at most a harmless duplicate warning, not a correctness issue. Consistent with PR A precedent for @renderer_bundle_path_deprecation_warned. 27 dummy specs pass; rubocop clean on all changed files. Refs: #3122, #3167 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e97260d commit 14f4a45

6 files changed

Lines changed: 56 additions & 11 deletions

File tree

react_on_rails/lib/react_on_rails/doctor.rb

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2748,10 +2748,7 @@ def check_deprecated_renderer_cache_task
27482748

27492749
checker.add_warning(<<~MSG.strip)
27502750
⚠️ Deprecated rake task '#{DEPRECATED_RENDERER_CACHE_TASK}' referenced in:
2751-
#{matches.map { |p| " • #{p}" }.join("\n")}
2752-
2753-
Replace with:
2754-
rake react_on_rails_pro:pre_seed_renderer_cache MODE=symlink
2751+
#{matches.map { |p| " • #{p}#{renderer_cache_migration_suggestion(p)}" }.join("\n")}
27552752
27562753
The unified 'pre_seed_renderer_cache' task uses MODE=copy by default (for
27572754
Docker/image builds) and MODE=symlink for same-filesystem workflows.
@@ -2760,6 +2757,17 @@ def check_deprecated_renderer_cache_task
27602757
checker.add_warning("⚠️ Could not scan for deprecated renderer-cache task references: #{e.message}")
27612758
end
27622759

2760+
# Dockerfile matches mean the user is building an image, so they want the
2761+
# copy-mode default (no MODE needed). Procfile/bin scripts mean same-filesystem
2762+
# runtime staging, which needs the symlink mode.
2763+
def renderer_cache_migration_suggestion(path)
2764+
if path.start_with?("Dockerfile")
2765+
"rake react_on_rails_pro:pre_seed_renderer_cache"
2766+
else
2767+
"rake react_on_rails_pro:pre_seed_renderer_cache MODE=symlink"
2768+
end
2769+
end
2770+
27632771
# The base 'react-on-rails' npm package is a transitive dependency of 'react-on-rails-pro',
27642772
# so `import ... from 'react-on-rails'` resolves silently — loading the base package instead
27652773
# of Pro. Components registered through the base package won't have Pro features (streaming,

react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,11 @@ def self.enforce_cache_dir_env_var!(mode)
6969
The Node Renderer's default cache directory resolution differs between the Ruby
7070
and standalone Node environments, so relying on the default in production-like
7171
deploys can cause pre-seeded bundles to land in a path the renderer never reads.
72+
73+
If you don't need an immutable artifact (e.g. in CI or same-filesystem deploys),
74+
use mode: :symlink instead:
75+
76+
rake react_on_rails_pro:pre_seed_renderer_cache MODE=symlink
7277
MSG
7378
end
7479
private_class_method :enforce_cache_dir_env_var!
@@ -127,10 +132,19 @@ def self.make_relative_symlink(source, destination)
127132

128133
# Canonicalize both sides so paths like /var -> /private/var do not
129134
# produce broken relative symlinks when the cache dir comes from tmpdir.
135+
# Pathname#realpath raises Errno::ENOENT on a dangling symlink or a
136+
# path that vanished between File.exist? and here (e.g. webpack output
137+
# rotating mid-stage). Surface that as a clear ReactOnRailsPro::Error
138+
# rather than a raw system error.
130139
source_path = Pathname.new(source).realpath
131140
relative_source_path = source_path.relative_path_from(destination_dir.realpath)
132141
File.symlink(relative_source_path, destination)
133142
puts "[ReactOnRailsPro] Symlinked #{relative_source_path} to #{destination}"
143+
rescue Errno::ENOENT => e
144+
raise ReactOnRailsPro::Error,
145+
"Could not resolve real path for symlink source #{source} " \
146+
"(#{e.message}). The file may have been removed or may be a dangling symlink. " \
147+
"Rebuild your bundles before staging the renderer cache."
134148
end
135149
private_class_method :make_relative_symlink
136150
end

react_on_rails_pro/lib/react_on_rails_pro/prepare_node_renderer_bundles.rb

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,11 @@ def self.emit_deprecation_warning!
2525

2626
# :nodoc: Test helper — resets the one-time deprecation-warning guard so
2727
# specs can exercise the warning path without leaking state between examples.
28+
# Private so it can only be invoked from specs via `send`; prevents accidental
29+
# reset from production code.
2830
def self.reset_deprecation_warned!
2931
@deprecation_warned = nil
3032
end
33+
private_class_method :reset_deprecation_warned!
3134
end
3235
end

react_on_rails_pro/lib/tasks/assets.rake

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,13 @@ namespace :react_on_rails_pro do
66
desc "Stage the Node Renderer bundle cache. MODE=copy (default; Docker/image builds) " \
77
"or MODE=symlink (dev/CI/same-filesystem deploys)."
88
task pre_seed_renderer_cache: :environment do
9-
mode = (ENV["MODE"] || "copy").to_sym
10-
ReactOnRailsPro::PreSeedRendererCache.call(mode: mode)
9+
raw_mode = ENV["MODE"].to_s.downcase
10+
raw_mode = "copy" if raw_mode.empty?
11+
unless ReactOnRailsPro::PreSeedRendererCache::VALID_MODES.map(&:to_s).include?(raw_mode)
12+
valid = ReactOnRailsPro::PreSeedRendererCache::VALID_MODES.map(&:to_s).join(", ")
13+
abort "[ReactOnRailsPro] Unknown MODE=#{ENV.fetch('MODE', nil).inspect}. Expected one of: #{valid}"
14+
end
15+
ReactOnRailsPro::PreSeedRendererCache.call(mode: raw_mode.to_sym)
1116
end
1217

1318
# Deprecated alias. Delegates to pre_seed_renderer_cache with MODE=symlink so

react_on_rails_pro/spec/dummy/spec/pre_seed_renderer_cache_spec.rb

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,26 @@
6060
end
6161

6262
context "when mode is :symlink" do
63-
it "symlinks instead of copying" do
63+
it "symlinks the bundle instead of copying it" do
6464
described_class.call(mode: :symlink)
6565

6666
dest_file = File.join(bundle_dir, "#{bundle_hash}.js")
6767
expect(File.exist?(dest_file)).to be(true)
6868
expect(File.symlink?(dest_file)).to be(true)
6969
end
70+
71+
it "symlinks assets rather than copying them" do
72+
FileUtils.cp(fixture_path, path_in_webpack_folder(asset_filename))
73+
FileUtils.cp(fixture_path2, path_in_webpack_folder(asset_filename2))
74+
75+
described_class.call(mode: :symlink)
76+
77+
first_asset = File.join(bundle_dir, asset_filename)
78+
second_asset = File.join(bundle_dir, asset_filename2)
79+
expect(File.symlink?(first_asset)).to be(true)
80+
expect(File.symlink?(second_asset)).to be(true)
81+
expect(File.realpath(first_asset)).to eq(path_in_webpack_folder(asset_filename).to_s)
82+
end
7083
end
7184

7285
context "when mode is :copy and no env var is set in a non-dev/test environment" do
@@ -81,10 +94,12 @@
8194
end
8295

8396
it "does not raise when the preferred env var is set" do
84-
ENV["RENDERER_SERVER_BUNDLE_CACHE_PATH"] = Dir.mktmpdir("renderer-cache-test")
97+
tmpdir = Dir.mktmpdir("renderer-cache-test")
98+
ENV["RENDERER_SERVER_BUNDLE_CACHE_PATH"] = tmpdir
8599
expect { described_class.call(mode: :copy) }.not_to raise_error
86100
ensure
87-
FileUtils.rm_rf(ENV.fetch("RENDERER_SERVER_BUNDLE_CACHE_PATH", nil))
101+
FileUtils.rm_rf(tmpdir) if tmpdir
102+
ENV.delete("RENDERER_SERVER_BUNDLE_CACHE_PATH")
88103
end
89104

90105
it "does not raise in :symlink mode even without an env var" do

react_on_rails_pro/spec/dummy/spec/prepare_node_renderer_bundles_spec.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
ENV.delete("RENDERER_SERVER_BUNDLE_CACHE_PATH")
4040
ENV.delete("RENDERER_BUNDLE_PATH")
4141
ReactOnRailsPro::Utils.reset_renderer_bundle_path_deprecation_warned!
42-
described_class.reset_deprecation_warned!
42+
described_class.send(:reset_deprecation_warned!)
4343
end
4444

4545
after do
@@ -49,7 +49,7 @@
4949
FileUtils.rm_f(path_in_webpack_folder(asset_filename2))
5050
ENV.delete("RENDERER_SERVER_BUNDLE_CACHE_PATH")
5151
ENV.delete("RENDERER_BUNDLE_PATH")
52-
described_class.reset_deprecation_warned!
52+
described_class.send(:reset_deprecation_warned!)
5353
end
5454

5555
it "emits a deprecation warning pointing at PreSeedRendererCache" do

0 commit comments

Comments
 (0)