[mirror] docs: warn about react_component helper collision with react-rails (#3143)#1
Open
yashwant86 wants to merge 10 commits intomm-base-3160from
Open
[mirror] docs: warn about react_component helper collision with react-rails (#3143)#1yashwant86 wants to merge 10 commits intomm-base-3160from
yashwant86 wants to merge 10 commits intomm-base-3160from
Conversation
Incremental migrations from react-rails were breaking silently as soon as react_on_rails was added: both gems define react_component with incompatible signatures, and Rails helper auto-loading makes the second-defined one win. Un-migrated views then raise ArgumentError. Extend the migration guide with a preflight callout and a new "Coexistence" section that explains the collision, shows how to detect affected call sites, and documents two paths: - migrate all call sites in one PR (recommended), or - preserve legacy react-rails semantics via a prepended shim and use an explicit react_on_rails_component alias for migrated mounts. Addresses shakacode#3143. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Clarify that the helper collision is driven by engine initializer load order, not file order in app/helpers/ - Forward &block in the LegacyReactComponent shim so react-rails block-form calls keep working (react-rails' react_component accepts a block for mount-tag content) - Split Option B into a helper file + initializer; apply the prepend/include inside Rails.application.config.to_prepare so patches run once at boot and are reapplied after dev reloads (avoiding Zeitwerk autoload timing issues and duplicate ancestors) - Reword "happens silently" to clarify that ArgumentError is raised at render time; Rails gives no boot-time warning - Add a comment in the react_on_rails_component snippet explaining why the unbound method is fetched from ReactOnRails::Helper (not ReactOnRailsHelper) to avoid infinite recursion - Add Action Mailer and Rails engines to the Option B known limitations alongside gem-provided engines Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clarify that 2-argument legacy calls like `react_component "Foo", props` do not raise — they are silently accepted and drop props — while only 3+ argument calls raise ArgumentError. The previous wording implied every un-migrated view would fail loudly, which could mislead readers doing staged migrations into missing silent prop loss. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address remaining PR review feedback on the react-rails coexistence section: - Replace the inaccurate "rename react_on_rails_component back to react_component" cleanup step with the actual procedure (update each call site and delete the two shim files). - Note that react_on_rails wins the helper collision in practice because of initializer sort order, but that teams should not rely on ordering to resolve the conflict. - Tighten "is not practical" to "is impractical". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion - Split the long "Why it collides" paragraph into two so the failure-mode bullets open their own paragraph (review #3106012646). - Document bind(self)'s precondition (self must include React::Rails::ViewHelper) and mention bind_call as a Ruby 2.7+ alternative (review #3106012927). - Note in the to_prepare block that Ruby skips duplicate prepend/include so reloads don't accumulate ancestors (review #3106013062). - Add an explicit "two-rename" callout so readers expect call sites to flip name twice across the migration lifecycle (review #3105700862). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address review feedback on the react_component collision section: - Define the ReactOnRailsCoexistence module directly in the initializer instead of splitting across app/helpers/ and config/initializers/. This keeps the shim out of Zeitwerk's autoload paths and avoids bookkeeping confusion in production eager-load mode. - Switch both helpers from .bind(self).call(...) to .bind_call(self, ...) so the code matches the Ruby 2.7+ guidance mentioned in the comment. React on Rails requires Ruby 3.0+, so bind_call is always available. - Broaden the detection grep to include app/components and note that users with Phlex/mailer/engine mounts should expand the path further or drop it entirely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Fix preflight item 7 wording: "second-loaded" implied gem/load order, but body correctly explains it's Rails helper module precedence. - Add Ruby 2.7+ caveat with `bind(self).call(...)` fallback for the `UnboundMethod#bind_call` calls in the shim. - Add a callout marking the shim as community-sourced and not covered by the react_on_rails test suite; recommend staging verification. - Foreground the two project-wide rename passes as the first bullet in Known limitations of Option B, removing the now-duplicate trailing Note. - Include `app/mailers` in the detection grep example so it matches the surrounding prose. - Qualify "Rails gives no boot-time warning" with "As of Rails 7/8" to future-proof the claim.
- Detection commands: added `app/helpers` to the `rg`/`grep` path list so view-helper wrappers that call `react_component` from Ruby are not missed. - Shim inline comment: spelled out that `bind_call` raises `TypeError` at render time when `self` doesn't include `React::Rails::ViewHelper`, and pointed readers at the Known Limitations section. - Known Limitations: upgraded the "will not help" bullet to make explicit that unsupported view contexts (Rails engines, ViewComponent, Action Mailer, gem engines) hard-fail with `TypeError` rather than degrade gracefully, and added a new bullet on cleanup ordering: delete `config/initializers/react_on_rails_coexistence.rb` in the same commit that drops `react-rails` from the `Gemfile`, otherwise any request that still routes through the legacy delegate raises `NameError: uninitialized constant React::Rails::ViewHelper` at render time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
⚡ Risk Assessment —
|
| Files | Summary |
|---|---|
Migration Documentationdocs/oss/migrating/migrating-from-react-rails.md |
Documents react_component helper naming collision and provides guidance for staged coexistence of react-rails and react_on_rails gems during migration. |
Dig Deeper With Commands
/review <file-path> <function-optional>/chat <file-path> "<question>"/roast <file-path>
Runs only when explicitly triggered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mirror of upstream shakacode#3160 for benchmark. Do not merge.
Summary by MergeMonkey