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.

Sign up for GitHub

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

Jump to bottom

Add JSDoc type annotations #3731

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from
+174 −74
Draft

Add JSDoc type annotations #3731

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f2c6548
Add annotations
y-hsgw Apr 9, 2024
3465df0
Add Ignore case
y-hsgw Apr 9, 2024
32d4165
Merge branch 'master' of https://github.com/y-hsgw/eslint-plugin-reac…
y-hsgw Apr 10, 2024
9018532
Delete Spaces and fix lint
y-hsgw Apr 10, 2024
fc04de6
fix lint
y-hsgw Apr 10, 2024
88d2280
Revert deletion to prevent JSXText node catching.
y-hsgw Apr 10, 2024
4a99c2a
Strict enforcement of type definitions
y-hsgw Apr 10, 2024
4ae24a2
Use String instead of toString
y-hsgw Apr 10, 2024
844d483
Update type definitions and revert changes
y-hsgw Apr 11, 2024
0ea8000
Fix failing tests
y-hsgw Apr 11, 2024
eb44ba7
Replace @ts-ignore with @ts-expect-error
y-hsgw Apr 11, 2024
ecee17c
Remove @ts-expect-error and add comments
y-hsgw Apr 11, 2024
25e6afd
Remove @ts-nocheck
y-hsgw Apr 11, 2024
a5ac70f
Fixed comments
y-hsgw Apr 11, 2024
040d77c
Narrow down the type to CallExpression
y-hsgw Apr 11, 2024
b6856ed
add test for button-has-type
y-hsgw Apr 11, 2024
d59ac20
Remove unnecessary type narrowing
y-hsgw Apr 11, 2024
5e77d12
Remove unnecessary type narrowing and ts-ignore
y-hsgw Apr 11, 2024
4161d14
Merge branch 'master' into feature/add-jsdoc-annotations
y-hsgw Apr 13, 2024
aa96591
Remove unnecessary type narrowing
y-hsgw Apr 13, 2024
e2c3497
Use type assertions
y-hsgw Apr 13, 2024
b392497
Fix of production code with decreased coverage
y-hsgw Apr 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/rules/button-has-type.js
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@ const messages = {
forbiddenValue: '"{{value}}" is an invalid value for button type attribute',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down Expand Up @@ -149,9 +150,11 @@ module.exports = {
}

const props = node.arguments[1].properties;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@y-hsgw y-hsgw Apr 11, 2024 •
edited

Choose a reason for hiding this comment

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

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.

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

if (!typeProp) {
if (typeProp.type !== 'Property') {
Copy link
Member

Choose a reason for hiding this comment

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

test?

Copy link
Contributor Author

@y-hsgw y-hsgw Apr 11, 2024 •
edited

Choose a reason for hiding this comment

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

I've added tests in b6856ed. What do you think?

reportMissing(node);
return;
}
Expand Down
3 changes: 2 additions & 1 deletion lib/rules/checked-requires-onchange-or-readonly.js
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@ const defaultOptions = {
};

/**
* @param {string[]} properties
* @param {object[]} properties
* @param {string} keyName
* @returns {Set<string>}
*/
Expand All @@ -41,6 +41,7 @@ function extractTargetProps(properties, keyName) {
);
}

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down
9 changes: 4 additions & 5 deletions lib/rules/forbid-elements.js
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@ const messages = {
forbiddenElement_message: '<{{element}}> is forbidden, {{message}}',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down Expand Up @@ -103,13 +104,11 @@ module.exports = {
return;
}

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(String(argument.value))) {
reportIfForbidden(argument.value, argument);
} else if (argType === 'MemberExpression') {
} else if (argument.type === 'MemberExpression') {
reportIfForbidden(context.getSourceCode().getText(argument), argument);
}
},
Expand Down
2 changes: 2 additions & 0 deletions lib/rules/forbid-foreign-prop-types.js
View file
Open in desktop
@@ -1,3 +1,4 @@
// @ts-nocheck
/**
* @fileoverview Forbid using another component's propTypes
* @author Ian Christian Myers
Expand All @@ -13,6 +14,7 @@ const messages = {
forbiddenPropType: 'Using propTypes from another component is not safe because they may be removed in production builds',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down
2 changes: 2 additions & 0 deletions lib/rules/forbid-prop-types.js
View file
Open in desktop
@@ -1,3 +1,4 @@
// @ts-nocheck
ljharb marked this conversation as resolved.
Show resolved Hide resolved
/**
* @fileoverview Forbid certain propTypes
*/
Expand Down Expand Up @@ -25,6 +26,7 @@ const messages = {
forbiddenPropType: 'Prop type "{{target}}" is forbidden',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down
1 change: 1 addition & 0 deletions lib/rules/hook-use-state.js
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ const messages = {
suggestMemo: 'Replace useState call with useMemo',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down
5 changes: 5 additions & 0 deletions lib/rules/jsx-closing-bracket-location.js
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ const messages = {
bracketLocation: 'The closing bracket must be {{location}}{{details}}',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down Expand Up @@ -195,7 +196,11 @@ module.exports = {
*/
function getTokensLocations(node) {
const sourceCode = context.getSourceCode();
// The types differ between ASTNode and ESTree.Node.
// @ts-expect-error
ljharb marked this conversation as resolved.
Show resolved Hide resolved
const opening = sourceCode.getFirstToken(node).loc.start;
// The types differ between ASTNode and ESTree.Node.
// @ts-expect-error
const closing = sourceCode.getLastTokens(node, node.selfClosing ? 2 : 1)[0].loc.start;
const tag = sourceCode.getFirstToken(node.name).loc.start;
let lastProp;
Expand Down
2 changes: 2 additions & 0 deletions lib/rules/jsx-curly-spacing.js
View file
Open in desktop
@@ -1,3 +1,4 @@
// @ts-nocheck
ljharb marked this conversation as resolved.
Show resolved Hide resolved
/**
* @fileoverview Enforce or disallow spaces inside of curly braces in JSX attributes.
* @author Jamund Ferguson
Expand Down Expand Up @@ -34,6 +35,7 @@ const messages = {
spaceNeededBefore: 'A space is required before \'{{token}}\'',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down
4 changes: 4 additions & 0 deletions lib/rules/jsx-equals-spacing.js
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@ const messages = {
needSpaceAfter: 'A space is required after \'=\'',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down Expand Up @@ -61,6 +62,9 @@ module.exports = {

const sourceCode = context.getSourceCode();
const equalToken = sourceCode.getTokenAfter(attrNode.name);
if (equalToken.type !== 'Punctuator') {
return;
}
const spacedBefore = sourceCode.isSpaceBetweenTokens(attrNode.name, equalToken);
const spacedAfter = sourceCode.isSpaceBetweenTokens(equalToken, attrNode.value);

Expand Down
3 changes: 2 additions & 1 deletion lib/rules/jsx-fragments.js
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@ const messages = {
preferFragment: 'Prefer fragment shorthand over {{react}}.{{fragment}}',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down Expand Up @@ -171,7 +172,7 @@ module.exports = {
ImportDeclaration(node) {
if (node.source && node.source.value === 'react') {
node.specifiers.forEach((spec) => {
if (spec.imported && spec.imported.name === fragmentPragma) {
if (spec.type === 'ImportSpecifier' && spec.imported && spec.imported.name === fragmentPragma) {
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? what are the possible type values of the objects contained an ImportDeclaration node's specifiers array - is it more than just ImportSpecifier?

Copy link
Contributor Author

@y-hsgw y-hsgw Apr 11, 2024 •
edited

Choose a reason for hiding this comment

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

what are the possible type values of the objects contained an ImportDeclaration node's specifiers array - is it more than just ImportSpecifier?

It's ImportSpecifier | ImportDefaultSpecifier | ImportNamespaceSpecifier .

I added it because only ImportSpecifier has the imported property.

Copy link
Member

Choose a reason for hiding this comment

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

would 'imported' in spec narrow properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will be narrowed down. Alternatively, it might be better to do it as follows.

 if (spec.type === 'ImportSpecifier' && spec.imported.name === fragmentPragma) { ...

if (spec.local) {
fragmentNames.add(spec.local.name);
}
Expand Down
3 changes: 3 additions & 0 deletions lib/rules/jsx-indent-props.js
View file
Open in desktop
Expand Up @@ -42,6 +42,7 @@ const messages = {
wrongIndent: 'Expected indentation of {{needed}} {{type}} {{characters}} but found {{gotten}}.',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down Expand Up @@ -140,6 +141,8 @@ module.exports = {
* @return {Number} Indent
*/
function getNodeIndent(node) {
// The types differ between ASTNode and ESTree.Node.
// @ts-expect-error
let src = context.getSourceCode().getText(node, node.loc.start.column + extraColumnStart);
const lines = src.split('\n');
src = lines[0];
Expand Down
2 changes: 2 additions & 0 deletions lib/rules/jsx-indent.js
View file
Open in desktop
@@ -1,3 +1,4 @@
// @ts-nocheck
ljharb marked this conversation as resolved.
Show resolved Hide resolved
/**
* @fileoverview Validate JSX indentation
* @author Yannick Croissant
Expand Down Expand Up @@ -45,6 +46,7 @@ const messages = {
wrongIndent: 'Expected indentation of {{needed}} {{type}} {{characters}} but found {{gotten}}.',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down
13 changes: 8 additions & 5 deletions lib/rules/jsx-no-bind.js
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ const messages = {
func: 'JSX props should not use functions',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down Expand Up @@ -122,8 +123,8 @@ module.exports = {
blockVariableNameSets[blockStart][violationType].add(variableName);
}

function getBlockStatementAncestors(node) {
return context.getAncestors(node).filter(
function getBlockStatementAncestors() {
return context.getAncestors().filter(
Comment on lines +126 to +127
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"
  },

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

(ancestor) => ancestor.type === 'BlockStatement'
).reverse();
}
Expand All @@ -145,7 +146,7 @@ module.exports = {
}

function findVariableViolation(node, name) {
getBlockStatementAncestors(node).find(
getBlockStatementAncestors().find(
(block) => reportVariableViolation(node, name, block.range[0])
);
}
Expand All @@ -156,7 +157,7 @@ module.exports = {
},

FunctionDeclaration(node) {
const blockAncestors = getBlockStatementAncestors(node);
const blockAncestors = getBlockStatementAncestors();
const variableViolationType = getNodeViolationType(node);

if (blockAncestors.length > 0 && variableViolationType) {
Expand All @@ -168,13 +169,15 @@ module.exports = {
if (!node.init) {
return;
}
const blockAncestors = getBlockStatementAncestors(node);
const blockAncestors = getBlockStatementAncestors();
const variableViolationType = getNodeViolationType(node.init);

if (
blockAncestors.length > 0
&& variableViolationType
&& node.parent.type === 'VariableDeclaration'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@y-hsgw y-hsgw Apr 11, 2024 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Member

@ljharb ljharb Apr 13, 2024 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

&& node.parent.kind === 'const' // only support const right now
&& node.id.type === 'Identifier'
) {
addVariableNameToSet(
variableViolationType, node.id.name, blockAncestors[0].range[0]
Expand Down
3 changes: 0 additions & 3 deletions lib/rules/jsx-no-leaked-render.js
View file
Open in desktop
Expand Up @@ -108,9 +108,6 @@ function ruleFixer(context, fixStrategy, fixer, reportedNode, leftNode, rightNod
throw new TypeError('Invalid value for "validStrategies" option');
}

/**
* @type {import('eslint').Rule.RuleModule}
*/
/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
Expand Down
9 changes: 9 additions & 0 deletions lib/rules/jsx-sort-default-props.js
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ const messages = {
propsNotSorted: 'Default prop types declarations should be sorted alphabetically',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
deprecated: true,
Expand Down Expand Up @@ -172,6 +173,14 @@ module.exports = {
return;
}

if (
node.parent.type !== 'BinaryExpression'
&& node.parent.type !== 'AssignmentExpression'
&& node.parent.type !== 'LogicalExpression'
) {
ljharb marked this conversation as resolved.
Show resolved Hide resolved
return;
}

checkNode(node.parent.right);
},

Expand Down
16 changes: 16 additions & 0 deletions lib/rules/jsx-space-before-closing.js
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@ const messages = {
needSpaceBeforeClose: 'A space is required before closing bracket',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
deprecated: true,
Expand Down Expand Up @@ -63,6 +64,21 @@ module.exports = {
return;
}

if (
closingSlash.type !== 'Boolean'
&& closingSlash.type !== 'Null'
&& closingSlash.type !== 'Identifier'
&& closingSlash.type !== 'Keyword'
&& closingSlash.type !== 'Punctuator'
&& closingSlash.type !== 'JSXIdentifier'
&& closingSlash.type !== 'JSXText'
&& closingSlash.type !== 'Numeric'
&& closingSlash.type !== 'String'
&& closingSlash.type !== 'RegularExpression'
) {
return;
}
ljharb marked this conversation as resolved.
Show resolved Hide resolved

if (configuration === 'always' && !sourceCode.isSpaceBetweenTokens(leftToken, closingSlash)) {
report(context, messages.needSpaceBeforeClose, 'needSpaceBeforeClose', {
loc: closingSlash.loc.start,
Expand Down