-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: master
Are you sure you want to change the base?
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.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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;
}
lib/rules/jsx-no-literals.js
Outdated
@@ -485,7 +447,7 @@ module.exports = { | |||
}); | |||
}); | |||
}, | |||
} : false, { | |||
} : {}, { |
There was a problem hiding this comment.
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 {}
.
There was a problem hiding this comment.
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
.
@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.ts
can be viewed in this link. The change highlight are shown below: