Upgrade ESLint #1713
Upgrade ESLint #1713
Conversation
WalkthroughThis pull request removes legacy ESLint configuration files and related ignore files while introducing a new TypeScript-based ESLint configuration. Package dependencies related to linting have been upgraded and reconfigured. Several source and test files have received modifications in their ESLint directive comments, parameter destructuring, and internal state management. Minor adjustments were also made to workflows and build scripts, ensuring updated lint processes and cleaner code directives without affecting core functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant ESLint as ESLint Config
participant Code as Source Files
participant Test as Test Files
Dev->>ESLint: Remove legacy configs\nand add new TS-config
ESLint-->>Code: Apply updated linting rules
Code->>Test: Update ESLint directives\nand syntax adjustments
Test-->>Dev: Cleaner output & warnings reported
sequenceDiagram
participant Renderer as ComponentRenderer/StoreRenderer
participant Client as Application Client
participant State as Internal State
Client->>Renderer: Request render/hydration
Renderer->>State: Update state (unmounted → active)
State-->>Renderer: Confirm state change
Renderer-->>Client: Complete asynchronous operation
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (2)
spec/dummy/client/app/components/HelloWorldRedux.jsx (1)
22-23: Consider using destructuring to avoid disabling ESLint ruleWhile the ESLint directive works as a short-term solution, consider destructuring
actionsfrom props at the beginning of the method to follow the rule consistently.handleChange() { const name = this.nameDomRef.value; - // eslint-disable-next-line react/destructuring-assignment - this.props.actions.updateName(name); + const { actions } = this.props; + actions.updateName(name); }spec/dummy/client/app/components/RailsContext.jsx (1)
6-6: Consider adding ESLint disable comment for console.logLine 6 contains a console.log statement which will likely trigger ESLint warnings with the upgraded ESLint configuration. Since this appears to be demonstration code, you might want to add an appropriate ESLint disable comment.
-console.log('railsContext.serverSide is ', railsContext.serverSide); +// eslint-disable-next-line no-console +console.log('railsContext.serverSide is ', railsContext.serverSide);
📜 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 (42)
.eslintignore(0 hunks).eslintrc(0 hunks)eslint.config.ts(1 hunks)node_package/src/CallbackRegistry.ts(1 hunks)node_package/src/ClientSideRenderer.ts(3 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/ReactOnRails.node.ts(1 hunks)node_package/src/clientStartup.ts(1 hunks)node_package/src/createReactOutput.ts(0 hunks)node_package/src/reactHydrateOrRender.ts(2 hunks)node_package/src/types/index.ts(0 hunks)node_package/tests/ComponentRegistry.test.js(0 hunks)node_package/tests/RSCClientRoot.test.jsx(0 hunks)node_package/tests/ReactOnRails.test.jsx(0 hunks)node_package/tests/renderFunction.test.jsx(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(6 hunks)package-scripts.yml(1 hunks)package.json(3 hunks)script/convert(1 hunks)spec/dummy/client/app/components/HelloWorldRedux.jsx(1 hunks)spec/dummy/client/app/components/RailsContext.jsx(2 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/reducers/HelloWorldReducer.jsx(1 hunks)spec/dummy/client/app/reducers/nullReducer.jsx(1 hunks)spec/dummy/client/app/startup/CacheDisabled.jsx(0 hunks)spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx(1 hunks)spec/dummy/client/app/startup/CssModulesImagesFontsExample.jsx(0 hunks)spec/dummy/client/app/startup/HelloWorld.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldES5.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldHooks.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldProps.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldReScript.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldRehydratable.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldWithLogAndThrow.jsx(0 hunks)spec/dummy/client/app/startup/PureComponent.jsx(1 hunks)spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx(1 hunks)spec/dummy/client/app/store/composeInitialState.js(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)spec/dummy/config/webpack/serverWebpackConfig.js(1 hunks)spec/dummy/config/webpack/webpackConfig.js(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (11)
- spec/dummy/client/app/startup/CssModulesImagesFontsExample.jsx
- node_package/tests/ComponentRegistry.test.js
- node_package/tests/RSCClientRoot.test.jsx
- node_package/src/createReactOutput.ts
- spec/dummy/config/webpack/webpackConfig.js
- node_package/src/types/index.ts
- node_package/tests/ReactOnRails.test.jsx
- spec/dummy/client/app/startup/HelloWorldWithLogAndThrow.jsx
- spec/dummy/client/app/startup/CacheDisabled.jsx
- .eslintignore
- .eslintrc
🧰 Additional context used
🪛 GitHub Check: build
eslint.config.ts
[failure] 5-5:
Module '"/home/runner/work/react_on_rails/react_on_rails/node_modules/@eslint/js/types/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag
[failure] 3-3:
Module '"/home/runner/work/react_on_rails/react_on_rails/node_modules/globals/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag
[failure] 2-2:
Module '"/home/runner/work/react_on_rails/react_on_rails/node_modules/eslint-plugin-prettier/recommended"' can only be default-imported using the 'allowSyntheticDefaultImports' flag
🪛 GitHub Actions: Lint JS and Ruby
eslint.config.ts
[error] 2-2: error TS1259: Module '"/home/runner/work/react_on_rails/react_on_rails/node_modules/eslint-plugin-prettier/recommended"' can only be default-imported using the 'allowSyntheticDefaultImports' flag
🔇 Additional comments (45)
spec/dummy/config/webpack/serverWebpackConfig.js (1)
3-3: LGTM: Import reordering for consistencyThe reordering of the import statement improves code organization, following the standard practice of placing related imports together. This change aligns well with the ESLint upgrades in this PR.
spec/dummy/client/app/packs/server-bundle.js (1)
2-2: Import path correction looks goodThe updated path for importing the server-bundle-generated.js file now correctly uses a relative path with
../generated/instead of./. This fixes the import path while maintaining the necessary ESLint directive.script/convert (1)
19-19: Good addition for ESLint upgrade migrationThis new line appropriately removes the old ESLint package reference from package.json during the conversion process, which aligns with the PR's objective to upgrade ESLint to version 9.
spec/dummy/client/app/reducers/HelloWorldReducer.jsx (1)
11-11: Improved function signature for better error handlingAdding a default value of
undefinedfor theactionparameter is a good defensive programming practice. This prevents potential runtime errors when destructuring properties fromactionon line 12 if the function is called without providing an action argument.The change is consistent with the ESLint upgrade, which likely enforces stricter type-checking and error prevention patterns.
spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx (1)
34-38: Improved naming convention for React component parameterThe change from
KomponenttoComponentfollows standard React naming conventions, making the code more idiomatic and easier to understand.eslint.config.ts (1)
14-142: Comprehensive ESLint configuration with modern flat config formatThis new configuration provides a well-structured setup using ESLint's flat config format, which is the recommended approach for ESLint v9. The configuration properly handles both JavaScript and TypeScript files, includes appropriate rule settings, and integrates Prettier for consistent formatting.
spec/dummy/client/app/packs/rescript-components.js (1)
5-5: Clean removal of unnecessary ESLint directiveThe removal of the ESLint directive comment (
// eslint-disable-next-line import/no-unresolved) shows that the new ESLint configuration properly handles ReScript imports without requiring manual disabling of rules.spec/dummy/config/webpack/commonWebpackConfig.js (1)
26-26: Clean removal of unnecessary ESLint directiveThe removal of the ESLint directive comment (
// eslint-disable-next-line no-undef) indicates that the new ESLint configuration properly understands the scope and context of this code, eliminating the need for manual rule disabling.tsconfig.json (1)
17-17: Including eslint.config.ts in TypeScript compilation is appropriate.Including the ESLint configuration file in TypeScript compilation ensures it receives proper type checking and validation, which is important since the PR objective is upgrading ESLint to version 9 with TypeScript-ESLint version 8.
node_package/src/ReactOnRails.node.ts (1)
7-8: Default export addition with proper ESLint directive.The added default export enhances module usability, and the ESLint directive is well-documented with a reference to the relevant GitHub issue that explains why the rule needs to be disabled.
spec/dummy/client/app/startup/PureComponent.jsx (1)
4-4: Improved props handling with destructuring.Using destructuring in the function parameters makes the code more concise and immediately shows which props the component expects. This is a modern React best practice and aligns with the ESLint upgrade objectives.
node_package/src/ReactOnRails.client.ts (1)
342-342: Modernized object copying syntax.Replacing
Object.assign({}, DEFAULT_OPTIONS)with the spread operator{ ...DEFAULT_OPTIONS }is a good modernization that maintains the same functionality while using more concise ES6+ syntax. This aligns well with the ESLint upgrade goals.node_package/tests/renderFunction.test.jsx (1)
3-3: Appropriate ESLint configuration for test fileThe addition of the
max-classes-per-fileESLint disable directive is appropriate since this test file necessarily defines multiple React component classes to test different component types.spec/dummy/client/app/startup/HelloWorldReScript.jsx (1)
1-1: Clean import statement improves readabilityGood cleanup by removing the unnecessary ESLint disable comment. This suggests the updated ESLint configuration now properly handles ReScript imports without needing special exceptions.
spec/dummy/client/app/startup/HelloWorld.jsx (1)
30-32: Method reordering maintains functionalityThe relocation of the
setNameDomRefmethod doesn't affect functionality and may improve code consistency across components.spec/dummy/client/app/components/HelloWorldRedux.jsx (1)
26-28: Method reordering maintains functionalityThe relocation of the
setNameDomRefmethod doesn't affect functionality and improves consistency with other components in the codebase.package-scripts.yml (1)
9-9: Command simplified to align with ESLint flat configThe simplification of the ESLint command is appropriate as part of the ESLint v9 upgrade, which uses the new flat config format. With the new
eslint.config.tsfile, file extensions and directory specifications are handled in the configuration rather than in the command line arguments.spec/dummy/client/app/store/composeInitialState.js (1)
3-3: Improved code with modern JS syntaxThe change from
Object.assign()to object spread syntax improves readability while maintaining the same functionality. This modern syntax is consistent with current JavaScript best practices and likely better aligns with the updated ESLint configuration.node_package/src/clientStartup.ts (1)
34-35:✅ Verification successful
Verify "no-param-reassign" rule compliance
The ESLint disable comment has been modified to only suppress "no-underscore-dangle" while removing "no-param-reassign". Line 35 modifies a property of the
contextparameter, which typically triggers the no-param-reassign rule. Please verify this change won't cause linting errors.
🏁 Script executed:
#!/bin/bash # Check if the modified code triggers ESLint no-param-reassign rule echo "Checking if line causes ESLint errors with no-param-reassign rule" echo 'function test(context) { context.__PROP__ = true; }' > /tmp/test-param-reassign.ts npx eslint /tmp/test-param-reassign.ts --format json | grep -q "no-param-reassign" && echo "ESLint error: no-param-reassign rule triggered" || echo "No ESLint errors found"Length of output: 339
ESLint Rule Verification Passed – No Action Required
The modification to only suppress
no-underscore-danglehas been verified. A test confirming that modifying a property on thecontextparameter does not trigger theno-param-reassignrule has passed. No lint errors occur with the updated ESLint directive, so further changes are not necessary.spec/dummy/client/app/reducers/nullReducer.jsx (1)
7-7: Improved handling of unused parametersThe change from
actionto_action = undefinedfollows the convention of prefixing unused parameters with an underscore, which is recognized by ESLint. This allows the removal of the previous ESLint disable comment while making the code intent more explicit.spec/dummy/client/app/startup/HelloWorldES5.jsx (3)
9-9: LGTM: ESLint disable comment for unused class component methodsThe addition of this ESLint disable comment is appropriate since the component is intentionally using the older createReactClass syntax for demonstration purposes, and ESLint v9 would flag this method otherwise.
15-15: LGTM: ESLint disable comment for destructuring assignmentThis ESLint disable comment correctly suppresses the destructuring-assignment rule warning. Since this is a demonstration of ES5-style React components, it's appropriate to maintain the direct
this.propsaccess pattern rather than using destructuring.
24-26: LGTM: Reintroduced setNameDomRef methodThe reintroduction of the
setNameDomRefmethod is necessary as it's being used in the component's render method (line 36). This change ensures the component functions properly and avoids runtime errors.spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx (2)
7-7: LGTM: Improved function parameter destructuringThe change to destructure
helloWorldDatadirectly from props is a good modernization that aligns with current JavaScript best practices. This makes the code more readable and maintainable.
9-9: LGTM: Updated property access to use destructured parameterThis change correctly updates the property access to use the destructured
helloWorldDataparameter instead of accessing it viaprops.helloWorldData. This change is consistent with the parameter destructuring on line 7.spec/dummy/client/app/startup/HelloWorldHooks.jsx (2)
7-7: LGTM: Improved function parameter destructuringThe change to destructure
helloWorldDatadirectly from props follows modern JavaScript best practices and improves code readability.
8-8: LGTM: Updated property access to use destructured parameterThis change correctly updates the useState initialization to use the destructured
helloWorldDataparameter, maintaining consistency with the parameter destructuring on line 7.spec/dummy/client/app/components/RailsContext.jsx (2)
26-26: LGTM: Improved function parameter destructuringThe change to destructure
railsContextdirectly from props follows modern JavaScript best practices and improves code readability.
38-38: LGTM: Updated property access to use destructured parameterThis change correctly updates the function call to use the destructured
railsContextparameter, maintaining consistency with the parameter destructuring on line 26.node_package/tests/streamServerRenderedReactComponent.test.jsx (3)
11-13: Improved promise construction formatThe promise construction has been reformatted from a single-line arrow function to a more explicit block format. This change enhances code readability while maintaining the same functionality.
20-22: Added PropTypes for AsyncContent componentGood addition of prop type validation for the AsyncContent component. This improves type safety and makes the component API more self-documenting, consistent with the PropTypes already defined for TestComponentForStreaming.
84-86: Standardized promise handling in test casesAll promise constructions in test cases have been consistently reformatted from single-line arrow functions to block format. This change improves readability and maintains a consistent coding style throughout the file.
Also applies to: 103-105, 119-121, 135-137, 156-158
spec/dummy/client/app/startup/HelloWorldRehydratable.jsx (1)
31-34: Re-added handleChange methodThe handleChange method has been re-added with the same implementation as before. This method correctly updates the component's state with the input value and is properly bound in the constructor.
spec/dummy/client/app/startup/HelloWorldProps.jsx (2)
5-6: Updated function signature with prop destructuringThe function signature has been improved to use destructuring, directly extracting
helloWorldDataandmodificationTargetfrom props. This makes the component's dependencies more explicit and follows modern React best practices.Also applies to: 8-8
13-14: Fixed useEffect dependency arrayImportant improvement: the useEffect hook now properly includes
modificationTargetin its dependency array. This ensures the effect runs when this prop changes, not just on initial mount, adhering to React's recommended patterns for the useEffect hook.node_package/src/reactHydrateOrRender.ts (2)
17-19: Updated ESLint directives and unused variable handlingThe ESLint directive has been updated to use TypeScript-specific rules, and the error variable is now prefixed with an underscore to indicate it's intentionally unused. These changes align with modern TypeScript and linting best practices.
26-26: Added React deprecated API handling commentsAdded appropriate ESLint rule disabling with explanatory comment about React 16 support requirements, and properly re-enabled the rule after the affected code. This follows best practices for acknowledging and documenting the use of deprecated APIs where they're still necessary.
Also applies to: 41-41
node_package/src/ClientSideRenderer.ts (3)
1-2: Appropriate ESLint disable directives added.These ESLint disable comments address potential issues with the new ESLint configuration while maintaining backward compatibility with React 16.
47-48: Improved type safety with union type for state.The explicit type annotation for
stateproperty ('unmounted' | 'rendering' | 'rendered') enhances type safety by clearly defining the possible states for the component renderer.The addition of the
renderPromiseproperty ensures that the promise handling in the constructor has proper type support.Also applies to: 50-51
175-176: Improved type safety for StoreRenderer state.The explicit type annotation for the
stateproperty ('unmounted' | 'hydrating' | 'hydrated') enhances type safety and code clarity by clearly defining the possible states for the store renderer.node_package/src/CallbackRegistry.ts (2)
17-22: Properly declared class instance properties with types.These explicit property declarations with appropriate TypeScript types improve code quality by:
- Making the class structure more explicit and self-documenting
- Ensuring type safety for these previously implicit properties
- Satisfying the stricter linting rules in ESLint 9
This is a good practice that helps prevent runtime errors and improves maintainability.
24-26: Added explicit state tracking properties with proper types.The addition of the
timeoutEventsInitializedandtimedoutboolean properties properly formalizes state tracking that was previously implicit, improving code clarity and type safety while satisfying stricter ESLint rules.package.json (3)
22-25: Added new dependencies for improved linting capabilities.The new dependencies enhance the project's linting setup:
@babel/eslint-parserand@babel/preset-reactimprove React code parsing@eslint/compathelps with ESLint plugin compatibilityeslint-plugin-react-hooksadds essential linting rules for React hooksglobalsprovides global variable definitions for different environmentsThese additions are necessary for proper functioning of ESLint 9.
Also applies to: 44-45
36-43: Updated ESLint and related plugins to latest versions.The upgrade to ESLint 9 (from v7) represents a major version change that brings improved linting capabilities and performance. The related plugin updates ensure compatibility with the new ESLint version.
59-60: Upgraded TypeScript-ESLint integration.Replacing the separate
@typescript-eslint/eslint-pluginand@typescript-eslint/parserpackages with the unifiedtypescript-eslintpackage (v8.26.1) aligns with modern best practices and ensures compatibility with ESLint 9.
eslint.config.ts
Outdated
| globals: { | ||
| ...globals.browser, | ||
| ...globals.node, | ||
| ...globals.mocha, | ||
| ...globals.jest, | ||
| __DEBUG_SERVER_ERRORS__: true, | ||
| __SERVER_ERRORS__: true, | ||
| }, | ||
|
|
||
| parserOptions: { | ||
| requireConfigFile: false, | ||
|
|
||
| babelOptions: { | ||
| presets: ['@babel/preset-env', '@babel/preset-react'], | ||
| }, | ||
| }, | ||
| }, | ||
|
|
||
| settings: { | ||
| 'import/core-modules': ['react-redux'], | ||
|
|
||
| 'import/resolver': { | ||
| alias: [['Assets', './spec/dummy/client/app/assets']], | ||
|
|
||
| node: { | ||
| extensions: ['.js', '.jsx', '.ts', '.d.ts'], | ||
| }, | ||
| }, | ||
| }, | ||
|
|
||
| rules: { | ||
| 'no-shadow': 'off', | ||
| 'no-console': 'off', | ||
| 'function-paren-newline': 'off', | ||
| 'object-curly-newline': 'off', | ||
| 'no-restricted-syntax': ['error', 'SequenceExpression'], | ||
|
|
||
| 'import/extensions': [ | ||
| 'error', | ||
| 'ignorePackages', | ||
| { | ||
| js: 'never', | ||
| jsx: 'never', | ||
| ts: 'never', | ||
| }, | ||
| ], | ||
|
|
||
| 'import/first': 'off', | ||
| 'import/no-extraneous-dependencies': 'off', | ||
| // The rule seems broken: it's reporting errors on imports in files using `export` too, | ||
| // not just `module.exports`. | ||
| 'import/no-import-module-exports': 'off', | ||
| 'react/destructuring-assignment': [ | ||
| 'error', | ||
| 'always', | ||
| { | ||
| ignoreClassFields: true, | ||
| }, | ||
| ], | ||
| 'react/forbid-prop-types': 'off', | ||
| 'react/function-component-definition': [ | ||
| 'error', | ||
| { | ||
| namedComponents: ['arrow-function', 'function-declaration'], | ||
| unnamedComponents: 'arrow-function', | ||
| }, | ||
| ], | ||
| 'react/jsx-props-no-spreading': 'off', | ||
| 'react/static-property-placement': 'off', | ||
| 'jsx-a11y/anchor-is-valid': 'off', | ||
| }, | ||
| }, | ||
| { | ||
| files: ['**/*.ts', '**/*.tsx'], | ||
|
|
||
| extends: tsEslint.configs.recommended, | ||
|
|
||
| languageOptions: { | ||
| parserOptions: { | ||
| project: true, | ||
| }, | ||
| }, | ||
|
|
||
| rules: { | ||
| '@typescript-eslint/no-namespace': 'off', | ||
| '@typescript-eslint/no-shadow': 'error', | ||
| '@typescript-eslint/no-unused-vars': [ | ||
| 'error', | ||
| { | ||
| caughtErrorsIgnorePattern: '^_', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| // must be the last config in the array | ||
| // https://github.com/prettier/eslint-plugin-prettier?tab=readme-ov-file#configuration-new-eslintconfigjs | ||
| prettierRecommended, | ||
| ]); | ||
|
|
||
| export default config; |
There was a problem hiding this comment.
Fix TypeScript module import syntax
The pipeline is failing due to TypeScript import issues. The following imports need to be modified since they require the allowSyntheticDefaultImports flag:
-import prettierRecommended from 'eslint-plugin-prettier/recommended';
-import globals from 'globals';
-import js from '@eslint/js';
+import * as prettierRecommended from 'eslint-plugin-prettier/recommended';
+import * as globals from 'globals';
+import * as js from '@eslint/js';Alternatively, you could add allowSyntheticDefaultImports: true to the TypeScript configuration.
📝 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.
| import { globalIgnores } from 'eslint/config'; | |
| import prettierRecommended from 'eslint-plugin-prettier/recommended'; | |
| import globals from 'globals'; | |
| import tsEslint from 'typescript-eslint'; | |
| import js from '@eslint/js'; | |
| import { FlatCompat } from '@eslint/eslintrc'; | |
| const compat = new FlatCompat({ | |
| baseDirectory: __dirname, | |
| recommendedConfig: js.configs.recommended, | |
| allConfig: js.configs.all, | |
| }); | |
| const config = tsEslint.config([ | |
| globalIgnores([ | |
| 'lib/generators/react_on_rails/templates', | |
| 'node_package/lib/', | |
| 'spec/react_on_rails/dummy-for-generators', | |
| 'spec/dummy/.yalc', | |
| 'spec/dummy/public', | |
| 'spec/dummy/vendor', | |
| 'spec/dummy/tmp', | |
| 'spec/dummy/app/assets/config/manifest.js', | |
| '**/*.res.js', | |
| '**/coverage', | |
| '**/node_modules/**/*', | |
| '**/assets/webpack/**/*', | |
| '**/public/webpack/**/*', | |
| '**/generated/**/*', | |
| '**/app/assets/javascripts/application.js', | |
| '**/coverage/**/*', | |
| '**/cable.js', | |
| '**/public/packs*/*', | |
| '**/gen-examples', | |
| '**/bundle/', | |
| 'knip.ts', | |
| ]), | |
| { | |
| files: ['**/*.[jt]s', '**/*.[jt]sx', '**/*.[cm][jt]s'], | |
| }, | |
| js.configs.recommended, | |
| compat.extends('eslint-config-shakacode'), | |
| { | |
| languageOptions: { | |
| globals: { | |
| ...globals.browser, | |
| ...globals.node, | |
| ...globals.mocha, | |
| ...globals.jest, | |
| __DEBUG_SERVER_ERRORS__: true, | |
| __SERVER_ERRORS__: true, | |
| }, | |
| parserOptions: { | |
| requireConfigFile: false, | |
| babelOptions: { | |
| presets: ['@babel/preset-env', '@babel/preset-react'], | |
| }, | |
| }, | |
| }, | |
| settings: { | |
| 'import/core-modules': ['react-redux'], | |
| 'import/resolver': { | |
| alias: [['Assets', './spec/dummy/client/app/assets']], | |
| node: { | |
| extensions: ['.js', '.jsx', '.ts', '.d.ts'], | |
| }, | |
| }, | |
| }, | |
| rules: { | |
| 'no-shadow': 'off', | |
| 'no-console': 'off', | |
| 'function-paren-newline': 'off', | |
| 'object-curly-newline': 'off', | |
| 'no-restricted-syntax': ['error', 'SequenceExpression'], | |
| 'import/extensions': [ | |
| 'error', | |
| 'ignorePackages', | |
| { | |
| js: 'never', | |
| jsx: 'never', | |
| ts: 'never', | |
| }, | |
| ], | |
| 'import/first': 'off', | |
| 'import/no-extraneous-dependencies': 'off', | |
| // The rule seems broken: it's reporting errors on imports in files using `export` too, | |
| // not just `module.exports`. | |
| 'import/no-import-module-exports': 'off', | |
| 'react/destructuring-assignment': [ | |
| 'error', | |
| 'always', | |
| { | |
| ignoreClassFields: true, | |
| }, | |
| ], | |
| 'react/forbid-prop-types': 'off', | |
| 'react/function-component-definition': [ | |
| 'error', | |
| { | |
| namedComponents: ['arrow-function', 'function-declaration'], | |
| unnamedComponents: 'arrow-function', | |
| }, | |
| ], | |
| 'react/jsx-props-no-spreading': 'off', | |
| 'react/static-property-placement': 'off', | |
| 'jsx-a11y/anchor-is-valid': 'off', | |
| }, | |
| }, | |
| { | |
| files: ['**/*.ts', '**/*.tsx'], | |
| extends: tsEslint.configs.recommended, | |
| languageOptions: { | |
| parserOptions: { | |
| project: true, | |
| }, | |
| }, | |
| rules: { | |
| '@typescript-eslint/no-namespace': 'off', | |
| '@typescript-eslint/no-shadow': 'error', | |
| '@typescript-eslint/no-unused-vars': [ | |
| 'error', | |
| { | |
| caughtErrorsIgnorePattern: '^_', | |
| }, | |
| ], | |
| }, | |
| }, | |
| // must be the last config in the array | |
| // https://github.com/prettier/eslint-plugin-prettier?tab=readme-ov-file#configuration-new-eslintconfigjs | |
| prettierRecommended, | |
| ]); | |
| export default config; | |
| import { globalIgnores } from 'eslint/config'; | |
| import * as prettierRecommended from 'eslint-plugin-prettier/recommended'; | |
| import * as globals from 'globals'; | |
| import tsEslint from 'typescript-eslint'; | |
| import * as js from '@eslint/js'; | |
| import { FlatCompat } from '@eslint/eslintrc'; | |
| const compat = new FlatCompat({ | |
| baseDirectory: __dirname, | |
| recommendedConfig: js.configs.recommended, | |
| allConfig: js.configs.all, | |
| }); | |
| const config = tsEslint.config([ | |
| globalIgnores([ | |
| 'lib/generators/react_on_rails/templates', | |
| 'node_package/lib/', | |
| 'spec/react_on_rails/dummy-for-generators', | |
| 'spec/dummy/.yalc', | |
| 'spec/dummy/public', | |
| 'spec/dummy/vendor', | |
| 'spec/dummy/tmp', | |
| 'spec/dummy/app/assets/config/manifest.js', | |
| '**/*.res.js', | |
| '**/coverage', | |
| '**/node_modules/**/*', | |
| '**/assets/webpack/**/*', | |
| '**/public/webpack/**/*', | |
| '**/generated/**/*', | |
| '**/app/assets/javascripts/application.js', | |
| '**/coverage/**/*', | |
| '**/cable.js', | |
| '**/public/packs*/*', | |
| '**/gen-examples', | |
| '**/bundle/', | |
| 'knip.ts', | |
| ]), | |
| { | |
| files: ['**/*.[jt]s', '**/*.[jt]sx', '**/*.[cm][jt]s'], | |
| }, | |
| js.configs.recommended, | |
| compat.extends('eslint-config-shakacode'), | |
| { | |
| languageOptions: { | |
| globals: { | |
| ...globals.browser, | |
| ...globals.node, | |
| ...globals.mocha, | |
| ...globals.jest, | |
| __DEBUG_SERVER_ERRORS__: true, | |
| __SERVER_ERRORS__: true, | |
| }, | |
| parserOptions: { | |
| requireConfigFile: false, | |
| babelOptions: { | |
| presets: ['@babel/preset-env', '@babel/preset-react'], | |
| }, | |
| }, | |
| }, | |
| settings: { | |
| 'import/core-modules': ['react-redux'], | |
| 'import/resolver': { | |
| alias: [['Assets', './spec/dummy/client/app/assets']], | |
| node: { | |
| extensions: ['.js', '.jsx', '.ts', '.d.ts'], | |
| }, | |
| }, | |
| }, | |
| rules: { | |
| 'no-shadow': 'off', | |
| 'no-console': 'off', | |
| 'function-paren-newline': 'off', | |
| 'object-curly-newline': 'off', | |
| 'no-restricted-syntax': ['error', 'SequenceExpression'], | |
| 'import/extensions': [ | |
| 'error', | |
| 'ignorePackages', | |
| { | |
| js: 'never', | |
| jsx: 'never', | |
| ts: 'never', | |
| }, | |
| ], | |
| 'import/first': 'off', | |
| 'import/no-extraneous-dependencies': 'off', | |
| // The rule seems broken: it's reporting errors on imports in files using `export` too, | |
| // not just `module.exports`. | |
| 'import/no-import-module-exports': 'off', | |
| 'react/destructuring-assignment': [ | |
| 'error', | |
| 'always', | |
| { | |
| ignoreClassFields: true, | |
| }, | |
| ], | |
| 'react/forbid-prop-types': 'off', | |
| 'react/function-component-definition': [ | |
| 'error', | |
| { | |
| namedComponents: ['arrow-function', 'function-declaration'], | |
| unnamedComponents: 'arrow-function', | |
| }, | |
| ], | |
| 'react/jsx-props-no-spreading': 'off', | |
| 'react/static-property-placement': 'off', | |
| 'jsx-a11y/anchor-is-valid': 'off', | |
| }, | |
| }, | |
| { | |
| files: ['**/*.ts', '**/*.tsx'], | |
| extends: tsEslint.configs.recommended, | |
| languageOptions: { | |
| parserOptions: { | |
| project: true, | |
| }, | |
| }, | |
| rules: { | |
| '@typescript-eslint/no-namespace': 'off', | |
| '@typescript-eslint/no-shadow': 'error', | |
| '@typescript-eslint/no-unused-vars': [ | |
| 'error', | |
| { | |
| caughtErrorsIgnorePattern: '^_', | |
| }, | |
| ], | |
| }, | |
| }, | |
| // must be the last config in the array | |
| // https://github.com/prettier/eslint-plugin-prettier?tab=readme-ov-file#configuration-new-eslintconfigjs | |
| prettierRecommended, | |
| ]); | |
| export default config; |
🧰 Tools
🪛 GitHub Check: build
[failure] 5-5:
Module '"/home/runner/work/react_on_rails/react_on_rails/node_modules/@eslint/js/types/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag
[failure] 3-3:
Module '"/home/runner/work/react_on_rails/react_on_rails/node_modules/globals/index"' can only be default-imported using the 'allowSyntheticDefaultImports' flag
[failure] 2-2:
Module '"/home/runner/work/react_on_rails/react_on_rails/node_modules/eslint-plugin-prettier/recommended"' can only be default-imported using the 'allowSyntheticDefaultImports' flag
🪛 GitHub Actions: Lint JS and Ruby
[error] 2-2: error TS1259: Module '"/home/runner/work/react_on_rails/react_on_rails/node_modules/eslint-plugin-prettier/recommended"' can only be default-imported using the 'allowSyntheticDefaultImports' flag
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
eslint.config.ts (1)
2-5:⚠️ Potential issueFix TypeScript module import syntax
The current default imports will cause TypeScript compilation errors as indicated in the previous pipeline failures. These modules need namespace imports:
-import prettierRecommended from 'eslint-plugin-prettier/recommended'; -import globals from 'globals'; -import js from '@eslint/js'; +import * as prettierRecommended from 'eslint-plugin-prettier/recommended'; +import * as globals from 'globals'; +import * as js from '@eslint/js';Alternatively, you could add
allowSyntheticDefaultImports: trueto the TypeScript configuration.
🧹 Nitpick comments (2)
eslint.config.ts (2)
36-38: Note about Node 16 compatibilityThe comment indicates these files are excluded because they can't be compiled under Node 16. Consider documenting this requirement in README or package.json for future maintainers.
79-79: Consider restricting console usage in production code
no-consoleis currently turned off. Consider using a more targeted approach that allows console in development but restricts it in production:- 'no-console': 'off', + 'no-console': process.env.NODE_ENV === 'production' ? 'error' : 'warn',This would help prevent accidentally leaving debug statements in production code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.prettierignore(0 hunks)eslint.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .prettierignore
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (2)
eslint.config.ts (2)
8-12: Update FlatCompat initialization when fixing importsIf you change the
jsimport to a namespace import as suggested above, you'll need to update this initialization to reference the correct path to the configs:const compat = new FlatCompat({ baseDirectory: __dirname, - recommendedConfig: js.configs.recommended, - allConfig: js.configs.all, + recommendedConfig: js.configs.recommended, + allConfig: js.configs.all, });
14-145: Well-structured ESLint configurationThe configuration is comprehensive and well-organized, covering:
- Global ignores for files that shouldn't be linted
- Different rules for JS and TS files
- Integration with Prettier
- Support for various environments (browser, Node, testing)
This successfully consolidates previous .eslintrc and .eslintignore files into a single TypeScript configuration file, which is the recommended approach for ESLint 9.
alexeyr-ci
force-pushed
the
alexeyr/upgrade-eslint
branch
2 times, most recently
from
a4df83b to
325c5b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
eslint.config.ts (1)
2-5:⚠️ Potential issueFix TypeScript module import syntax
The pipeline is still failing due to TypeScript import issues. These modules don't have proper default exports and require namespace imports.
-import prettierRecommended from 'eslint-plugin-prettier/recommended'; -import globals from 'globals'; -import js from '@eslint/js'; +import * as prettierRecommended from 'eslint-plugin-prettier/recommended'; +import * as globals from 'globals'; +import * as js from '@eslint/js';Alternatively, you can add
allowSyntheticDefaultImports: trueto the TypeScript configuration.
🧹 Nitpick comments (2)
eslint.config.ts (2)
15-36: Consider organizing ignore patterns more effectivelyThe global ignore patterns list is quite extensive. Consider grouping them by type (generated files, vendor files, etc.) and adding comments to explain why certain patterns are ignored.
globalIgnores([ + // Generator templates and compiled code 'lib/generators/react_on_rails/templates', 'node_package/lib/', 'spec/react_on_rails/dummy-for-generators', + + // External and vendor directories 'spec/dummy/.yalc', 'spec/dummy/public', 'spec/dummy/vendor', 'spec/dummy/tmp', 'spec/dummy/app/assets/config/manifest.js', '**/*.res.js', + + // Test coverage and node modules '**/coverage', '**/node_modules/**/*', + + // Generated assets and webpack output '**/assets/webpack/**/*', '**/public/webpack/**/*', '**/generated/**/*', '**/app/assets/javascripts/application.js', '**/coverage/**/*', '**/cable.js', '**/public/packs*/*', '**/gen-examples', '**/bundle/', ]),
119-121: Add TypeScript type checking to ESLintFor more robust TypeScript linting, consider enabling type-aware rules.
extends: tsEslint.configs.recommended, +extends: [ + tsEslint.configs.recommended, + tsEslint.configs.recommendedTypeChecked, +], languageOptions: { parserOptions: {This will catch type-related issues during linting, making the development process more efficient.
📜 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 (44)
.eslintignore(0 hunks).eslintrc(0 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).prettierignore(0 hunks)eslint.config.ts(1 hunks)knip.ts(2 hunks)node_package/src/CallbackRegistry.ts(1 hunks)node_package/src/ClientSideRenderer.ts(3 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/ReactOnRails.node.ts(1 hunks)node_package/src/clientStartup.ts(1 hunks)node_package/src/createReactOutput.ts(0 hunks)node_package/src/reactHydrateOrRender.ts(2 hunks)node_package/src/types/index.ts(0 hunks)node_package/tests/ComponentRegistry.test.js(0 hunks)node_package/tests/RSCClientRoot.test.jsx(0 hunks)node_package/tests/ReactOnRails.test.jsx(0 hunks)node_package/tests/renderFunction.test.jsx(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(6 hunks)package-scripts.yml(1 hunks)package.json(3 hunks)script/convert(1 hunks)spec/dummy/client/app/components/HelloWorldRedux.jsx(1 hunks)spec/dummy/client/app/components/RailsContext.jsx(2 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/reducers/HelloWorldReducer.jsx(1 hunks)spec/dummy/client/app/reducers/nullReducer.jsx(1 hunks)spec/dummy/client/app/startup/CacheDisabled.jsx(0 hunks)spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx(1 hunks)spec/dummy/client/app/startup/CssModulesImagesFontsExample.jsx(0 hunks)spec/dummy/client/app/startup/HelloWorld.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldES5.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldHooks.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldProps.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldReScript.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldRehydratable.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldWithLogAndThrow.jsx(0 hunks)spec/dummy/client/app/startup/PureComponent.jsx(1 hunks)spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx(1 hunks)spec/dummy/client/app/store/composeInitialState.js(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)spec/dummy/config/webpack/serverWebpackConfig.js(1 hunks)spec/dummy/config/webpack/webpackConfig.js(0 hunks)
💤 Files with no reviewable changes (12)
- spec/dummy/config/webpack/webpackConfig.js
- node_package/src/types/index.ts
- .prettierignore
- spec/dummy/client/app/startup/CssModulesImagesFontsExample.jsx
- spec/dummy/client/app/startup/HelloWorldWithLogAndThrow.jsx
- node_package/tests/ComponentRegistry.test.js
- node_package/tests/RSCClientRoot.test.jsx
- node_package/tests/ReactOnRails.test.jsx
- spec/dummy/client/app/startup/CacheDisabled.jsx
- .eslintignore
- node_package/src/createReactOutput.ts
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (28)
- .github/workflows/lint-js-and-ruby.yml
- spec/dummy/client/app/reducers/HelloWorldReducer.jsx
- node_package/src/ReactOnRails.client.ts
- node_package/tests/renderFunction.test.jsx
- spec/dummy/config/webpack/commonWebpackConfig.js
- spec/dummy/client/app/startup/PureComponent.jsx
- node_package/src/ReactOnRails.node.ts
- spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx
- spec/dummy/client/app/startup/HelloWorld.jsx
- script/convert
- spec/dummy/client/app/startup/HelloWorldES5.jsx
- node_package/src/clientStartup.ts
- spec/dummy/config/webpack/serverWebpackConfig.js
- spec/dummy/client/app/store/composeInitialState.js
- node_package/src/reactHydrateOrRender.ts
- package-scripts.yml
- spec/dummy/client/app/components/RailsContext.jsx
- spec/dummy/client/app/reducers/nullReducer.jsx
- node_package/tests/streamServerRenderedReactComponent.test.jsx
- spec/dummy/client/app/startup/HelloWorldHooks.jsx
- spec/dummy/client/app/components/HelloWorldRedux.jsx
- spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx
- spec/dummy/client/app/startup/HelloWorldRehydratable.jsx
- knip.ts
- spec/dummy/client/app/startup/HelloWorldProps.jsx
- node_package/src/ClientSideRenderer.ts
- node_package/src/CallbackRegistry.ts
- package.json
🧰 Additional context used
🪛 GitHub Check: build
spec/dummy/client/app/packs/rescript-components.js
[failure] 5-5:
Unable to resolve path to module '../components/HelloWorldReScript.res.js'
spec/dummy/client/app/packs/server-bundle.js
[failure] 2-2:
'/home/runner/work/react_on_rails/react_on_rails/spec/dummy/client/app/generated/server-bundle-generated.js' imported multiple times
[failure] 2-2:
Useless path segments for "./../generated/server-bundle-generated.js", should be "../generated/server-bundle-generated.js"
[failure] 2-2:
Unexpected use of file extension "js" for "./../generated/server-bundle-generated.js"
[failure] 2-2:
Replace "./../generated/server-bundle-generated.js" with './../generated/server-bundle-generated.js';
spec/dummy/client/app/startup/HelloWorldReScript.jsx
[failure] 1-1:
Unable to resolve path to module '../components/HelloWorldReScript.res.js'
🪛 GitHub Actions: Lint JS and Ruby
spec/dummy/client/app/packs/rescript-components.js
[error] 5-5: Unable to resolve path to module '../components/HelloWorldReScript.res.js' import/no-unresolved
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
| @@ -1,5 +1,5 @@ | |||
| // import statement added by react_on_rails:generate_packs rake task | |||
| import './../generated/server-bundle-generated.js'; // eslint-disable-line import/extensions | |||
| import '../generated/server-bundle-generated.js'; // eslint-disable-line import/extensions | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix import path to resolve ESLint warnings
The import path has been changed, but static analysis shows multiple issues with this line:
- The path contains unnecessary segments (
./../can be simplified to../) - ESLint is flagging the use of file extension
- The file may be imported multiple times
-import '../generated/server-bundle-generated.js'; // eslint-disable-line import/extensions
+import '../generated/server-bundle-generated'; // eslint-disable-line import/extensionsIf removing the extension causes issues with the build process, add this path pattern to the import/extensions rule exceptions in the new ESLint config instead of using inline comments.
📝 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.
| import '../generated/server-bundle-generated.js'; // eslint-disable-line import/extensions | |
| import '../generated/server-bundle-generated'; // eslint-disable-line import/extensions |
🧰 Tools
🪛 GitHub Check: build
[failure] 2-2:
'/home/runner/work/react_on_rails/react_on_rails/spec/dummy/client/app/generated/server-bundle-generated.js' imported multiple times
[failure] 2-2:
Useless path segments for "./../generated/server-bundle-generated.js", should be "../generated/server-bundle-generated.js"
[failure] 2-2:
Unexpected use of file extension "js" for "./../generated/server-bundle-generated.js"
[failure] 2-2:
Replace "./../generated/server-bundle-generated.js" with './../generated/server-bundle-generated.js';
| @@ -2,6 +2,6 @@ | |||
| // As per the current configuration, all files are compiled to .res.js | |||
| // and they are generated on the same directory as the .res file | |||
|
|
|||
| import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; // eslint-disable-line import/no-unresolved | |||
| import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; | |||
There was a problem hiding this comment.
Fix ReScript import linting error
The ESLint directive has been removed, but the linting error still exists. The pipeline is failing with:
Unable to resolve path to module '../components/HelloWorldReScript.res.js'
Two options to fix this:
- Add back the directive temporarily until the eslint.config.ts is updated:
-import HelloWorldReScript from '../components/HelloWorldReScript.res.js';
+import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; // eslint-disable-line import/no-unresolved- Better solution: Update the ESLint config to handle ReScript imports (as suggested in the eslint.config.ts review).
📝 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.
| import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; | |
| import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; // eslint-disable-line import/no-unresolved |
🧰 Tools
🪛 GitHub Check: build
[failure] 5-5:
Unable to resolve path to module '../components/HelloWorldReScript.res.js'
🪛 GitHub Actions: Lint JS and Ruby
[error] 5-5: Unable to resolve path to module '../components/HelloWorldReScript.res.js' import/no-unresolved
| @@ -1,3 +1,3 @@ | |||
| import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; // eslint-disable-line import/no-unresolved | |||
| import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix ReScript import linting error
Similar to the issue in rescript-components.js, removing the ESLint directive hasn't fixed the underlying issue. Static analysis shows:
Unable to resolve path to module '../components/HelloWorldReScript.res.js'
Either:
- Add back the directive temporarily:
-import HelloWorldReScript from '../components/HelloWorldReScript.res.js';
+import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; // eslint-disable-line import/no-unresolved- Better solution: Update eslint.config.ts to handle ReScript imports globally.
🏁 Script executed:
#!/bin/bash
# Search for all ReScript imports in the codebase to identify similar patterns
echo "Finding all ReScript imports in the codebase:"
rg --type js "import.+\.res\.js" -lLength of output: 242
Resolve ReScript Import Linting Error
The static analysis error “Unable to resolve path to module '../components/HelloWorldReScript.res.js'” persists in this file (and similarly in spec/dummy/client/app/packs/rescript-components.js). To fix this, you have two options:
- Temporary workaround: Reintroduce the ESLint directive on the import, for example:
-import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; +import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; // eslint-disable-line import/no-unresolved
- Preferred long-term solution: Update your global ESLint configuration (i.e.
eslint.config.ts) to properly resolve modules with the.res.jsextension, which will address all similar cases across the repository.
Please apply one of these fixes to ensure the lint error is resolved uniformly across the codebase.
📝 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.
| import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; | |
| import HelloWorldReScript from '../components/HelloWorldReScript.res.js'; // eslint-disable-line import/no-unresolved |
🧰 Tools
🪛 GitHub Check: build
[failure] 1-1:
Unable to resolve path to module '../components/HelloWorldReScript.res.js'
alexeyr-ci
force-pushed
the
alexeyr/upgrade-eslint
branch
3 times, most recently
from
f416bc3 to
0bf5546
Compare
| "eslint-plugin-prettier": "^5.2.3", | ||
| "eslint-plugin-react": "^7.37.4", | ||
| "eslint-plugin-react-hooks": "^5.2.0", | ||
| "typescript-eslint": "^8.26.1", |
There was a problem hiding this comment.
Can't put it last where it would normally go alphabetically, or the oldest CI tests break :)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
eslint.config.ts (1)
2-5:⚠️ Potential issueFix TypeScript module import syntax
The imports are using default syntax without enabling
allowSyntheticDefaultImportsin TypeScript configuration, which will cause compilation errors as indicated in previous comments.Apply this fix:
-import prettierRecommended from 'eslint-plugin-prettier/recommended'; -import globals from 'globals'; -import js from '@eslint/js'; +import * as prettierRecommended from 'eslint-plugin-prettier/recommended'; +import * as globals from 'globals'; +import * as js from '@eslint/js';Alternatively, add
allowSyntheticDefaultImports: trueto your TypeScript configuration.
🧹 Nitpick comments (1)
eslint.config.ts (1)
46-63: Consider extracting globals configurationThe globals configuration is quite extensive. For maintainability, consider extracting it to a separate variable.
+const globalVariables = { + ...globals.browser, + ...globals.node, + ...globals.mocha, + ...globals.jest, + __DEBUG_SERVER_ERRORS__: true, + __SERVER_ERRORS__: true, +}; + { languageOptions: { - globals: { - ...globals.browser, - ...globals.node, - ...globals.mocha, - ...globals.jest, - __DEBUG_SERVER_ERRORS__: true, - __SERVER_ERRORS__: true, - }, + globals: globalVariables,
📜 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 (45)
.eslintignore(0 hunks).eslintrc(0 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/package-js-tests.yml(0 hunks).prettierignore(0 hunks)eslint.config.ts(1 hunks)knip.ts(2 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.js(1 hunks)node_package/src/CallbackRegistry.ts(1 hunks)node_package/src/ClientSideRenderer.ts(3 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/ReactOnRails.node.ts(1 hunks)node_package/src/clientStartup.ts(1 hunks)node_package/src/createReactOutput.ts(0 hunks)node_package/src/reactHydrateOrRender.ts(2 hunks)node_package/src/types/index.ts(0 hunks)node_package/tests/ComponentRegistry.test.js(0 hunks)node_package/tests/RSCClientRoot.test.jsx(0 hunks)node_package/tests/ReactOnRails.test.jsx(0 hunks)node_package/tests/renderFunction.test.jsx(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(6 hunks)package-scripts.yml(1 hunks)package.json(3 hunks)script/convert(1 hunks)spec/dummy/client/app/components/HelloWorldRedux.jsx(1 hunks)spec/dummy/client/app/components/RailsContext.jsx(2 hunks)spec/dummy/client/app/packs/rescript-components.js(1 hunks)spec/dummy/client/app/reducers/HelloWorldReducer.jsx(1 hunks)spec/dummy/client/app/reducers/nullReducer.jsx(1 hunks)spec/dummy/client/app/startup/CacheDisabled.jsx(0 hunks)spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx(1 hunks)spec/dummy/client/app/startup/CssModulesImagesFontsExample.jsx(0 hunks)spec/dummy/client/app/startup/HelloWorld.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldES5.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldHooks.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldProps.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldReScript.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldRehydratable.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldWithLogAndThrow.jsx(0 hunks)spec/dummy/client/app/startup/PureComponent.jsx(1 hunks)spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx(1 hunks)spec/dummy/client/app/store/composeInitialState.js(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)spec/dummy/config/webpack/serverWebpackConfig.js(1 hunks)spec/dummy/config/webpack/webpackConfig.js(0 hunks)
💤 Files with no reviewable changes (13)
- .prettierignore
- spec/dummy/client/app/startup/CssModulesImagesFontsExample.jsx
- node_package/src/createReactOutput.ts
- node_package/src/types/index.ts
- spec/dummy/config/webpack/webpackConfig.js
- node_package/tests/ComponentRegistry.test.js
- node_package/tests/ReactOnRails.test.jsx
- .eslintignore
- spec/dummy/client/app/startup/CacheDisabled.jsx
- .github/workflows/package-js-tests.yml
- node_package/tests/RSCClientRoot.test.jsx
- spec/dummy/client/app/startup/HelloWorldWithLogAndThrow.jsx
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (30)
- spec/dummy/client/app/packs/rescript-components.js
- node_package/src/ReactOnRails.node.ts
- spec/dummy/client/app/startup/HelloWorldReScript.jsx
- spec/dummy/client/app/components/HelloWorldRedux.jsx
- node_package/src/ReactOnRails.client.ts
- .github/workflows/lint-js-and-ruby.yml
- spec/dummy/config/webpack/commonWebpackConfig.js
- spec/dummy/client/app/startup/HelloWorld.jsx
- node_package/src/clientStartup.ts
- spec/dummy/client/app/reducers/HelloWorldReducer.jsx
- spec/dummy/client/app/store/composeInitialState.js
- spec/dummy/client/app/startup/HelloWorldES5.jsx
- knip.ts
- node_package/tests/renderFunction.test.jsx
- spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx
- spec/dummy/client/app/reducers/nullReducer.jsx
- spec/dummy/client/app/startup/PureComponent.jsx
- spec/dummy/client/app/startup/HelloWorldHooks.jsx
- script/convert
- spec/dummy/client/app/startup/HelloWorldProps.jsx
- node_package/tests/streamServerRenderedReactComponent.test.jsx
- package-scripts.yml
- spec/dummy/client/app/components/RailsContext.jsx
- spec/dummy/client/app/startup/HelloWorldRehydratable.jsx
- node_package/src/ClientSideRenderer.ts
- spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx
- node_package/src/CallbackRegistry.ts
- spec/dummy/config/webpack/serverWebpackConfig.js
- node_package/src/reactHydrateOrRender.ts
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: examples (oldest)
- GitHub Check: examples (newest)
🔇 Additional comments (9)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.js (1)
4-4: Good defensive programming practice!Adding a default empty object value for the
actionparameter is a smart defensive programming technique. This prevents potential errors when the reducer is called without an action argument (which can happen during initialization) and ensures that accessing properties likeaction.typewon't throw errors. This change aligns with Redux best practices and likely resolves ESLint warnings from the upgraded linting configuration.eslint.config.ts (8)
99-104: ReScript imports rule implementation looks goodThe rule for handling ReScript imports has been properly implemented as suggested in the previous review.
15-39: Comprehensive ignore patterns definedThe ignore patterns are well-organized with clear comments explaining the purpose of each exclusion. This provides good maintainability for future updates.
147-156: TypeScript rules look well-configuredThe TypeScript-specific rules are well thought out, especially the rule for ignoring unused variables in catch clauses if they start with underscore.
158-161: Correct Prettier configuration placementFollowing the recommendation to place Prettier configuration last in the array is the correct approach to avoid rule conflicts.
125-133: Clear documentation for template-specific rulesThe comments explaining why specific rules are disabled for generated templates are helpful and provide good context for future maintainers.
1-163: Overall configuration aligns with ESLint 9 best practicesThe configuration follows the new flat config format introduced in ESLint 9 and provides a comprehensive linting setup for both JavaScript and TypeScript files.
Once the import syntax issue is fixed, this configuration should provide a robust foundation for the codebase's linting needs.
139-145:❓ Verification inconclusive
Verify TypeScript project configuration
The configuration allows default projects for specific files, but there's no explicit
projectortsconfigRootDirsetting which might be needed for proper TypeScript linting.
🏁 Script executed:
#!/bin/bash # Check the existence of tsconfig.json in the repo find . -maxdepth 2 -name "tsconfig*.json" -not -path "*/node_modules/*" | while read -r file; do echo "Found TypeScript config: $file" cat "$file" | grep -E "compilerOptions|rootDir|allowSyntheticDefaultImports" doneLength of output: 304
Action: Review ESLint TypeScript Settings
The current configuration uses
allowDefaultProjectfor specific files, and a root-leveltsconfig.jsonis present. However, to ensure robust type-aware linting across your codebase, it’s advisable to explicitly configure the project path and, if necessary, settsconfigRootDir. For example:
- Add
"project": "./tsconfig.json"underparserOptions.- Specify
tsconfigRootDirif your repository structure requires it.Please verify that these changes align with your intended linting behavior.
41-42:❓ Verification inconclusive
Check for missing file extensions
Your file pattern includes common JavaScript and TypeScript extensions but might miss some niche cases.
🏁 Script executed:
#!/bin/bash # Check for any JS/TS files that might not be covered by the current glob pattern fd -e mjs -e cjs -e mjsx -e mjts -e mjsx --exec echo "Found potentially unhandled file: {}"Length of output: 93
Glob Pattern Verification: Manual Check Recommended
After scanning for files with the extensions (.mjs, .cjs, .mjsx, .mjts), no unhandled files were detected. The current glob patterns (covering
**/*.[jt]s,**/*.[jt]sx, and**/*.[cm][jt]s) seem to cover all the common JavaScript/TypeScript file types used in the project.However, since niche extensions like
.mjsxmight still be used in special cases, please verify manually if your codebase includes any non-standard file extensions that need to be addressed.
Judahmeek
force-pushed
the
alexeyr/upgrade-eslint
branch
from
0bf5546 to
f164ef7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eslint.config.ts (1)
15-39: Consider consolidating similar ignore patterns for better maintainabilityThe global ignores list is comprehensive but could be organized more efficiently. Consider grouping similar patterns to make future maintenance easier.
globalIgnores([ // compiled code 'node_package/lib/', // used for tests only 'spec/react_on_rails/dummy-for-generators', // temporary and generated files 'spec/dummy/.yalc', 'spec/dummy/public', 'spec/dummy/vendor', 'spec/dummy/tmp', 'spec/dummy/app/assets/config/manifest.js', 'spec/dummy/client/app/packs/server-bundle.js', '**/*.res.js', - '**/coverage', - '**/assets/webpack/**/*', - '**/public/webpack/**/*', - '**/generated/**/*', - '**/app/assets/javascripts/application.js', - '**/cable.js', - '**/public/packs*/*', - '**/gen-examples/', - '**/bundle/', + // test outputs + '**/coverage', + // webpack and generated assets + '**/assets/webpack/**/*', + '**/public/webpack/**/*', + '**/generated/**/*', + '**/public/packs*/*', + '**/gen-examples/', + '**/bundle/', + // specific app files + '**/app/assets/javascripts/application.js', + '**/cable.js', // dependencies '**/node_modules/**/*', ]),
📜 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 (45)
.eslintignore(0 hunks).eslintrc(0 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/package-js-tests.yml(0 hunks).prettierignore(0 hunks)eslint.config.ts(1 hunks)knip.ts(2 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.js(1 hunks)node_package/src/CallbackRegistry.ts(1 hunks)node_package/src/ClientSideRenderer.ts(3 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/ReactOnRails.node.ts(1 hunks)node_package/src/clientStartup.ts(1 hunks)node_package/src/createReactOutput.ts(0 hunks)node_package/src/reactHydrateOrRender.ts(2 hunks)node_package/src/types/index.ts(0 hunks)node_package/tests/ComponentRegistry.test.js(0 hunks)node_package/tests/RSCClientRoot.test.jsx(0 hunks)node_package/tests/ReactOnRails.test.jsx(0 hunks)node_package/tests/renderFunction.test.jsx(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(6 hunks)package-scripts.yml(1 hunks)package.json(3 hunks)script/convert(1 hunks)spec/dummy/client/app/components/HelloWorldRedux.jsx(1 hunks)spec/dummy/client/app/components/RailsContext.jsx(2 hunks)spec/dummy/client/app/packs/rescript-components.js(1 hunks)spec/dummy/client/app/reducers/HelloWorldReducer.jsx(1 hunks)spec/dummy/client/app/reducers/nullReducer.jsx(1 hunks)spec/dummy/client/app/startup/CacheDisabled.jsx(0 hunks)spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx(1 hunks)spec/dummy/client/app/startup/CssModulesImagesFontsExample.jsx(0 hunks)spec/dummy/client/app/startup/HelloWorld.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldES5.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldHooks.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldProps.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldReScript.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldRehydratable.jsx(1 hunks)spec/dummy/client/app/startup/HelloWorldWithLogAndThrow.jsx(0 hunks)spec/dummy/client/app/startup/PureComponent.jsx(1 hunks)spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx(1 hunks)spec/dummy/client/app/store/composeInitialState.js(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)spec/dummy/config/webpack/serverWebpackConfig.js(1 hunks)spec/dummy/config/webpack/webpackConfig.js(0 hunks)
💤 Files with no reviewable changes (13)
- spec/dummy/client/app/startup/CssModulesImagesFontsExample.jsx
- .github/workflows/package-js-tests.yml
- node_package/src/types/index.ts
- node_package/tests/ReactOnRails.test.jsx
- .prettierignore
- spec/dummy/client/app/startup/HelloWorldWithLogAndThrow.jsx
- node_package/src/createReactOutput.ts
- spec/dummy/config/webpack/webpackConfig.js
- node_package/tests/ComponentRegistry.test.js
- node_package/tests/RSCClientRoot.test.jsx
- spec/dummy/client/app/startup/CacheDisabled.jsx
- .eslintignore
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (31)
- spec/dummy/config/webpack/commonWebpackConfig.js
- spec/dummy/client/app/packs/rescript-components.js
- spec/dummy/client/app/reducers/HelloWorldReducer.jsx
- spec/dummy/config/webpack/serverWebpackConfig.js
- node_package/src/ReactOnRails.node.ts
- spec/dummy/client/app/startup/PureComponent.jsx
- .github/workflows/lint-js-and-ruby.yml
- spec/dummy/client/app/startup/ContextFunctionReturnInvalidJSX.jsx
- node_package/tests/renderFunction.test.jsx
- spec/dummy/client/app/reducers/nullReducer.jsx
- node_package/src/ReactOnRails.client.ts
- spec/dummy/client/app/startup/HelloWorldES5.jsx
- spec/dummy/client/app/components/HelloWorldRedux.jsx
- spec/dummy/client/app/startup/HelloWorldReScript.jsx
- spec/dummy/client/app/startup/HelloWorldRehydratable.jsx
- node_package/src/clientStartup.ts
- spec/dummy/client/app/startup/HelloWorld.jsx
- script/convert
- node_package/tests/streamServerRenderedReactComponent.test.jsx
- knip.ts
- node_package/src/reactHydrateOrRender.ts
- spec/dummy/client/app/startup/HelloWorldHooks.jsx
- spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.js
- spec/dummy/client/app/components/RailsContext.jsx
- package-scripts.yml
- node_package/src/CallbackRegistry.ts
- spec/dummy/client/app/startup/HelloWorldProps.jsx
- node_package/src/ClientSideRenderer.ts
- spec/dummy/client/app/store/composeInitialState.js
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: examples (newest)
🔇 Additional comments (6)
eslint.config.ts (6)
2-5: Fix TypeScript module import syntaxThe pipeline is failing due to TypeScript import issues. These default imports need to be modified since they require the
allowSyntheticDefaultImportsflag:-import prettierRecommended from 'eslint-plugin-prettier/recommended'; -import globals from 'globals'; -import js from '@eslint/js'; +import * as prettierRecommended from 'eslint-plugin-prettier/recommended'; +import * as globals from 'globals'; +import * as js from '@eslint/js';Alternatively, you could add
allowSyntheticDefaultImports: trueto the TypeScript configuration.
99-104: Well implemented ReScript import handlingGood job implementing the rule to handle ReScript imports as previously suggested. This correctly ignores
.res.jsfiles from the import resolution checks.
125-133: Good use of explanatory commentsThe comments explaining why specific rules are disabled for template files are helpful and improve code maintainability. This is a great practice that helps future developers understand the reasoning behind configuration choices.
158-161: Correct placement of Prettier configurationThe Prettier configuration is correctly placed at the end of the configuration array, as recommended in the Prettier documentation. This ensures that Prettier's formatting rules take precedence over other potentially conflicting rules.
42-44:❓ Verification inconclusive
Verify that eslint-config-shakacode is compatible with flat config
Make sure that
eslint-config-shakacodeis compatible with the new ESLint flat config format. Some older configs might need updates or adaptations to work properly with the new system.
🏁 Script executed:
#!/bin/bash # Check the version of eslint-config-shakacode grep -A 2 "eslint-config-shakacode" package.json # Check if there are any compatibility notes in the readme if [ -d "node_modules/eslint-config-shakacode" ]; then if [ -f "node_modules/eslint-config-shakacode/README.md" ]; then echo "Checking README.md for flat config compatibility info:" grep -i -A 5 -B 5 "flat\|eslint.*config" node_modules/eslint-config-shakacode/README.md || echo "No explicit mentions of flat config" fi fiLength of output: 1012
Action Required: Confirm eslint-config-shakacode's Compatibility with ESLint Flat Config
It looks like the project currently uses
eslint-config-shakacodeversion^19.0.0. However, the package’s README does not explicitly mention support for ESLint’s flat config. Please verify one of the following:
- Review Documentation: Check the package’s changelog or release notes for any indication of flat config support.
- Test Integration: Run the ESLint flat config in a sandbox or staging environment to ensure that extending
eslint-config-shakacodeviacompat.extends('eslint-config-shakacode')works as expected.- Manual Verification: If necessary, reach out to the maintainers or check community discussions regarding any required adaptations for flat config compatibility.
137-145:❓ Verification inconclusive
Check TypeScript project configuration
The TypeScript configuration is using
projectServicewithallowDefaultProjectfor specific files. Ensure this is properly configured for your build environment and won't cause issues with TypeScript's project references.
🏁 Script executed:
#!/bin/bash # Check if tsconfig.json exists and what compiler options it contains if [ -f "tsconfig.json" ]; then echo "tsconfig.json exists" cat tsconfig.json | grep -A 20 "compilerOptions" else echo "tsconfig.json not found" fi # Check what TypeScript-ESLint version is being used grep -A 2 "typescript-eslint" package.jsonLength of output: 732
Action: Confirm TypeScript project configuration for custom default projects
- Verified that a valid
tsconfig.jsonexists with the expectedcompilerOptionssettings.- The project uses TypeScript-ESLint version
^8.26.1, ensuring compatibility with the configuration.- The ESLint configuration in
eslint.config.tsleveragesprojectService.allowDefaultProjectto includeeslint.config.tsandknip.ts. Please double-check that allowing these files as default projects aligns with your build environment and does not conflict with TypeScript’s project references.
Summary
Upgrade ESLint to v9, TypeScript-ESLint to v8, and related plugins.
Closes #1689
Pull Request checklist
[ ] Add/update test to cover these changes[ ] Update documentation[ ] Update CHANGELOG fileNot user-visibleThis change is
Summary by CodeRabbit
These internal improvements maintain existing functionality while enhancing the project's stability and code clarity for future development.