[Fix] correct generated type declaration #3840
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Co-authored-by: Jordan Harband <[email protected]>
… assignment cannot be used in a module with other exported elements.
|
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; | |||
There was a problem hiding this comment.
this means that it won't always inherit the builtin type for repeat. is this necessary?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ah, right. how about:
| function repeat(text: string, count: number): string; | |
| const repeat: typeof Function.call.bind(String.prototype.repeat); |
?
There was a problem hiding this comment.
It doesn't work :(
declare module 'string.prototype.repeat' {
const repeat: typeof Function.call.bind(String.prototype.repeat);
// ^: ',' expected. ts(1005)
export = repeat;
}
lib/rules/jsx-no-literals.js
Outdated
| @@ -485,7 +447,7 @@ module.exports = { | |||
| }); | |||
| }); | |||
| }, | |||
| } : false, { | |||
| } : {}, { | |||
There was a problem hiding this comment.
why this change? primitives are object-spread the same as {}.
There was a problem hiding this comment.
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.
|
@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
// Bad
/** @type {import('eslint').Linter.Config[]} */
module.exports = [ ... ]
// Good
/** @type {import('eslint').Linter.Config[]} */
const config = [ ... ]
module.exports = config;
$ 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 |
Closes #3838
This PR fixes various issues in the generated
index.d.ts. The full diff forindex.d.tscan 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;