|
| 1 | +# Issue #3258 — RSC Migration Parser Helper Clarity Cleanup |
| 2 | + |
| 3 | +**Status:** Approved |
| 4 | +**Author:** Justin Gordon (drafted with Claude Code) |
| 5 | +**Date:** 2026-05-21 |
| 6 | +**Related PR:** #3219 (`codex/fix-rsc-client-reference-scope`) |
| 7 | +**Branch strategy:** Stacked follow-up PR on top of `codex/fix-rsc-client-reference-scope` so review of the behavioral fix in #3219 stays isolated from this clarity cleanup. |
| 8 | + |
| 9 | +## Background |
| 10 | + |
| 11 | +PR #3219 (`fix: scope RSC client reference discovery`) ships a behavioral fix |
| 12 | +that scopes `RSCWebpackPlugin#clientReferences` to the Shakapacker |
| 13 | +`config.source_path` for both new installs and existing webpack configs. The PR |
| 14 | +introduces a sizeable migration path in |
| 15 | +`react_on_rails/lib/generators/react_on_rails/rsc_setup.rb` that detects an |
| 16 | +existing `RSCWebpackPlugin(...)` call, parses its options block with a |
| 17 | +lightweight JS scanner, and either rewrites the options or warns when a rewrite |
| 18 | +is unsafe. |
| 19 | + |
| 20 | +The cloud reviews on #3219 flagged three non-blocking clarity items. They were |
| 21 | +intentionally deferred from #3219 to keep that PR focused on behavior. Issue |
| 22 | +\#3258 tracks them. |
| 23 | + |
| 24 | +This spec covers only those three items. No behavior changes. |
| 25 | + |
| 26 | +## In Scope |
| 27 | + |
| 28 | +1. Rename `rsc_plugin_without_client_references?` so the per-section |
| 29 | + ("any one of the sections lacks `clientReferences`") semantics are explicit. |
| 30 | +2. Tighten the scanner-supported-surface documentation around regex literals |
| 31 | + with curly braces. No parser changes. |
| 32 | +3. Remove the duplicate defensive guards at the top of |
| 33 | + `add_rsc_client_references_setup`, and document its precondition. |
| 34 | + |
| 35 | +All three changes are confined to `rsc_setup.rb` and its existing spec file. |
| 36 | + |
| 37 | +## Out of Scope |
| 38 | + |
| 39 | +- Behavioral changes to the migration logic. |
| 40 | +- Expanding the JS scanner to handle regex-literal brace quantifiers |
| 41 | + (e.g. `/a{2}/`). The generated configs never produce such regexes, and |
| 42 | + existing-config rewrites already emit a warning rather than guess when the |
| 43 | + scanner cannot parse a block. Revisit only if a future RSC plugin option |
| 44 | + requires it. |
| 45 | +- Touching any other PR #3219 review feedback. Each remaining cloud-review item |
| 46 | + either has its own follow-up issue or was already addressed in #3219. |
| 47 | + |
| 48 | +## Item 1 — Rename `rsc_plugin_without_client_references?` |
| 49 | + |
| 50 | +### Current state |
| 51 | + |
| 52 | +```ruby |
| 53 | +def rsc_plugin_without_client_references?(content, is_server:) |
| 54 | + rsc_plugin_option_sections(content, is_server: is_server).any? do |section| |
| 55 | + !rsc_plugin_options_without_comments(section.fetch(:body)).match?(/\bclientReferences\s*:/) |
| 56 | + end |
| 57 | +end |
| 58 | +``` |
| 59 | + |
| 60 | +The implementation uses `.any?`, but the method name suggests an all-or-nothing |
| 61 | +check ("the plugin has no clientReferences"). When two plugin instances share |
| 62 | +the same `isServer` value and one already has `clientReferences` while the |
| 63 | +other does not, the method correctly returns `true` and the rewrite proceeds — |
| 64 | +but a reader who only inspects the name would expect that case to return |
| 65 | +`false`. |
| 66 | + |
| 67 | +### New name |
| 68 | + |
| 69 | +`any_rsc_plugin_section_without_client_references?` |
| 70 | + |
| 71 | +- Keeps the existing predicate suffix (`?`). |
| 72 | +- Adds the `any_` prefix that mirrors the `.any?` in the body. |
| 73 | +- Replaces `plugin` with `plugin_section` to match `rsc_plugin_option_sections` |
| 74 | + (the data the method iterates over). |
| 75 | + |
| 76 | +### Affected sites |
| 77 | + |
| 78 | +Only one call site exists in `rsc_setup.rb`: |
| 79 | + |
| 80 | +``` |
| 81 | +react_on_rails/lib/generators/react_on_rails/rsc_setup.rb:576 |
| 82 | +``` |
| 83 | + |
| 84 | +The method is private to the generator class; no spec asserts on the method |
| 85 | +name directly. A repo-wide grep confirms no other production or test code |
| 86 | +references the symbol. |
| 87 | + |
| 88 | +### Acceptance |
| 89 | + |
| 90 | +- `bundle exec rubocop react_on_rails/lib/generators/react_on_rails/rsc_setup.rb` |
| 91 | + passes. |
| 92 | +- `bundle exec rspec react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb` |
| 93 | + passes unchanged. |
| 94 | + |
| 95 | +## Item 2 — Document scanner-supported surface for regex-literal braces |
| 96 | + |
| 97 | +### Current state |
| 98 | + |
| 99 | +`rsc_plugin_options_without_comments` (around line 671 on the PR branch) carries |
| 100 | +a comment noting that regex literals like `/a{2}/` are outside the scanner's |
| 101 | +supported surface because brace quantifiers can confuse |
| 102 | +`matching_js_closing_brace`'s depth counter. |
| 103 | + |
| 104 | +`matching_js_closing_brace` itself (the helper that actually does the brace |
| 105 | +tracking) carries no such note. A reader who lands on that helper directly — |
| 106 | +e.g. via a stack trace from a future bug — has no inline indication that regex |
| 107 | +literals are an unsupported input class. |
| 108 | + |
| 109 | +### Change |
| 110 | + |
| 111 | +Add a brief comment block above `matching_js_closing_brace` that lists what it |
| 112 | +supports (strings, line and block comments) and what it does not (regex |
| 113 | +literals — specifically brace quantifiers like `/a{2}/` and character classes |
| 114 | +like `/[{]/`). Cross-reference `rsc_plugin_options_without_comments` so readers |
| 115 | +can see why the limitation is acceptable in the current call graph. |
| 116 | + |
| 117 | +Tighten the existing comment on `rsc_plugin_options_without_comments` to use |
| 118 | +the same phrasing for "unsupported constructs," so both comments read as one |
| 119 | +documented contract instead of two slightly different warnings. |
| 120 | + |
| 121 | +The warning text emitted by `warn_unparseable_rsc_plugin_sections` already |
| 122 | +explains the user-facing failure mode (`"most often a regex literal with an |
| 123 | +unmatched '{' or '}', e.g. '/\{/' or '/[{]/'"`), so no warning-message change |
| 124 | +is needed. |
| 125 | + |
| 126 | +### Acceptance |
| 127 | + |
| 128 | +- No code changes outside of comments. |
| 129 | +- `bundle exec rubocop` passes. |
| 130 | + |
| 131 | +## Item 3 — Remove duplicate defensive guards in `add_rsc_client_references_setup` |
| 132 | + |
| 133 | +### Current state |
| 134 | + |
| 135 | +```ruby |
| 136 | +def add_rsc_client_references_setup(config_path, content, is_server:) |
| 137 | + return if scoped_rsc_client_references_defined?(content) |
| 138 | + return if rsc_client_references_defined?(content) |
| 139 | + |
| 140 | + existing_imports_content = content_before_rsc_setup_anchor(content, is_server: is_server) |
| 141 | + replace_rsc_client_references_setup_anchor(config_path, content, is_server: is_server) do |anchor| |
| 142 | + [ |
| 143 | + anchor, |
| 144 | + shakapacker_config_import_statement(existing_imports_content), |
| 145 | + path_resolve_import_statement(existing_imports_content), |
| 146 | + "", |
| 147 | + rsc_client_references_js |
| 148 | + ].compact.join("\n") |
| 149 | + end |
| 150 | +end |
| 151 | +``` |
| 152 | + |
| 153 | +The sole caller, `ensure_rsc_client_references_setup`, already performs both |
| 154 | +checks before invoking this method: |
| 155 | + |
| 156 | +```ruby |
| 157 | +def ensure_rsc_client_references_setup(config_path, content, is_server:) |
| 158 | + return true if scoped_rsc_client_references_defined?(content) |
| 159 | + |
| 160 | + if rsc_client_references_defined?(content) |
| 161 | + warn_unscoped_rsc_client_references_helper(config_path) |
| 162 | + return false |
| 163 | + end |
| 164 | + ... |
| 165 | + add_rsc_client_references_setup(config_path, content, is_server: is_server) |
| 166 | + ... |
| 167 | +end |
| 168 | +``` |
| 169 | + |
| 170 | +The duplicated guards inside `add_rsc_client_references_setup` are unreachable |
| 171 | +in the current call graph. CLAUDE.md guidance: "Don't add error handling, |
| 172 | +fallbacks, or validation for scenarios that can't happen. Trust internal code |
| 173 | +and framework guarantees." |
| 174 | + |
| 175 | +### Change |
| 176 | + |
| 177 | +- Remove the two `return if ...` guards. |
| 178 | +- Replace the existing leading comment with one that names the precondition: |
| 179 | + must only be called via `ensure_rsc_client_references_setup`, which has |
| 180 | + already verified that no `rscClientReferences` declaration (scoped or |
| 181 | + otherwise) exists at module scope and that the import anchor is present. |
| 182 | + |
| 183 | +### Acceptance |
| 184 | + |
| 185 | +- `bundle exec rspec react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb` |
| 186 | + passes — existing specs already exercise the `ensure_*` path on every code |
| 187 | + path that ends up calling `add_*`. |
| 188 | +- `bundle exec rubocop` passes. |
| 189 | +- Manual code reading: no other call site to `add_rsc_client_references_setup` |
| 190 | + appears in the repo (verified via `grep`). |
| 191 | + |
| 192 | +## Risk Assessment |
| 193 | + |
| 194 | +- **Item 1:** Pure rename of a private method with a single call site. Risk: |
| 195 | + near zero. Worst case: a missed reference produces an immediate |
| 196 | + `NoMethodError` caught by the existing spec suite. |
| 197 | +- **Item 2:** Comments only. Zero runtime risk. |
| 198 | +- **Item 3:** Removes dead code. The risk is that a future caller could be |
| 199 | + added without re-applying the guards. Mitigated by the precondition comment; |
| 200 | + the helper is private and lives in the same file as its caller, so a future |
| 201 | + contributor sees both within the same edit window. |
| 202 | + |
| 203 | +## Test Plan |
| 204 | + |
| 205 | +- `bundle exec rspec react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb` |
| 206 | +- `bundle exec rubocop react_on_rails/lib/generators/react_on_rails/rsc_setup.rb react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb` |
| 207 | +- Manual `grep` for the old method name across the repo to confirm zero stale |
| 208 | + references after the rename. |
| 209 | + |
| 210 | +## Out of Scope (Reiterated) |
| 211 | + |
| 212 | +No changes to: |
| 213 | + |
| 214 | +- `react_on_rails/lib/generators/react_on_rails/templates/**` |
| 215 | +- Any documentation file outside this design spec. |
| 216 | +- The behavior or wording of `GeneratorMessages` warnings. |
| 217 | + |
| 218 | +## Follow-ups |
| 219 | + |
| 220 | +None planned. If a future RSC plugin option introduces regex literals with |
| 221 | +brace quantifiers, file a new issue to expand `matching_js_closing_brace` (or |
| 222 | +adopt a real JS parser); the warning emitted today gives users a clear manual |
| 223 | +remediation in the meantime. |
0 commit comments