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

[Fix] correct generated type declaration #3840

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

ocavue
Copy link

@ocavue ocavue commented Oct 12, 2024

Closes #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;

package.json Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

index.js Show resolved Hide resolved
lib/rules/jsx-fragments.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Oct 16, 2024

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 :-)

@@ -1,3 +1,4 @@
declare module 'string.prototype.repeat' {
export = typeof Function.call.bind(String.prototype.repeat);
function repeat(text: string, count: number): string;
Copy link
Member

Choose a reason for hiding this comment

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

this means that it won't always inherit the builtin type for repeat. is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

The previous version of types/string.prototype.repeat/index.d.ts doesn't work.

declare module 'string.prototype.repeat' {
  export = typeof Function.call.bind(String.prototype.repeat);
           // ^: The expression of an export assignment must be an identifier or qualified name in an ambient context.ts(2714)
}

This error didn't show up because tsconfig.json didn't include types/ at all, thus string.prototype.repeat was been treated as any by TypeScript.

I'm not sure if there are better way to write this index.d.ts, but it definitely need some changes.

Copy link
Member

Choose a reason for hiding this comment

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

ah, right. how about:

Suggested change
function repeat(text: string, count: number): string;
const repeat: typeof Function.call.bind(String.prototype.repeat);

?

Copy link
Author

@ocavue ocavue Oct 17, 2024

Choose a reason for hiding this comment

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

It doesn't work :(

declare module 'string.prototype.repeat' {
  const repeat: typeof Function.call.bind(String.prototype.repeat);
                                      // ^: ',' expected. ts(1005)
  export = repeat;
}

@@ -485,7 +447,7 @@ module.exports = {
});
});
},
} : false, {
} : {}, {
Copy link
Member

Choose a reason for hiding this comment

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

why this change? primitives are object-spread the same as {}.

Copy link
Author

Choose a reason for hiding this comment

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

I don't remember why I did this change exactly, probably because false isn't compatible with the latest TypeScript or @types/eslint.

Anyway, let me revert this change for now. We might will need to change it to {} in the future if we update TypeScript or @types/eslint.

@ocavue
Copy link
Author

ocavue commented Oct 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: TS types are broken
3 participants