Skip to content
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

Resolve 14.1.1 incompatibility with eslint & make sure that spec/dummy is linted by eslint #1693

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
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
10 changes: 7 additions & 3 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
node_modules
coverage
spec/react_on_rails/dummy-for-generators
spec/dummy
spec/dummy/.yalc
spec/dummy/public
spec/dummy/vendor
spec/dummy/tmp
spec/dummy/app/assets/config/manifest.js
spec/dummy/client/node_modules
spec/dummy/client/app/components/HelloWorldReScript.res.js
node_package/lib/
node_package/tests/node_modules
node_package/webpack.config.js
**/node_modules/**
**/assets/webpack/**
**/public/webpack/**
Expand Down
8 changes: 6 additions & 2 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,18 @@ rules:
object-curly-newline: 0
no-restricted-syntax: ["error", "SequenceExpression"]
# https://stackoverflow.com/a/59268871/5241481
import/extensions: ['error', 'ignorePackages', {"js": 'never',"ts": "never"}]
import/extensions: ['error', 'ignorePackages', {"js": 'never', "jsx": 'never', "ts": "never", " ": "never"}]

# https://github.com/benmosher/eslint-plugin-import/issues/340
import/no-extraneous-dependencies: 0

react/forbid-prop-types: 0
jsx-a11y/anchor-is-valid: 0

settings:
import/core-modules:
- react-redux
import/resolver:
alias: [ ["Assets", "./spec/dummy/client/app/assets"] ]
node:
extensions: [".js", ".ts", ".d.ts"]
extensions: [".js", ".jsx", ".ts", ".d.ts"]
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ Please follow the recommendations outlined at [keepachangelog.com](http://keepac
### [Unreleased]
Changes since the last non-beta release.

#### Fixed

- Resolved 14.1.1 incompatibility with eslint & made sure that spec/dummy is linted by eslint. [PR 1693](https://github.com/shakacode/react_on_rails/pull/1693) by [judahmeek](https://github.com/judahmeek).

### [14.1.1] - 2025-01-15

#### Fixed
Expand Down
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"name": "react-on-rails",
"version": "14.1.1",
"description": "react-on-rails JavaScript for react_on_rails Ruby gem",
"main": "node_package/lib/ReactOnRails.js",
"exports": {
".": {
"node": "./node_package/lib/ReactOnRails.node.js",
Expand All @@ -26,6 +27,7 @@
"eslint": "^7.32.0",
"eslint-config-prettier": "^7.0.0",
"eslint-config-shakacode": "^16.0.1",
"eslint-import-resolver-alias": "^1.1.2",
"eslint-plugin-import": "^2.29.1",
"eslint-plugin-jsx-a11y": "^6.8.0",
"eslint-plugin-prettier": "^3.4.1",
Expand All @@ -42,8 +44,7 @@
"ts-jest": "^29.2.5",
"typescript": "^5.6.2"
},
"dependencies": {
},
"dependencies": {},
"peerDependencies": {
"react": ">= 16",
"react-dom": ">= 16"
Expand Down
5 changes: 3 additions & 2 deletions spec/dummy/babel.config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module.exports = function (api) {
const defaultConfigFunc = require('shakapacker/package/babel/preset.js');
const defaultConfigFunc = require('shakapacker/package/babel/preset');

module.exports = function createBabelConfig(api) {
const resultConfig = defaultConfigFunc(api);
const isProductionEnv = api.env('production');
const isDevelopmentEnv = api.env('development');
Expand Down
1 change: 0 additions & 1 deletion spec/dummy/client/.eslintignore

This file was deleted.

12 changes: 0 additions & 12 deletions spec/dummy/client/.eslintrc

This file was deleted.

7 changes: 1 addition & 6 deletions spec/dummy/client/app/components/RouterLayout.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import PropTypes from 'prop-types';
import React from 'react';
import { Link, Route, Switch } from 'react-router-dom';
import RouterFirstPage from './RouterFirstPage';
import RouterSecondPage from './RouterSecondPage';

const RouterLayout = ({ children }) => (
const RouterLayout = () => (
<div className="container">
<h1>React Router is working!</h1>
<p>
Expand All @@ -29,8 +28,4 @@ const RouterLayout = ({ children }) => (
</div>
);

RouterLayout.propTypes = {
children: PropTypes.object,
};

export default RouterLayout;
2 changes: 1 addition & 1 deletion spec/dummy/client/app/packs/rescript-components.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@

import HelloWorldReScript from '../components/HelloWorldReScript.res.js';

export { HelloWorldReScript };
export default HelloWorldReScript;
4 changes: 2 additions & 2 deletions spec/dummy/client/app/packs/server-bundle.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// import statement added by react_on_rails:generate_packs rake task
import './../generated/server-bundle-generated.js';
// Shows the mapping from the exported object to the name used by the server rendering.
import ReactOnRails from 'react-on-rails';
// import statement added by react_on_rails:generate_packs rake task
import './../generated/server-bundle-generated';
// Example of server rendering with no React
import HelloString from '../non_react/HelloString';

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Example of incorrectly taking two params and returning JSX
import React, { useState } from 'react';
import React from 'react';
import PropTypes from 'prop-types';
import css from '../components/HelloWorld.module.scss';
import RailsContext from '../components/RailsContext';

Expand All @@ -11,6 +12,12 @@ const ContextFunctionReturnInvalidJSX = (props, railsContext) => (
</>
);

ContextFunctionReturnInvalidJSX.propTypes = {
helloWorldData: PropTypes.shape({
name: PropTypes.string,
}).isRequired,
}

/* Wrapping in a function would be correct in this case, since two params
are passed to the registered function:

Expand Down
2 changes: 1 addition & 1 deletion spec/dummy/client/app/startup/HelloTurboStream.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import css from '../components/HelloWorld.module.scss';
const HelloTurboStream = ({ helloTurboStreamData, railsContext }) => {
const [name, setName] = useState(helloTurboStreamData.name);
const nameDomRef = useRef(null);

// eslint-disable-next-line no-unused-vars
const handleChange = () => {
setName(nameDomRef.current.value);
};
Expand Down
7 changes: 7 additions & 0 deletions spec/dummy/client/app/startup/HelloWorldHooks.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Super simple example of the simplest possible React component
import React, { useState } from 'react';
import PropTypes from 'prop-types';
import css from '../components/HelloWorld.module.scss';

// TODO: make more like the HelloWorld.jsx
Expand All @@ -16,4 +17,10 @@ function HelloWorldHooks(props) {
);
}

HelloWorldHooks.propTypes = {
helloWorldData: PropTypes.shape({
name: PropTypes.string,
}).isRequired,
}
Comment on lines +20 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Strengthen PropTypes validation.

The name property should be marked as required since it's used directly in the component.

 HelloWorldHooks.propTypes = {
   helloWorldData: PropTypes.shape({
-      name: PropTypes.string,
+      name: PropTypes.string.isRequired,
    }).isRequired,
 }
📝 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
HelloWorldHooks.propTypes = {
helloWorldData: PropTypes.shape({
name: PropTypes.string,
}).isRequired,
}
HelloWorldHooks.propTypes = {
helloWorldData: PropTypes.shape({
name: PropTypes.string.isRequired,
}).isRequired,
}


export default HelloWorldHooks;
13 changes: 11 additions & 2 deletions spec/dummy/client/app/startup/HelloWorldHooksContext.jsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
// Example of using hooks when taking the props and railsContext
// Note, you need the call the hooks API within the react component stateless function
import React, { useState } from 'react';
import PropTypes from 'prop-types';
import css from '../components/HelloWorld.module.scss';
import RailsContext from '../components/RailsContext';

const HelloWorldHooksContext = (props, railsContext) => {
// You could pass props here or use the closure
return () => {
const HelloWorldHooksContext = (props, railsContext) => {
const Result = () => {
const [name, setName] = useState(props.helloWorldData.name);
return (
<>
Expand All @@ -20,6 +21,14 @@ const HelloWorldHooksContext = (props, railsContext) => {
</>
);
};

Result.propTypes = {
helloWorldData: PropTypes.shape({
name: PropTypes.string,
}).isRequired,
};

return Result;
};

export default HelloWorldHooksContext;
12 changes: 10 additions & 2 deletions spec/dummy/client/app/startup/HelloWorldProps.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React, { useState, useEffect } from 'react';
import PropTypes from 'prop-types';
import css from '../components/HelloWorld.module.scss';

function HelloWorldHooks(props) {
function HelloWorldProps(props) {
console.log(`HelloWorldProps modification target prop value: ${props.modificationTarget}`);

const [name, setName] = useState(props.helloWorldData.name);
Expand All @@ -24,4 +25,11 @@ function HelloWorldHooks(props) {
);
}

export default HelloWorldHooks;
HelloWorldProps.propTypes = {
helloWorldData: PropTypes.shape({
name: PropTypes.string,
}).isRequired,
modificationTarget: PropTypes.string.isRequired,
}

export default HelloWorldProps;
10 changes: 6 additions & 4 deletions spec/dummy/client/app/startup/ImageExample.jsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import React from 'react';
import css from '../components/ImageExample/ImageExample.module.scss';

// Note the global alias for images
import bowerLogo from '../components/ImageExample/bower.png';
import blueprintIcon from '../components/ImageExample/blueprint_icon.svg';
import logo from 'Assets/images/256egghead.png';
import legoIcon from 'Assets/images/lego_icon.svg';

const TestComponent = (_props) => (
import css from '../components/ImageExample/ImageExample.module.scss';

import bowerLogo from '../components/ImageExample/bower.png';
import blueprintIcon from '../components/ImageExample/blueprint_icon.svg';

const TestComponent = () => (
<div>
<h1 className={css.red}>This is a test of CSS module color red.</h1>
<hr />
Expand Down
12 changes: 5 additions & 7 deletions spec/dummy/client/app/startup/RouterApp.server.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,8 @@ import { StaticRouter } from 'react-router-dom';

import routes from '../routes/routes';

export default (props, railsContext) => {
return () => (
<StaticRouter location={railsContext.location} {...props}>
{routes}
</StaticRouter>
);
};
export default (props, railsContext) => () => (
<StaticRouter location={railsContext.location} {...props}>
{routes}
</StaticRouter>
);
4 changes: 2 additions & 2 deletions spec/dummy/config/webpack/commonWebpackConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const baseClientWebpackConfig = generateWebpackConfig();

const webpack = require('webpack');

const aliasConfig = require('./alias.js');
const aliasConfig = require('./alias');

const commonOptions = {
resolve: {
Expand All @@ -23,7 +23,7 @@ const sassLoaderConfig = {

const scssConfigIndex = baseClientWebpackConfig.module.rules.findIndex((config) =>
'.scss'.match(config.test),
);
); // eslint-disable-next-line no-undef
baseClientWebpackConfig.module.rules[scssConfigIndex]?.use.push(sassLoaderConfig);

// add jquery
Expand Down
10 changes: 5 additions & 5 deletions spec/dummy/config/webpack/serverWebpackConfig.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { merge, config } = require('shakapacker');
const { config } = require('shakapacker');
const commonWebpackConfig = require('./commonWebpackConfig');

const webpack = require('webpack');
Expand Down Expand Up @@ -28,7 +28,7 @@ const configureServer = () => {
// replace file-loader with null-loader
serverWebpackConfig.module.rules.forEach((loader) => {
if (loader.use && loader.use.filter) {
loader.use = loader.use.filter(
loader.use = loader.use.filter( // eslint-disable-line no-param-reassign
(item) => !(typeof item === 'string' && item.match(/mini-css-extract-plugin/)),
);
}
Expand Down Expand Up @@ -65,11 +65,11 @@ const configureServer = () => {
// Remove the mini-css-extract-plugin from the style loaders because
// the client build will handle exporting CSS.
// replace file-loader with null-loader
const rules = serverWebpackConfig.module.rules;
const { rules } = serverWebpackConfig.module;
rules.forEach((rule) => {
if (Array.isArray(rule.use)) {
// remove the mini-css-extract-plugin and style-loader
rule.use = rule.use.filter((item) => {
rule.use = rule.use.filter((item) => { // eslint-disable-line no-param-reassign
let testValue;
if (typeof item === 'string') {
testValue = item;
Expand All @@ -95,7 +95,7 @@ const configureServer = () => {

// Skip writing image files during SSR by setting emitFile to false
} else if (rule.use && (rule.use.loader === 'url-loader' || rule.use.loader === 'file-loader')) {
rule.use.options.emitFile = false;
rule.use.options.emitFile = false; // eslint-disable-line no-param-reassign
}
});

Expand Down
4 changes: 2 additions & 2 deletions spec/dummy/config/webpack/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ const envSpecificConfig = () => {
const path = resolve(__dirname, `${env.nodeEnv}.js`);
if (existsSync(path)) {
console.log(`Loading ENV specific webpack configuration file ${path}`);
// eslint-disable-next-line import/no-dynamic-require, global-require
return require(path);
} else {
return generateWebpackConfig();
}
return generateWebpackConfig();
Comment on lines +10 to +13
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

Review the control flow logic.

The current implementation might have unintended behavior:

  1. If the environment-specific config file exists, it's required but the result isn't used
  2. generateWebpackConfig() is always called regardless of existsSync result

Consider this fix:

 const envSpecificConfig = () => {
   const path = resolve(__dirname, `${env.nodeEnv}.js`);
   if (existsSync(path)) {
     console.log(`Loading ENV specific webpack configuration file ${path}`);
     // eslint-disable-next-line import/no-dynamic-require, global-require
-    return require(path);
+    const envConfig = require(path);
+    return envConfig || generateWebpackConfig();
   }
   return generateWebpackConfig();
 };
📝 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
// eslint-disable-next-line import/no-dynamic-require, global-require
return require(path);
} else {
return generateWebpackConfig();
}
return generateWebpackConfig();
const envSpecificConfig = () => {
const path = resolve(__dirname, `${env.nodeEnv}.js`);
if (existsSync(path)) {
console.log(`Loading ENV specific webpack configuration file ${path}`);
// eslint-disable-next-line import/no-dynamic-require, global-require
const envConfig = require(path);
return envConfig || generateWebpackConfig();
}
return generateWebpackConfig();
};

};

module.exports = envSpecificConfig();
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2992,6 +2992,11 @@ eslint-config-shakacode@^16.0.1:
babel-eslint "8.2.2"
eslint-config-airbnb "16.1.0"

eslint-import-resolver-alias@^1.1.2:
version "1.1.2"
resolved "https://registry.yarnpkg.com/eslint-import-resolver-alias/-/eslint-import-resolver-alias-1.1.2.tgz#297062890e31e4d6651eb5eba9534e1f6e68fc97"
integrity sha512-WdviM1Eu834zsfjHtcGHtGfcu+F30Od3V7I9Fi57uhBEwPkjDcii7/yW8jAT+gOhn4P/vOxxNAXbFAKsrrc15w==

eslint-import-resolver-node@^0.3.9:
version "0.3.9"
resolved "https://registry.yarnpkg.com/eslint-import-resolver-node/-/eslint-import-resolver-node-0.3.9.tgz#d4eaac52b8a2e7c3cd1903eb00f7e053356118ac"
Expand Down
Loading