jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
9.01k stars 2.77k forks source link

[Fix] correct generated type declaration #3840

Open ocavue opened 1 month ago

ocavue commented 1 month ago

Closes https://github.com/jsx-eslint/eslint-plugin-react/issues/3838

This PR fixes various issues in the generated index.d.ts. The full diff for index.d.ts can be viewed in this link. The change highlight are shown below:

  rules: {
-   'react/display-name': number;
+   'react/display-name': 2;
-   'react/jsx-key': number;
+   'react/jsx-key': 2;
    'jsx-no-duplicate-props': import("eslint").Rule.RuleModule;
    'jsx-no-leaked-render': import("eslint").Rule.RuleModule;
-   'jsx-no-literals': {
-      meta: import("eslint").Rule.RuleMetaData;
-      create(context: any): (false & {
-      ...
-   };
+   'jsx-no-literals': import("eslint").Rule.RuleModule;
codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.54%. Comparing base (5c23573) to head (bab6a75).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3840 +/- ## ========================================== - Coverage 97.71% 97.54% -0.17% ========================================== Files 133 133 Lines 9958 9966 +8 Branches 3693 3698 +5 ========================================== - Hits 9730 9721 -9 - Misses 228 245 +17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ljharb commented 1 month ago

ok, i fixed the issue with index.js in master, but i've rebased this PR on top of that, since this is probably a better outcome still :-)

ocavue commented 1 month ago

@ljharb Hi. Thanks for your review. I've addressed all your previous comments.

In addition to that, I was thinking why we have issues like https://github.com/jsx-eslint/eslint-plugin-react/issues/3838 even if we have .github/workflows/type-check.yml to check the built types. I found there are three issues in the CI workflow and I fixed them.

  1. In test-published-types/index.js, to make the typescript to check the type, we need to create a variable first and export it later
// Bad
/** @type {import('eslint').Linter.Config[]} */
module.exports = [ ... ] 

// Good
/** @type {import('eslint').Linter.Config[]} */
const config = [ ... ]
module.exports = config;
  1. In test-published-types/package.json, if we use "eslint-plugin-react": "file:..", we would copy/link all node_modules under eslint-plugin-react to test-published-types/node_modules/eslint-plugin-react/node_modules. This is problematic because test-published-types use the built-in types from the latest eslint, while eslint-plugin-react use the deprecated @types/eslint. They have conflict.

    I fixed this by packing eslint-plugin-react into a .taz file and install it later. By doing that, @types/eslint, which is a dev dependency of eslint-plugin-react, won't be installed under test-published-types.

  2. There is a small issue in the eslint's type declarations:

$ cd test-published-types
$ npx tsc --lib es2015

node_modules/eslint/lib/types/index.d.ts:928:81 - error TS2574: A rest element type must be an array type.

928     type RuleSeverityAndOptions<Options extends any[] = any[]> = [RuleSeverity, ...Partial<Options>];
                                                                                    ~~~~~~~~~~~~~~~~~~~

I need to fix it by adding "skipLibCheck": false in test-published-types/tsconfig.json.