Skip to content

Add tests for React 16 to CI #1732

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

Merged
merged 2 commits into from
Apr 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions eslint.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,12 @@ const config = tsEslint.config([
'@typescript-eslint/restrict-template-expressions': 'off',
},
},
{
files: ['**/app-react16/**/*'],
rules: {
'react/no-deprecated': 'off',
},
},
// must be the last config in the array
// https://github.com/prettier/eslint-plugin-prettier?tab=readme-ov-file#configuration-new-eslintconfigjs
prettierRecommended,
Expand Down
1 change: 1 addition & 0 deletions knip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const config: KnipConfig = {
// Declaring this as webpack.config instead doesn't work correctly
'config/webpack/webpack.config.js',
],
ignore: ['**/app-react16/**/*'],
project: ['**/*.{js,cjs,mjs,jsx,ts,cts,mts,tsx}!', 'config/webpack/*.js'],
paths: {
'Assets/*': ['client/app/assets/*'],
Expand Down
33 changes: 27 additions & 6 deletions script/convert
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,47 @@ def gsub_file_content(path, old_content, new_content)
File.binwrite(path, content)
end

old_config = File.expand_path("../spec/dummy/config/shakapacker.yml", __dir__)
new_config = File.expand_path("../spec/dummy/config/webpacker.yml", __dir__)
def move(old_path, new_path)
old_path = File.expand_path(old_path, __dir__)
new_path = File.expand_path(new_path, __dir__)
File.rename(old_path, new_path)
end

File.rename(old_config, new_config)
move("../spec/dummy/config/shakapacker.yml", "../spec/dummy/config/webpacker.yml")

# Shakapacker
gsub_file_content("../Gemfile.development_dependencies", /gem "shakapacker", "[^"]*"/, 'gem "shakapacker", "6.6.0"')
gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "6.6.0",')

# The below packages don't work on the oldest supported Node version and aren't needed there anyway
gsub_file_content("../package.json", /"eslint": "[^"]*",/, "")
gsub_file_content("../package.json", /"globals": "[^"]*",/, "")
gsub_file_content("../package.json", /"knip": "[^"]*",/, "")
gsub_file_content("../package.json", /"publint": "[^"]*",/, "")
gsub_file_content("../package.json", /"typescript-eslint": "[^"]*",/, "")
gsub_file_content("../package.json", %r{"@arethetypeswrong/cli": "[^"]*",}, "")
gsub_file_content("../package.json", %r{"@eslint/compat": "[^"]*",}, "")
gsub_file_content("../package.json", %r{"@testing-library/dom": "[^"]*",}, "")
gsub_file_content("../package.json", %r{"@testing-library/react": "[^"]*",}, "")
gsub_file_content("../package.json", /"knip": "[^"]*",/, "")
gsub_file_content("../package.json", /"publint": "[^"]*",/, "")

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",')
Copy link
Collaborator Author

@alexeyr-ci2 alexeyr-ci2 Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

gsub_file_content("../package.json", /"react-dom": "[^"]*",/, '"react-dom": "16.14.0",')
gsub_file_content("../spec/dummy/package.json", /"react": "[^"]*",/, '"react": "16.14.0",')
gsub_file_content("../spec/dummy/package.json", /"react-dom": "[^"]*",/, '"react-dom": "16.14.0",')
gsub_file_content(
"../package.json",
"jest node_package/tests",
'jest node_package/tests --testPathIgnorePatterns=\".*(RSC|stream|serverRenderReactComponent).*\"'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this we can either:

  1. Include something like #react18 in the description of these tests and filter them out;
  2. Use https://www.npmjs.com/package/jest-runner-groups.

Any preference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If what you have works for now, I say we use it.

)
gsub_file_content("../tsconfig.json", "react-jsx", "react")
gsub_file_content("../spec/dummy/babel.config.js", "runtime: 'automatic'", "runtime: 'classic'")
# https://rescript-lang.org/docs/react/latest/migrate-react#configuration
gsub_file_content("../spec/dummy/rescript.json", '"version": 4', '"version": 4, "mode": "classic"')
# Find all files under app-react16 and replace the React 19 versions
Dir.glob(File.expand_path("../spec/dummy/**/app-react16/**/*.*", __dir__)).each do |file|
move(file, file.gsub("-react16", ""))
end

gsub_file_content("../spec/dummy/config/webpack/commonWebpackConfig.js", /generateWebpackConfig(\(\))?/,
"webpackConfig")
Expand Down
18 changes: 18 additions & 0 deletions spec/dummy/client/app-react16/startup/ManualRenderApp.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React from 'react';
import ReactDOM from 'react-dom';

export default (props, _railsContext, domNodeId) => {
const reactElement = (
<div>
<h1 id="manual-render">Manual Render Example</h1>
<p>If you can see this, you can register renderer functions.</p>
</div>
);

const domNode = document.getElementById(domNodeId);
if (props.prerender) {
ReactDOM.hydrate(reactElement, domNode);
} else {
ReactDOM.render(reactElement, domNode);
}
};
57 changes: 57 additions & 0 deletions spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Top level component for client side.
// Compare this to the ./ServerApp.jsx file which is used for server side rendering.
// NOTE: these are basically the same, but they are shown here

import React from 'react';
import { combineReducers, applyMiddleware, createStore } from 'redux';
import { Provider } from 'react-redux';
import thunkMiddleware from 'redux-thunk';
import ReactDOM from 'react-dom';

import reducers from '../../app/reducers/reducersIndex';
import composeInitialState from '../../app/store/composeInitialState';

import HelloWorldContainer from '../../app/components/HelloWorldContainer';

/*
* Export a function that takes the props and returns a ReactComponent.
* This is used for the client rendering hook after the page html is rendered.
* React will see that the state is the same and not do anything.
*
*/
export default (props, railsContext, domNodeId) => {
const render = props.prerender ? ReactDOM.hydrate : ReactDOM.render;
// eslint-disable-next-line no-param-reassign
delete props.prerender;

const combinedReducer = combineReducers(reducers);
const combinedProps = composeInitialState(props, railsContext);

// This is where we'll put in the middleware for the async function. Placeholder.
// store will have helloWorldData as a top level property
const store = createStore(combinedReducer, combinedProps, applyMiddleware(thunkMiddleware));

// renderApp is a function required for hot reloading. see
// https://github.com/retroalgic/react-on-rails-hot-minimal/blob/master/client/src/entry.js

// Provider uses this.props.children, so we're not typical React syntax.
// This allows redux to add additional props to the HelloWorldContainer.
const renderApp = (Komponent) => {
const element = (
<Provider store={store}>
<Komponent />
</Provider>
);

render(element, document.getElementById(domNodeId));
};

renderApp(HelloWorldContainer);

if (module.hot) {
module.hot.accept(['../reducers/reducersIndex', '../components/HelloWorldContainer'], () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
module.hot.accept(['../reducers/reducersIndex', '../components/HelloWorldContainer'], () => {
module.hot.accept(
['../../app/reducers/reducersIndex', '../../app/components/HelloWorldContainer'],
() => {
store.replaceReducer(combineReducers(reducers));
renderApp(HelloWorldContainer);
}
);

store.replaceReducer(combineReducers(reducers));
renderApp(HelloWorldContainer);
});
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Top level component for the client side.
// Compare this to the ./ReduxSharedStoreApp.server.jsx file which is used for server side rendering.

import React from 'react';
import { Provider } from 'react-redux';
import ReactOnRails from 'react-on-rails/client';
import ReactDOM from 'react-dom';

import HelloWorldContainer from '../../app/components/HelloWorldContainer';

/*
* Export a function that returns a ReactComponent, depending on a store named SharedReduxStore.
* This is used for the client rendering hook after the page html is rendered.
* React will see that the state is the same and not do anything.
*/
export default (props, _railsContext, domNodeId) => {
const render = props.prerender ? ReactDOM.hydrate : ReactDOM.render;
// eslint-disable-next-line no-param-reassign
delete props.prerender;

// This is where we get the existing store.
const store = ReactOnRails.getStore('SharedReduxStore');

// renderApp is a function required for hot reloading. see
// https://github.com/retroalgic/react-on-rails-hot-minimal/blob/master/client/src/entry.js

// Provider uses this.props.children, so we're not typical React syntax.
// This allows redux to add additional props to the HelloWorldContainer.
const renderApp = (Component) => {
const element = (
<Provider store={store}>
<Component />
</Provider>
);
render(element, document.getElementById(domNodeId));
};

renderApp(HelloWorldContainer);

if (module.hot) {
module.hot.accept(['../components/HelloWorldContainer'], () => {
renderApp(HelloWorldContainer);
});
}
Comment on lines +40 to +44
Copy link
Contributor

@coderabbitai coderabbitai bot Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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*" | sort

Length 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.jsx

Length 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change isn't needed because the file will be copied to /app first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

};
Loading