Add tests for React 16 to CI #1732
Conversation
WalkthroughThis set of changes introduces new configuration and client-side code to support React 16 in the codebase. ESLint and Knip configurations are updated to accommodate files under Changes
Sequence Diagram(s)sequenceDiagram
participant Script as convert script
participant FS as Filesystem
participant Config as Config Files
participant App as React16 App Entry
Script->>FS: Move/rename app-react16 files (remove -react16 suffix)
Script->>Config: Update dependencies to React 16 in package.json, tsconfig, babel.config.js, etc.
Script->>Config: Pin shakapacker to v6.6.0 in Gemfile and package.json
Script->>Config: Adjust Jest and TypeScript settings for React 16
App->>DOM: Render or hydrate React16 app (ManualRenderApp/ReduxApp/ReduxSharedStoreApp)
App->>Redux: Create or use Redux store as needed
App->>HMR: Support hot module replacement for containers/reducers
Assessment against linked issues
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alexeyr-ci
force-pushed
the
alexeyr/react-16-tests
branch
5 times, most recently
from
c8f58d1 to
4034118
Compare
| gsub_file_content( | ||
| "../package.json", | ||
| "jest node_package/tests", | ||
| 'jest node_package/tests --testPathIgnorePatterns=\".*(RSC|stream|serverRenderReactComponent).*\"' |
There was a problem hiding this comment.
Instead of this we can either:
- Include something like
#react18in the description of these tests and filter them out; - Use https://www.npmjs.com/package/jest-runner-groups.
Any preference?
There was a problem hiding this comment.
If what you have works for now, I say we use it.
alexeyr-ci
force-pushed
the
alexeyr/react-16-tests
branch
2 times, most recently
from
d44422d to
a824b57
Compare
alexeyr-ci
force-pushed
the
alexeyr/react-16-tests
branch
2 times, most recently
from
d20da7f to
0fe51fd
Compare
alexeyr-ci
force-pushed
the
alexeyr/react-16-tests
branch
from
18d85d0 to
4d1f174
Compare
|
|
||
| gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "6.6.0",') | ||
| # Switch to the oldest supported React version | ||
| gsub_file_content("../package.json", /"react": "[^"]*",/, '"react": "16.14.0",') |
There was a problem hiding this comment.
16.0.0 doesn't work due to missing createContext, 16.3.0 doesn't work due to missing memo (I think both are in the dummy app specifically, not in the NPM package, so it doesn't mean much for its compatibility).
I could try to bisect the earliest working version, but I don't see much point; I think it's enough to show we can work with some 16.x.y.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
script/convert (1)
39-43: Consider alternative approaches for filtering tests.The current approach of modifying the Jest command with ignore patterns works, but as previously suggested, alternative approaches like test groups or descriptive tags might be more maintainable long-term.
If you choose to implement test groups with jest-runner-groups, you'd need to:
- Add
jest-runner-groupsto your dependencies- Configure test groups in your Jest config
- Mark tests with group annotations instead of relying on filename patterns
🧹 Nitpick comments (3)
spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx (1)
17-19: Consider avoiding thedeleteoperator for better performanceThe
deleteoperator can impact performance. Consider using object destructuring instead.- const render = props.prerender ? ReactDOM.hydrate : ReactDOM.render; - // eslint-disable-next-line no-param-reassign - delete props.prerender; + const { prerender, ...restProps } = props; + const render = prerender ? ReactDOM.hydrate : ReactDOM.render;🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
spec/dummy/client/app-react16/startup/ReduxApp.client.jsx (2)
25-25: Consider replacing delete with undefined assignment.The delete operator can impact performance. While not critical in this non-loop context, using an undefined assignment is generally preferred.
- delete props.prerender; + props.prerender = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
39-39: Consider using standard naming convention for components.React component variables conventionally start with a capital letter. While
Komponentworks,Componentwould better follow React naming conventions.- const renderApp = (Komponent) => { + const renderApp = (Component) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
eslint.config.ts(1 hunks)knip.ts(1 hunks)script/convert(1 hunks)spec/dummy/client/app-react16/startup/ManualRenderApp.jsx(1 hunks)spec/dummy/client/app-react16/startup/ReduxApp.client.jsx(1 hunks)spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/dummy/client/app-react16/startup/ReduxApp.client.jsx (3)
spec/dummy/client/app-react16/startup/ManualRenderApp.jsx (1)
props(4-18)spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx (4)
props(16-45)render(17-17)store(22-22)renderApp(29-36)spec/dummy/client/app/components/HelloWorldContainer.jsx (1)
HelloWorldContainer(10-12)
🪛 Biome (1.9.4)
spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx
[error] 19-19: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
[error] 25-25: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (7)
knip.ts (1)
57-57: LGTM: Properly ignores React 16 test filesThis change excludes the React 16 test files from Knip's analysis, which is appropriate since these files intentionally use older React patterns that might trigger unnecessary warnings.
eslint.config.ts (1)
187-192: LGTM: Appropriate ESLint configuration for React 16 filesDisabling the
react/no-deprecatedrule specifically for React 16 test files is the correct approach. This allows the tests to use APIs that might be deprecated in newer React versions while maintaining strict linting for the rest of the codebase.spec/dummy/client/app-react16/startup/ManualRenderApp.jsx (1)
1-18: LGTM: Clean implementation of React 16 manual renderingThis component correctly implements the React on Rails pattern for manual rendering with React 16, using the appropriate
ReactDOM.renderandReactDOM.hydratemethods based on whether the content was prerendered.spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx (2)
1-46: Overall implementation looks goodThe component correctly implements a Redux-connected React component with shared store and hot module replacement support for React 16.
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
9-9:✅ Verification successful
Verify HelloWorldContainer compatibility with React 16
The component is importing
HelloWorldContainerfrom outside the React 16 directory. Ensure that this component is compatible with React 16.
🏁 Script executed:
#!/bin/bash # Check if HelloWorldContainer uses any React APIs not compatible with React 16 rg -A 5 -B 5 "createRoot|hydrateRoot|useId|useDeferredValue|useTransition|useSyncExternalStore" client/app/components/HelloWorldContainer.jsxLength of output: 229
🏁 Script executed:
#!/bin/bash # Find HelloWorldContainer file(s) and search for React 16 incompatible APIs files=$(fd HelloWorldContainer.* -t f) if [ -z "$files" ]; then echo "No HelloWorldContainer file found." exit 1 fi for file in $files; do echo "Inspecting $file..." rg -A 5 -B 5 'createRoot|hydrateRoot|useId|useDeferredValue|useTransition|useSyncExternalStore' "$file" || echo "No matches in $file" echo doneLength of output: 1693
HelloWorldContainer compatibility verified for React 16
No React 18‑only APIs (
createRoot,hydrateRoot,useId,useDeferredValue,useTransition,useSyncExternalStore) are used in the importedHelloWorldContainer(spec/dummy/client/app/components/HelloWorldContainer.jsx). This component is safe to use with React 16.script/convert (2)
11-15: Well-designed path handling function.The
movemethod properly handles path expansion, making the file operations more robust when dealing with relative paths.
34-38: Appropriate React version pinning for compatibility testing.Pinning React and ReactDOM to version 16.14.0 aligns perfectly with the PR objective of ensuring React 16 support.
| if (module.hot) { | ||
| module.hot.accept(['../components/HelloWorldContainer'], () => { | ||
| renderApp(HelloWorldContainer); | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify HMR path for the HelloWorldContainer component
The hot module replacement configuration is accepting changes from ../components/HelloWorldContainer, but the import path is ../../app/components/HelloWorldContainer. This mismatch might prevent hot reloading from working correctly.
🏁 Script executed:
#!/bin/bash
# Check the directory structure to understand the correct path for HMR
find client -name "HelloWorldContainer*" | sortLength of output: 91
🏁 Script executed:
#!/bin/bash
# Locate the HelloWorldContainer component file
find spec -type f -iname "HelloWorldContainer.*" -print
# Show import and HMR usage in the startup file
rg "HelloWorldContainer" -n spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsxLength of output: 517
Update HMR path to match import for HelloWorldContainer
The hot‑reload hook is currently watching
module.hot.accept(['../components/HelloWorldContainer'], …but the component is actually imported via
import HelloWorldContainer from '../../app/components/HelloWorldContainer';From
spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx
you need to adjust the HMR path to the same relative location:
- if (module.hot) {
- module.hot.accept(['../components/HelloWorldContainer'], () => {
- renderApp(HelloWorldContainer);
- });
- }
+ if (module.hot) {
+ module.hot.accept(['../../app/components/HelloWorldContainer'], () => {
+ renderApp(HelloWorldContainer);
+ });
+ }This will ensure hot‑module replacement picks up changes to your container.
There was a problem hiding this comment.
The change isn't needed because the file will be copied to /app first.
There was a problem hiding this comment.
@alexeyr-ci2 Thank you for the clarification. I understand now - since the file gets copied from app-react16/startup/ to /app/startup/ during the conversion process, the relative path for HMR will actually be correct at runtime. The current path ../components/HelloWorldContainer is properly configured for where the file will exist when executed, not its current location in the source.
✏️ Learnings added
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.813Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| renderApp(HelloWorldContainer); | ||
|
|
||
| if (module.hot) { | ||
| module.hot.accept(['../reducers/reducersIndex', '../components/HelloWorldContainer'], () => { |
There was a problem hiding this comment.
Fix incorrect HMR module paths.
The hot module replacement paths don't match the actual import paths, which will break HMR for reducers and components.
- module.hot.accept(['../reducers/reducersIndex', '../components/HelloWorldContainer'], () => {
+ module.hot.accept(['../../app/reducers/reducersIndex', '../../app/components/HelloWorldContainer'], () => {
store.replaceReducer(combineReducers(reducers));
renderApp(HelloWorldContainer);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| module.hot.accept(['../reducers/reducersIndex', '../components/HelloWorldContainer'], () => { | |
| module.hot.accept( | |
| ['../../app/reducers/reducersIndex', '../../app/components/HelloWorldContainer'], | |
| () => { | |
| store.replaceReducer(combineReducers(reducers)); | |
| renderApp(HelloWorldContainer); | |
| } | |
| ); |
| gsub_file_content( | ||
| "../package.json", | ||
| "jest node_package/tests", | ||
| 'jest node_package/tests --testPathIgnorePatterns=\".*(RSC|stream|serverRenderReactComponent).*\"' |
There was a problem hiding this comment.
If what you have works for now, I say we use it.
Summary
Make sure we don't accidentally break React 16 support.
Closes #1679.
Pull Request checklist
[ ] Update documentation[ ] Update CHANGELOG fileThis change is
Summary by CodeRabbit