Add JSDoc type annotations #3731

y-hsgw · 2024-04-09T02:39:21Z

I have added type annotations and fixed associated type errors.

ljharb

this isn't just adding type annotations and a lot of the changes are potentially breaking.

ljharb · 2024-04-09T02:49:52Z

lib/rules/button-has-type.js

+        const typeProp = props.find(
+          (prop) => prop.type === "Property" && prop.key && prop.key.type === "PrivateIdentifier" && prop.key.name === 'type'
+        );

-        if (!typeProp) {
+        if (typeProp.type !== "Property") {


these changes aren't simple annotation changes. can they be extracted at least to a different commit, if not to a different PR?

I see. My apologies 🙇‍♂️
Would it be acceptable to create a PR that adds annotations only to rules where type changes are unnecessary?
Or would it be better to add annotations to all rules, and if changes are needed, ignore the entire file (@ ts-nocheck)?

yes, that would be fine.

any non-type changes that are needed must be accompanied by tests - if the ostensible type error can't be reproduced, then the type system is wrong.

lib/rules/checked-requires-onchange-or-readonly.js

ljharb · 2024-04-09T02:51:11Z

lib/rules/forbid-elements.js

-        const argType = argument.type;
-
-        if (argType === 'Identifier' && /^[A-Z_]/.test(argument.name)) {
+        if (argument.type === 'Identifier' && /^[A-Z_]/.test(argument.name)) {
          reportIfForbidden(argument.name, argument);
-        } else if (argType === 'Literal' && /^[a-z][^.]*$/.test(argument.value)) {
+        } else if (argument.type === 'Literal' && /^[a-z][^.]*$/.test(argument.value.toString())) {
          reportIfForbidden(argument.value, argument);
-        } else if (argType === 'MemberExpression') {
+        } else if (argument.type === 'MemberExpression') {


the original only has one observable lookup; this has 3. why is this an improvement?

also, String(x), never x.toString()

the original only has one observable lookup; this has 3. why is this an improvement?

The reason is because the following error message occurs during the execution of the type-check script.

Property 'name' does not exist on type 'ClassExpression | ArrayExpression | ArrowFunctionExpression | AssignmentExpression | ... 23 more ... | SpreadElement'. Property 'name' does not exist on type 'ClassExpression'. if (argType === 'Identifier' && /^[A-Z_]/.test(argument.name)) {

also, String(x), never x.toString()

thanks! Fixed in 4ae24a2 .

lib/rules/forbid-foreign-prop-types.js

y-hsgw · 2024-04-10T05:44:57Z

I have addressed this in pull request #3732 and will proceed to close this one. I'll handle the areas requiring type changes separately.

ljharb · 2024-04-10T06:08:43Z

I would prefer to update this PR with those changes rather than abandoning it.

…into feature/add-jsdoc-annotations

ljharb

i only went like 20% through, but this should help you refine the entire thing

lib/rules/checked-requires-onchange-or-readonly.js

lib/rules/forbid-foreign-prop-types.js

lib/rules/forbid-prop-types.js

lib/rules/jsx-closing-bracket-location.js

lib/rules/jsx-curly-spacing.js

lib/rules/jsx-indent.js

ljharb · 2024-04-10T14:53:20Z

lib/rules/jsx-no-bind.js

+    function getBlockStatementAncestors() {
+      return context.getAncestors().filter(


i'm pretty sure this change will cause eslint 9 to break. can you confirm?

Sorry. I do not understand it well.
Should I add v9 to eslint in package.json as shown below and see if I get a compile error?

"devDependencies": { "eslint": "^3 || ^4 || ^5 || ^6 || ^7 || ^8 || ^9", ... }, "peerDependencies": { "eslint": "^3 || ^4 || ^5 || ^6 || ^7 || ^8 || ^9" },

No, adding eslint 9 is a hard task. What i mean is, what’s wrong with passing the node here?

When looking at the type information for context.getAncestors, it seems that the argument is unnecessary.

(method) Rule.RuleContext.getAncestors(): Node[]

ljharb · 2024-04-10T14:53:38Z

lib/rules/jsx-no-bind.js

        const variableViolationType = getNodeViolationType(node.init);

        if (
          blockAncestors.length > 0
          && variableViolationType
+          && node.parent.type === 'VariableDeclaration'


we're already inside VariableDeclarator, this shouldn't be needed

Property 'kind' does not exist on type 'Node'.
This error occurs.

if this is just for narrowing, we can use type casts to tell TS what kind of thing it is.

As far as I know, since it's a JavaScript file, I don't think type assertions can be used.

they can, by wrapping the value in parens and preceding it with a @type comment.

I didn't know that method! Thank you! Fixed in e2c3497 .

lib/rules/jsx-sort-default-props.js

lib/rules/jsx-space-before-closing.js

codecov · 2024-04-11T02:59:02Z

Codecov Report

Attention: Patch coverage is 94.89796% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 97.69%. Comparing base (4467db5) to head (e2c3497).

❗ Current head e2c3497 differs from pull request most recent head b392497. Consider uploading reports for the commit b392497 to get more accurate results

Files	Patch %	Lines
lib/rules/jsx-curly-spacing.js	83.33%	1 Missing ⚠️
lib/rules/jsx-equals-spacing.js	50.00%	1 Missing ⚠️
lib/rules/jsx-space-before-closing.js	50.00%	1 Missing ⚠️
lib/rules/no-deprecated.js	92.85%	1 Missing ⚠️
lib/rules/no-unused-class-component-methods.js	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3731      +/-   ##
==========================================
- Coverage   97.71%   97.69%   -0.03%     
==========================================
  Files         133      133              
  Lines        9470     9507      +37     
  Branches     3469     3494      +25     
==========================================
+ Hits         9254     9288      +34     
- Misses        216      219       +3

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

lib/rules/jsx-closing-bracket-location.js

ljharb · 2024-04-11T06:21:21Z

lib/rules/button-has-type.js

@@ -149,9 +150,11 @@ module.exports = {
        }

        const props = node.arguments[1].properties;


is the type of node properly narrowed to a CallExpression node here? if not, let's do that as an annotation on the visitor function rather than needing to add all these runtime changes.

~~I attempted to make corrections in 040d77c, but is my understanding correct?~~
Sorry. I didn't fully understand it.

is the type of node properly narrowed to a CallExpression node here?

The type of node is not properly narrowed down.

It's (parameter) node: CallExpression & Rule.NodeParentExtension.

y-hsgw added 2 commits April 9, 2024 11:36

Add annotations

f2c6548

Add Ignore case

3465df0

ljharb requested changes Apr 9, 2024

View reviewed changes

ljharb marked this pull request as draft April 9, 2024 02:52

y-hsgw mentioned this pull request Apr 9, 2024

Add JSDoc type annotations #3732

Merged

y-hsgw closed this Apr 10, 2024

y-hsgw deleted the feature/add-jsdoc-annotations branch April 10, 2024 05:45

y-hsgw restored the feature/add-jsdoc-annotations branch April 10, 2024 06:10

y-hsgw reopened this Apr 10, 2024

y-hsgw mentioned this pull request Apr 10, 2024

Add type annotation for button-has-type rule #3734

Closed

y-hsgw added 6 commits April 10, 2024 15:20

Merge branch 'master' of https://github.com/y-hsgw/eslint-plugin-react …

32d4165

…into feature/add-jsdoc-annotations

Delete Spaces and fix lint

9018532

fix lint

fc04de6

Revert deletion to prevent JSXText node catching.

88d2280

Strict enforcement of type definitions

4a99c2a

Use String instead of toString

4ae24a2

ljharb requested changes Apr 10, 2024

View reviewed changes

y-hsgw added 3 commits April 11, 2024 10:08

Update type definitions and revert changes

844d483

Fix failing tests

0ea8000

Replace @ts-ignore with @ts-expect-error

eb44ba7

Remove @ts-expect-error and add comments

ecee17c

ljharb reviewed Apr 11, 2024

View reviewed changes

lib/rules/jsx-closing-bracket-location.js Outdated Show resolved Hide resolved

y-hsgw added 2 commits April 11, 2024 15:15

Remove @ts-nocheck

25e6afd

Fixed comments

a5ac70f

y-hsgw changed the title ~~Add JSDoc annotations~~ Add JSDoc type annotations Apr 11, 2024

ljharb reviewed Apr 11, 2024

View reviewed changes

Narrow down the type to CallExpression

040d77c

y-hsgw and others added 7 commits April 11, 2024 16:50

add test for button-has-type

b6856ed

Remove unnecessary type narrowing

d59ac20

Remove unnecessary type narrowing and ts-ignore

5e77d12

Merge branch 'master' into feature/add-jsdoc-annotations

4161d14

Remove unnecessary type narrowing

aa96591

Use type assertions

e2c3497

Fix of production code with decreased coverage

b392497

y-hsgw requested a review from ljharb April 17, 2024 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JSDoc type annotations #3731

Add JSDoc type annotations #3731

y-hsgw commented Apr 9, 2024 •

edited

ljharb left a comment

ljharb Apr 9, 2024

y-hsgw Apr 9, 2024 •

edited

ljharb Apr 9, 2024

ljharb Apr 9, 2024

y-hsgw Apr 10, 2024 •

edited

y-hsgw commented Apr 10, 2024

ljharb commented Apr 10, 2024

ljharb left a comment

ljharb Apr 10, 2024

y-hsgw Apr 11, 2024

ljharb Apr 11, 2024

y-hsgw Apr 11, 2024

ljharb Apr 10, 2024

y-hsgw Apr 11, 2024

ljharb Apr 11, 2024

y-hsgw Apr 11, 2024 •

edited

ljharb Apr 13, 2024 •

edited

y-hsgw Apr 13, 2024

codecov bot commented Apr 11, 2024 •

edited

ljharb Apr 11, 2024

y-hsgw Apr 11, 2024 •

edited

		function getBlockStatementAncestors() {
		return context.getAncestors().filter(

		@@ -149,9 +150,11 @@ module.exports = {
		}

		const props = node.arguments[1].properties;

Add JSDoc type annotations #3731

Are you sure you want to change the base?

Add JSDoc type annotations #3731

Conversation

y-hsgw commented Apr 9, 2024 • edited

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-hsgw Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-hsgw Apr 10, 2024 • edited

Choose a reason for hiding this comment

y-hsgw commented Apr 10, 2024

ljharb commented Apr 10, 2024

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-hsgw Apr 11, 2024 • edited

Choose a reason for hiding this comment

ljharb Apr 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 11, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

y-hsgw Apr 11, 2024 • edited

Choose a reason for hiding this comment

y-hsgw commented Apr 9, 2024 •

edited

y-hsgw Apr 9, 2024 •

edited

y-hsgw Apr 10, 2024 •

edited

y-hsgw Apr 11, 2024 •

edited

ljharb Apr 13, 2024 •

edited

codecov bot commented Apr 11, 2024 •

edited

y-hsgw Apr 11, 2024 •

edited