-
Notifications
You must be signed in to change notification settings - Fork 380
Migrate to react on rails auto-registration #649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 17 commits
9721c36
d12c40d
5854552
6a2e29b
71a60b2
027bfd7
a9c10e9
0d04fa2
fe066fb
9b9bb86
d6d123e
81f84aa
2547b80
9d2bf5e
ee5bf38
ab31233
4daa2ec
351824b
58846b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # TODO: Fix Turbo Warning in Future PR | ||
|
|
||
| ## Issue | ||
| There's a console warning about Turbo being loaded from within the `<body>` element instead of the `<head>`. | ||
|
|
||
| ## Root Cause | ||
| Conflicting requirements between three systems: | ||
| 1. **Turbo** - Wants to be loaded in `<head>` to avoid re-evaluation on page changes | ||
| 2. **Shakapacker** - Requires all `append_javascript_pack_tag` calls to happen before the final `javascript_pack_tag` | ||
| 3. **React on Rails** - The `react_component` helper internally calls `append_javascript_pack_tag` when rendering components in the body | ||
|
|
||
| ## Attempted Solutions That Failed | ||
| 1. Moving `javascript_pack_tag` to head - Breaks because `react_component` calls come after it | ||
| 2. Using `data-turbo-suppress-warning` - Doesn't properly suppress the warning | ||
|
|
||
| ## Potential Future Solutions | ||
| 1. Extract Turbo into a separate pack from stimulus-bundle and load it in the head | ||
| 2. Use `prepend_javascript_pack_tag` instead of `append` for component registration | ||
| 3. Configure React on Rails v16 to use a different component loading strategy | ||
| 4. Investigate if the auto-registration feature has a different recommended pack loading pattern | ||
|
|
||
| ## Current State | ||
| The application works correctly with the pack tags at the end of the body. The Turbo warning is cosmetic and doesn't affect functionality. | ||
|
|
||
| ## References | ||
| - PR #649: Initial v16 migration | ||
| - Shakapacker docs: https://github.com/shakacode/shakapacker#view-helper-append_javascript_pack_tag | ||
| - Turbo docs: https://turbo.hotwired.dev/handbook/building#working-with-script-elements | ||
| - React on Rails v16 docs: https://www.shakacode.com/react-on-rails/docs/ |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,13 +5,9 @@ | |||||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||||||
| <title>RailsReactTutorial</title> | ||||||
|
|
||||||
| <%= stylesheet_pack_tag('client-bundle', | ||||||
| media: 'all', | ||||||
| 'data-turbolinks-track': true) %> | ||||||
|
|
||||||
| <%= javascript_pack_tag('client-bundle', | ||||||
| 'data-turbolinks-track': true, | ||||||
| defer: true) %> | ||||||
| <%= append_stylesheet_pack_tag('stimulus-bundle') %> | ||||||
| <%= append_javascript_pack_tag('stimulus-bundle') %> | ||||||
| <%= append_javascript_pack_tag('stores-registration') %> | ||||||
|
|
||||||
| <%= csrf_meta_tags %> | ||||||
| </head> | ||||||
|
|
@@ -24,6 +20,9 @@ | |||||
|
|
||||||
| <%= react_component "Footer" %> | ||||||
|
|
||||||
| <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> | ||||||
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> | ||||||
|
||||||
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> | |
| <%= javascript_pack_tag('application', 'data-turbolinks-track': true, defer: true) %> |
Copilot uses AI. Check for mistakes.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,13 +5,8 @@ | |||||||||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||||||||||
| <title>RailsReactTutorial</title> | ||||||||||
|
|
||||||||||
| <%= stylesheet_pack_tag('client-bundle', | ||||||||||
| media: 'all', | ||||||||||
| 'data-turbolinks-track': true) %> | ||||||||||
|
|
||||||||||
| <%= javascript_pack_tag('stimulus-bundle', | ||||||||||
| 'data-turbolinks-track': true, | ||||||||||
| defer: true) %> | ||||||||||
| <%= append_stylesheet_pack_tag('stimulus-bundle') %> | ||||||||||
| <%= append_javascript_pack_tag('stimulus-bundle') %> | ||||||||||
|
|
||||||||||
| <%= csrf_meta_tags %> | ||||||||||
| </head> | ||||||||||
|
|
@@ -23,5 +18,8 @@ | |||||||||
| </div> | ||||||||||
|
|
||||||||||
| <%= react_component "Footer" %> | ||||||||||
|
|
||||||||||
| <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> | ||||||||||
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> | ||||||||||
|
||||||||||
| <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %> | |
| <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %> | |
| <%= stylesheet_pack_tag('stimulus-bundle', media: 'all', 'data-turbolinks-track': true) %> | |
| <%= javascript_pack_tag('stimulus-bundle', 'data-turbolinks-track': true, defer: true) %> |
Copilot uses AI. Check for mistakes.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,129 @@ | ||||||||
| // Generated by ReScript, PLEASE EDIT WITH CARE | ||||||||
|
||||||||
| // Generated by ReScript, PLEASE EDIT WITH CARE | |
| // This file is generated by ReScript but is intentionally committed to version control and may be manually edited. |
Copilot uses AI. Check for mistakes.
Copilot
AI
Sep 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment suggests the file is auto-generated and should not be edited, but it's being added to version control in a PR. Auto-generated files should typically be excluded from version control or the warning should be removed if manual editing is expected.
| // Generated by ReScript, PLEASE EDIT WITH CARE |
Copilot uses AI. Check for mistakes.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| // Compare to ../ServerRouterApp.jsx | ||
| // Compare to ./RouterApp.server.jsx | ||
|
||
| import { Provider } from 'react-redux'; | ||
| import React from 'react'; | ||
| import ReactOnRails from 'react-on-rails'; | ||
| import { BrowserRouter } from 'react-router-dom'; | ||
| import routes from '../routes/routes.jsx'; | ||
| import routes from '../../../routes/routes.jsx'; | ||
|
|
||
| function ClientRouterApp(_props) { | ||
| const store = ReactOnRails.getStore('routerCommentsStore'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| // Compare to ../ClientRouterApp.jsx | ||
| // Compare to ./RouterApp.client.jsx | ||
|
||
| import { Provider } from 'react-redux'; | ||
| import React from 'react'; | ||
| import { StaticRouter } from 'react-router-dom/server'; | ||
| import ReactOnRails from 'react-on-rails'; | ||
| import routes from '../routes/routes.jsx'; | ||
| import routes from '../../../routes/routes.jsx'; | ||
|
|
||
| function ServerRouterApp(_props, railsContext) { | ||
| const store = ReactOnRails.getStore('routerCommentsStore'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,13 @@ | ||
| // Example of React + Redux | ||
| import ReactOnRails from 'react-on-rails'; | ||
|
|
||
| import App from './App'; | ||
| import RouterApp from './ServerRouterApp'; | ||
| import SimpleCommentScreen from '../components/SimpleCommentScreen/SimpleCommentScreen'; | ||
| import NavigationBarApp from './NavigationBarApp'; | ||
| import App from './App/ror_components/App'; | ||
| import RouterApp from './RouterApp/ror_components/RouterApp.server'; | ||
| import SimpleCommentScreen from '../components/SimpleCommentScreen/ror_components/SimpleCommentScreen'; | ||
| import NavigationBarApp from './NavigationBarApp/ror_components/NavigationBarApp'; | ||
|
Comment on lines
+4
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major 🧩 Analysis chainIf using auto-registration, this file should not manually register components/stores. With v16 generated packs, components under ror_components are auto-registered. Keeping manual ReactOnRails.register and registerStore here can cause duplicate registrations if this file is ever imported. Prefer removing manual registrations (and the store imports) or delete this file entirely. If you need server-only variants (e.g., RouterApp.server), let the generator handle it. Please run: If unused, remove this file; if used for a special case, strip store registration and rely on packs/stores-registration.js. Based on learnings. Also applies to: 10-10 🏁 Script executed: #!/bin/bash
# 1) Verify whether this file is referenced anywhere
rg -nP 'bundles/comments/startup/serverRegistration\.(jsx|tsx|js|ts)'
# 2) List all store registrations to spot duplicates with packs/stores-registration
rg -nP 'ReactOnRails\.registerStore\s*\(' -n -C2
# 3) List all component registrations to spot duplicates with generated packs
rg -nP 'ReactOnRails\.register\s*\(' -n -C2Length of output: 1644 Remove duplicate store registrations in serverRegistration.jsx 🤖 Prompt for AI Agents |
||
| import routerCommentsStore from '../store/routerCommentsStore'; | ||
| import commentsStore from '../store/commentsStore'; | ||
| import Footer from '../components/Footer/Footer'; | ||
| import Footer from '../components/Footer/ror_components/Footer'; | ||
|
|
||
| ReactOnRails.register({ | ||
| App, | ||
|
|
||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent startup race: generate packs before webpack processes start
With v16 auto-registration, webpack can error if wp-client/wp-server start before packs exist. Prepend generate_packs to both webpack commands (and optionally Rails) and set HOST/PORT defaults for container/devbox access.
Based on learnings.
📝 Committable suggestion
🤖 Prompt for AI Agents