Resolve 14.1.1 incompatibility with eslint & make sure that spec/dummy is linted by eslint #1693
Conversation
WalkthroughThis pull request introduces a series of configuration and code modifications across multiple files in the project. The changes primarily focus on ESLint configurations, component prop type validations, and minor structural adjustments in React components. The updates aim to improve code quality, type safety, and project structure, with specific attention to linting rules, import statements, and component prop handling. Changes
Suggested reviewers
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
spec/dummy/client/app/startup/HelloWorldHooksContext.jsx (2)
Line range hint
9-23: Consider extracting Result component to a separate file.While the restructuring improves hooks usage, the inner
Resultcomponent could be moved to its own file to improve maintainability and reusability.
25-29: PropTypes should be moved with the Result component.If the
Resultcomponent is extracted, ensure the PropTypes validation moves with it.spec/dummy/client/app/startup/HelloWorldProps.jsx (1)
Line range hint
11-14: Consider simplifying the delayed value implementation.The current implementation using state and useEffect could be simplified since
delayedValueis only set once and never updated.- const [delayedValue, setDelayedValue] = useState(null); - - useEffect(() => { - setDelayedValue(props.modificationTarget); - }, []); + const delayedValue = React.useMemo(() => props.modificationTarget, []);.eslintrc (1)
49-50: Consider documenting the rationale for disabled rules.While disabling these rules might be necessary for your use case, it would be helpful to document why these specific rules are disabled:
- react/forbid-prop-types
- jsx-a11y/anchor-is-valid
- react/forbid-prop-types: 0 - jsx-a11y/anchor-is-valid: 0 + # Disabled because we use compound prop types in legacy components + react/forbid-prop-types: 0 + # Disabled because we handle anchor clicks via custom handlers + jsx-a11y/anchor-is-valid: 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
.eslintignore(1 hunks).eslintrc(1 hunks)CHANGELOG.md(1 hunks)package.json(3 hunks)spec/dummy/babel.config.js(1 hunks)spec/dummy/client/.eslintignore(0 hunks)spec/dummy/client/.eslintrc(0 hunks)spec/dummy/client/app/components/RouterLayout.jsx(1 hunks)spec/dummy/client/app/packs/rescript-components.js(1 hunks)spec/dummy/client/app/packs/server-bundle.js(1 hunks)spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx(2 hunks)spec/dummy/client/app/startup/HelloTurboStream.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldHooks.jsx(2 hunks)spec/dummy/client/app/startup/HelloWorldHooksContext.jsx(2 hunks)spec/dummy/client/app/startup/HelloWorldProps.jsx(2 hunks)spec/dummy/client/app/startup/ImageExample.jsx(1 hunks)spec/dummy/client/app/startup/RouterApp.server.jsx(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(2 hunks)spec/dummy/config/webpack/serverWebpackConfig.js(4 hunks)spec/dummy/config/webpack/webpack.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- spec/dummy/client/.eslintignore
- spec/dummy/client/.eslintrc
✅ Files skipped from review due to trivial changes (4)
- spec/dummy/client/app/packs/server-bundle.js
- spec/dummy/client/app/startup/HelloTurboStream.jsx
- spec/dummy/client/app/startup/RouterApp.server.jsx
- spec/dummy/config/webpack/commonWebpackConfig.js
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: examples (oldest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (19)
spec/dummy/client/app/packs/rescript-components.js (1)
7-7: Verify all import statements for HelloWorldReScript component.The change from named export to default export requires updating all import statements from
import { HelloWorldReScript }toimport HelloWorldReScript.✅ Verification successful
Export change is compatible with existing imports ✅
All existing imports of
HelloWorldReScriptare already using the default import syntax, so no updates are required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining destructured imports of HelloWorldReScript rg "import \{ .*HelloWorldReScript.* \}" spec/dummy/Length of output: 54
Script:
#!/bin/bash # Search for any imports containing HelloWorldReScript echo "=== Searching for all HelloWorldReScript imports ===" rg "import.*HelloWorldReScript" -A 2 # Search for any usage of HelloWorldReScript in JSX/TSX files echo -e "\n=== Searching for HelloWorldReScript usage in components ===" rg "<HelloWorldReScript" -A 2Length of output: 857
spec/dummy/client/app/startup/HelloWorldHooks.jsx (1)
6-6: Address the TODO comment.The comment "TODO: make more like the HelloWorld.jsx" should be addressed or removed if no longer applicable.
Would you like me to help implement the changes to make this component more similar to HelloWorld.jsx?
spec/dummy/client/app/components/RouterLayout.jsx (1)
6-6: LGTM! Clean refactor removing unused children prop.The removal of the unused children prop and its PropTypes definition improves code clarity. The router implementation remains correct and follows React Router best practices.
spec/dummy/babel.config.js (2)
1-1: LGTM! Import path updated to follow ESLint conventions.The removal of the
.jsextension aligns with common ESLint import rules.
3-3: LGTM! Named function improves code clarity.Replacing the anonymous function with
createBabelConfigenhances debugging and stack traces.spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx (2)
15-19: LGTM! Added PropTypes validation.The addition of PropTypes improves type checking and aligns with ESLint best practices.
Line range hint
6-13: Verify if this incorrect implementation is intentional.The component's implementation takes two parameters but doesn't wrap them correctly as noted in the comments below. If this is meant to be a test case for invalid implementations, please add a comment at the top of the file explaining this purpose.
✅ Verification successful
Implementation is intentionally incorrect as a test case
The file contains explicit comments indicating this is an example of incorrect implementation:
- Top comment states: "Example of incorrectly taking two params and returning JSX"
- Detailed comments explain the correct implementation that would use a wrapped function
- Located in
spec/dummydirectory alongside other test cases🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files or documentation referencing this component rg -l "ContextFunctionReturnInvalidJSX" --type=testLength of output: 80
Script:
#!/bin/bash # Check the content of the file for comments echo "=== File content ===" cat spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx echo -e "\n=== Similar files in directory ===" ls -1 spec/dummy/client/app/startup/ echo -e "\n=== References in JS/JSX files ===" rg "ContextFunctionReturnInvalidJSX" -t js -t jsxLength of output: 2161
spec/dummy/client/app/startup/HelloWorldProps.jsx (1)
28-33: LGTM! Added comprehensive PropTypes validation.The addition of PropTypes for both
helloWorldDataandmodificationTargetimproves type safety.spec/dummy/client/app/startup/ImageExample.jsx (2)
7-10: LGTM! Import statements are now properly organized.The imports are now logically grouped and placed at the top of the file, improving code organization.
12-12: Good cleanup: Removed unused props parameter.Removing the unused
_propsparameter improves code clarity since the component doesn't use any props.spec/dummy/config/webpack/serverWebpackConfig.js (3)
31-33: LGTM! Properly documented ESLint rule exception.The ESLint disable comment is correctly scoped to the specific line where parameter reassignment is necessary.
Line range hint
72-81: LGTM! Well-structured CSS loader filtering logic.The code properly filters out mini-css-extract-plugin and style-loader while maintaining clear logic flow.
98-99: LGTM! Properly documented ESLint rule exception for SSR file handling.The ESLint disable comment is correctly scoped to the specific line where parameter reassignment is needed for SSR file handling.
.eslintignore (1)
4-10: LGTM! Well-structured ESLint ignore patterns.The changes properly:
- Remove the broad spec/dummy ignore to enable linting
- Add specific ignore patterns for build artifacts, vendor files, and generated code
- Maintain proper linting coverage while excluding appropriate files
This aligns well with the PR objective to ensure spec/dummy is linted by ESLint.
.eslintrc (2)
44-44: LGTM! Proper extension handling configuration.The import/extensions rule is properly configured to handle both JS and JSX files.
56-58: LGTM! Good configuration for module resolution.The changes:
- Add proper alias for Assets directory
- Include .jsx in the extensions list for proper module resolution
package.json (2)
30-30: LGTM: ESLint import resolver additionThe addition of
eslint-import-resolver-aliasis appropriate for resolving the ESLint compatibility issues mentioned in the PR objectives.
Line range hint
42-43: Verify React version compatibilityThe package.json specifies React version ^19.0.0, but React 19 has not been officially released yet. This could lead to compatibility issues.
CHANGELOG.md (1)
21-24: LGTM: Well-formatted changelog entryThe changelog entry is properly formatted, accurately describes the changes, and provides appropriate attribution to the contributor.
| // eslint-disable-next-line import/no-dynamic-require, global-require | ||
| return require(path); | ||
| } else { | ||
| return generateWebpackConfig(); | ||
| } | ||
| return generateWebpackConfig(); |
There was a problem hiding this comment.
Review the control flow logic.
The current implementation might have unintended behavior:
- If the environment-specific config file exists, it's required but the result isn't used
generateWebpackConfig()is always called regardless ofexistsSyncresult
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.
| // 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(); | |
| }; |
| HelloWorldHooks.propTypes = { | ||
| helloWorldData: PropTypes.shape({ | ||
| name: PropTypes.string, | ||
| }).isRequired, | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| HelloWorldHooks.propTypes = { | |
| helloWorldData: PropTypes.shape({ | |
| name: PropTypes.string, | |
| }).isRequired, | |
| } | |
| HelloWorldHooks.propTypes = { | |
| helloWorldData: PropTypes.shape({ | |
| name: PropTypes.string.isRequired, | |
| }).isRequired, | |
| } |
See title.
Summary
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
Bug Fixes
Chores
Documentation
Refactor