Skip to content

Commit ae5ff44

Browse files
justin808claude
justin808
and
claude
committed
docs(spike): evaluate Prism-based Gemfile rewriting for Pro migration
Spike for #3313. Standalone Prism prototype + 32-case behavior matrix + benchmark + decision record. Lives under react_on_rails/spike/ so it does not touch the production scanner from #3232 (per the issue's non-goals). Recommendation: keep the current text scanner. Revisit when the Ruby floor rises to 3.3 and Prism becomes a free runtime dependency. The empty-else case the issue calls out is solvable cleanly by the Prism prototype (conditional collapse), but the cost of adding prism as a runtime gem for Ruby 3.0-3.2 users outweighs the cleanup for a generator that runs once per migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 05b6a23 commit ae5ff44

6 files changed

Lines changed: 1141 additions & 0 deletions

File tree

‎react_on_rails/.rubocop.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ AllCops:
1111

1212
Exclude:
1313
- 'spec/dummy/bin/*'
14+
- 'spike/**/*' # Exploratory spike code outside lib/ — not part of the production surface
1415

1516
Naming/FileName:
1617
Exclude:

‎react_on_rails/spike/3313_prism_gemfile_rewriter/DECISION.md‎

Lines changed: 257 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,257 @@
1+
# Decision record: Prism-based Gemfile rewriting for Pro migration
2+
3+
**Issue:** [#3313](https://github.com/shakacode/react_on_rails/issues/3313)
4+
**Status:** Spike complete, recommendation: **wait for Ruby floor to rise to 3.3**, keep
5+
the current text scanner in the meantime, and revisit then.
6+
**Date:** 2026-05-21
7+
**Author:** Justin Gordon (Claude Code assistance)
8+
9+
## TL;DR
10+
11+
A working Prism prototype lives at
12+
[`prism_gemfile_rewriter.rb`](./prism_gemfile_rewriter.rb) with a 32-case behavior
13+
matrix in [`prism_gemfile_rewriter_spec.rb`](./prism_gemfile_rewriter_spec.rb). All
14+
cases pass, including the empty-`else` case the current scanner gets wrong.
15+
16+
The prototype demonstrates that a parser-backed rewrite is **viable** and produces
17+
cleaner output for conditional declarations. It is also dramatically simpler than
18+
the current scanner (~210 lines including the conditional-collapse pass, vs.
19+
~175 lines of pure scanner in `pro_migration.rb` plus ~330 lines of rewrite logic
20+
across `pro_generator.rb`).
21+
22+
But the **cost is a new runtime gem dependency** (`prism`) for the ~30% of users
23+
still on Ruby 3.0–3.2. Until the gem's Ruby floor rises to 3.3 (when Prism is
24+
in CRuby's stdlib), the dependency cost outweighs the rewrite cleanup. Migration
25+
generator code runs once per user, and the current scanner is tested against the
26+
full matrix already.
27+
28+
Recommendation: keep the current scanner. Revisit when `react_on_rails` drops
29+
Ruby 3.0–3.2 support.
30+
31+
## Background
32+
33+
PR [#3232](https://github.com/shakacode/react_on_rails/pull/3232) hardened the Pro
34+
migration generator's Gemfile rewrite path with a regex-free text scanner. The
35+
review cycle exposed a maintenance pattern: each unusual Gemfile shape (multiline
36+
parenthesized calls, postfix guards, trailing-comma suffixes, no-final-newline,
37+
stale-base-with-pro-present) needed more scanner special-casing. CodeQL ReDoS
38+
detection tripped on the original regex implementation and forced a regex-free
39+
rewrite.
40+
41+
Issue #3313 asked: would a Prism-backed rewrite reduce this maintenance cost and
42+
remove the static-analysis exposure?
43+
44+
## What was built
45+
46+
A standalone Prism-based rewriter at
47+
[`spike/3313_prism_gemfile_rewriter/`](./).
48+
49+
| File | Purpose |
50+
| -------------------------------- | ---------------------------------------------------------------------------- |
51+
| `prism_gemfile_rewriter.rb` | The prototype: parse → locate `gem` call nodes → location-based source edits |
52+
| `prism_gemfile_rewriter_spec.rb` | 32-case behavior matrix |
53+
| `benchmark.rb` | Parse + rewrite timing vs. the current scanner |
54+
55+
The prototype is **outside `lib/`** to satisfy the issue's "Do not rewrite #3232
56+
as part of this spike" non-goal.
57+
58+
### Approach
59+
60+
1. Parse Gemfile source with `Prism.parse`.
61+
2. Walk the AST and collect every `CallNode` whose receiver is `nil`, name is `:gem`,
62+
and whose first argument is a `StringNode` literal of `react_on_rails` or
63+
`react_on_rails_pro`.
64+
3. Decide based on whether an active Pro gem is present:
65+
- **No Pro gem:** for each base call, replace the first string literal with
66+
`react_on_rails_pro`. If the call has no user version pin (no second positional
67+
`StringNode`), splice in the default version literal after the gem name.
68+
- **Pro gem already present:** for each base call, remove its enclosing
69+
statement byte range (line start through trailing newline).
70+
4. After removals, re-parse and find any `if/unless ... end` whose branch became
71+
empty. If the conditional's surviving branch contains exactly the Pro gem call,
72+
collapse the conditional to that single declaration. Otherwise remove just the
73+
empty branch.
74+
75+
All edits are **byte-offset splices**, so comments, spacing, heredocs, and unrelated
76+
Ruby are preserved by construction (no AST formatter).
77+
78+
## Behavior matrix — full coverage
79+
80+
The spec covers every shape enumerated in #3313 and they all pass with the
81+
prototype as written:
82+
83+
| Shape | Prism | Current scanner |
84+
| ---------------------------------------------------------- | ----------------------------------- | -------------------------- |
85+
| Exact version pin (`gem "react_on_rails", "16.0.0"`) | ✅ | ✅ |
86+
| Pessimistic version pin (`"~> 16.0"`) | ✅ | ✅ |
87+
| Multi-constraint pins (`">= 15.0", "< 16.0"`) | ✅ | ✅ |
88+
| Single quote style | ✅ | ✅ |
89+
| `path:`, `git:`, `github:`, `require: false`, `platforms:` | ✅ | ✅ |
90+
| Postfix guard (`if ENV["X"]`) | ✅ | ✅ |
91+
| Multiline non-parenthesized continuation | ✅ | ✅ |
92+
| Multiline parenthesized | ✅ | ✅ (parens stripped) |
93+
| Parenthesized with postfix guard | ✅ (parens preserved) | ✅ (parens stripped) |
94+
| Trailing comment after closing paren | ✅ | ✅ |
95+
| Inline comments containing `)` | ✅ | ✅ |
96+
| Comment-only continuation lines | ✅ | ✅ |
97+
| Trailing-comma-only suffix (`gem("ror",)`) | ✅ (Ruby 3.3+ parses cleanly) | ✅ |
98+
| No final newline | ✅ | ✅ |
99+
| Duplicate declarations across groups | ✅ | ✅ |
100+
| Active Pro present, base stale | ✅ (removes base) | ✅ |
101+
| Conditional with `if/else` (both branches base) | ✅ (rewrites both) | ✅ |
102+
| Conditional with `if pro / else base` | ✅ **collapses to single Pro decl** | ⚠️ **leaves empty `else`** |
103+
104+
The only **observable behavior differences**:
105+
106+
1. **Parenthesized declarations.** The current scanner strips parens
107+
(`gem("ror", "~> 16.0")` → `gem "ror", "~> 16.0"`). The Prism prototype
108+
preserves the user's style. Neither is wrong; preserving the user's source style
109+
is arguably more polite.
110+
2. **The empty-`else` case (called out in #3313).** The current scanner leaves an
111+
ugly empty `else` branch. The Prism prototype collapses the entire conditional
112+
to a single `gem "react_on_rails_pro", "16.0.0"` declaration, because the
113+
conditional's only purpose was to select between gem variants and there is only
114+
one variant after migration.
115+
116+
This is the policy choice asked for in the issue's acceptance criteria. See
117+
"Conditional/empty-branch policy" below for the reasoning.
118+
119+
## Conditional / empty-branch policy
120+
121+
**Chosen:** collapse the conditional when a branch becomes empty _and_ the surviving
122+
branch contains only `gem "react_on_rails_pro"`.
123+
124+
```ruby
125+
# Before
126+
if ENV["PRO"]
127+
gem "react_on_rails_pro", "16.0.0"
128+
else
129+
gem "react_on_rails", "16.0.0"
130+
end
131+
132+
# After
133+
gem "react_on_rails_pro", "16.0.0"
134+
```
135+
136+
**Why collapse, not "delete empty branch" or "leave it":**
137+
138+
- "Leave it" is what the scanner does today, and the issue explicitly calls this
139+
out as the ugliness the spike should evaluate.
140+
- "Delete the empty `else`" would leave `if ENV["PRO"] then gem_pro end`, which
141+
changes the user's runtime behavior: before, _some_ gem was always installed;
142+
after, the gem is conditional on `ENV["PRO"]` being set. This is a silent
143+
semantic change and the worst option.
144+
- Collapse preserves the original semantic ("install one specific gem") with the
145+
same conditionality the user had (none).
146+
147+
**Edge cases the prototype does _not_ collapse** (and leaves the empty branch in
148+
place):
149+
150+
- If the surviving branch has multiple statements (the Pro gem plus other gems
151+
or Bundler DSL calls), the conditional is left structurally intact.
152+
- If the empty branch is the `if`-branch (rare, would require an inverse setup
153+
like `if ENV["BASE"] then gem_base else gem_pro end`), we leave it; rewriting
154+
`if X; (empty); else BODY; end` into `unless X; BODY; end` is mechanically
155+
fine but visually surprising. Out of scope for the spike.
156+
157+
## Parse-failure policy
158+
159+
**Chosen:** return the original Gemfile content untouched with `parse_failed: true`
160+
and the list of Prism errors. The calling generator should fall back to a clear
161+
manual-edit message ("we could not parse your Gemfile; please update it manually").
162+
163+
Why not fall back to the current scanner: it would silently re-introduce the exact
164+
class of edge-case bugs Prism was supposed to remove. Either we trust the parser or
165+
we don't.
166+
167+
Verified against a deliberately malformed Gemfile in the spec
168+
(`returns the original content untouched when Gemfile cannot be parsed`). Prism's
169+
error-tolerant parsing means even quite broken Ruby may produce _some_ AST, but
170+
`Prism::ParseResult#failure?` is the boundary: if any errors are recorded, we
171+
treat the file as unmodifiable.
172+
173+
## Compatibility notes
174+
175+
| Ruby | Prism status |
176+
| ---- | ----------------------------------------------------- |
177+
| 3.0 | gem only, MRI does not bundle |
178+
| 3.1 | gem only, MRI does not bundle |
179+
| 3.2 | gem only, MRI does not bundle |
180+
| 3.3+ | bundled with CRuby (the parser used by the VM itself) |
181+
182+
`react_on_rails`'s gemspec currently sets `required_ruby_version >= 3.0.0`. Shipping
183+
Prism as a runtime dependency means:
184+
185+
- Ruby 3.3+ users: no new install. Prism is already in their Ruby.
186+
- Ruby 3.0–3.2 users: adds one gem (`prism` ~250KB, native extension, builds in
187+
seconds). No transitive dependencies.
188+
189+
The prism gem itself is maintained by the Ruby core team, sees frequent releases,
190+
and the 1.x line has been stable since 2024.
191+
192+
The issue notes that the `parser` gem (whitequark) is in soft deprecation and
193+
redirects users to `Prism::Translation::Parser`. Any new parser dependency
194+
should target Prism directly — confirmed.
195+
196+
## Performance
197+
198+
Measured on Ruby 3.4.8 / Prism 1.9.0 / Apple Silicon. 200 iterations per case, with
199+
3 warm-up iterations to avoid first-call overhead.
200+
201+
| Gemfile | Scanner | Prism | Ratio |
202+
| ---------------------------------- | --------------- | --------------- | ----- |
203+
| Small (~10 lines, 1 ror entry) | 0.012ms/rewrite | 0.015ms/rewrite | 1.25× |
204+
| Medium (~45 lines, groups) | 0.040ms/rewrite | 0.048ms/rewrite | 1.20× |
205+
| Large (~300 lines, scaled fixture) | 0.285ms/rewrite | 0.304ms/rewrite | 1.07× |
206+
207+
Both are sub-millisecond. The Pro generator runs `bundle install` immediately after
208+
the rewrite, which takes 5–60 seconds. **Parse-time cost is not a decision factor.**
209+
210+
## Recommendation
211+
212+
**Wait for the Ruby floor to rise to 3.3, then revisit.**
213+
214+
Reasoning:
215+
216+
1. **The current scanner works.** PR #3232 closed the open bugs. The test matrix
217+
is comprehensive. CodeQL is happy. The empty-`else` case is the only known
218+
residual, and it is **valid Ruby that produces a valid Bundler DSL** — only
219+
ugly, not broken.
220+
2. **Migration code runs once per user.** A small ugliness left behind by the
221+
rewrite that the user then commits to their repo is, at worst, a cleanup nit
222+
the user can resolve in one minute. It does not regress on subsequent runs
223+
because the base gem is gone.
224+
3. **Adding `prism` as a runtime dep for ~30% of users (Ruby 3.0–3.2) buys
225+
migration-generator cleanup only.** No other path in `react_on_rails` would
226+
benefit from a parser, and a single-purpose runtime dep is a smell.
227+
4. **When the Ruby floor moves to 3.3** (planned for the next major), Prism
228+
becomes free and the recommendation flips: cut over to the Prism implementation,
229+
delete the scanner.
230+
5. **Until then, the scanner is the right tool.** Pragmatic, tested, and gated
231+
behind a generator that the user runs once.
232+
233+
## What to do if/when this flips
234+
235+
1. Move `prism_gemfile_rewriter.rb` into `lib/react_on_rails/pro_migration/prism_gemfile_rewriter.rb`.
236+
2. Add `spec.add_dependency "prism", "~> 1.0"` to `react_on_rails.gemspec`.
237+
3. Replace the `swap_base_gem_for_pro_in_gemfile` body in
238+
`lib/generators/react_on_rails/pro_generator.rb` with a call to the Prism rewriter.
239+
4. Add a parse-failure UX path: when `result.parse_failed`, surface a clear
240+
"please update your Gemfile manually" message with the prism error line numbers.
241+
5. Migrate the existing spec assertions to expect the Prism output (parens preserved
242+
in `gem("…")` cases, empty-`else` collapsed).
243+
6. Delete `lib/react_on_rails/pro_migration.rb`'s scanner methods (keep the JS-side
244+
constants).
245+
7. Run the spike spec under the production path to confirm parity.
246+
247+
Approximate effort with the prototype as a starting point: 4–8 hours including
248+
review and spec migration.
249+
250+
## Open questions left for the implementation PR (not this spike)
251+
252+
- Should the generator output a `say` message when it collapses a conditional, so
253+
the user understands the structural change to their Gemfile?
254+
- Should the empty-`if`-branch case (`if X; (empty); else BODY; end`) be rewritten
255+
to `unless X; BODY; end`, or left alone? The prototype leaves it alone.
256+
- Is `Prism::Translation::Parser` a useful escape hatch for projects that already
257+
depend on `parser`, or is the direct Prism API sufficient?

‎react_on_rails/spike/3313_prism_gemfile_rewriter/README.md‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
# Spike: Prism-based Gemfile rewriter for Pro migration (#3313)
2+
3+
Exploratory prototype evaluating whether a Prism AST-based Gemfile rewriter would
4+
reduce maintenance and static-analysis risk vs. the current text/scanner approach
5+
shipped in PR #3232.
6+
7+
**This is not production code.** It is intentionally outside `lib/` to satisfy the
8+
"Do not rewrite #3232 as part of this spike" non-goal in
9+
[issue #3313](https://github.com/shakacode/react_on_rails/issues/3313).
10+
11+
## Files
12+
13+
- `prism_gemfile_rewriter.rb` — the prototype rewriter.
14+
- `prism_gemfile_rewriter_spec.rb` — drives the prototype through the behavior
15+
matrix from issue #3313 and compares against the current scanner.
16+
- `benchmark.rb` — parse+rewrite perf comparison.
17+
18+
## How to run
19+
20+
```
21+
cd react_on_rails
22+
bundle exec rspec spike/3313_prism_gemfile_rewriter/prism_gemfile_rewriter_spec.rb
23+
bundle exec ruby spike/3313_prism_gemfile_rewriter/benchmark.rb
24+
```
25+
26+
## Decision record
27+
28+
See [`DECISION.md`](./DECISION.md).

0 commit comments

Comments
 (0)