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
Draft

Add JSDoc type annotations #3731

Show file tree
Hide file tree
Changes from all 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 || typeProp.type !== 'Property') {
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
7 changes: 4 additions & 3 deletions lib/rules/forbid-foreign-prop-types.js
View file
Open in desktop
Expand Up @@ -13,6 +13,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 Expand Up @@ -108,8 +109,8 @@ module.exports = {
&& !ast.isAssignmentLHS(node)
&& !isAllowedAssignment(node)
)) || (
(node.property.type === 'Literal' || node.property.type === 'JSXText')
&& node.property.value === 'propTypes'
// @ts-expect-error The JSXText type is not present in the estree type definitions
(node.property.type === 'Literal' || node.property.type === 'JSXText') && node.property.value === 'propTypes'
&& !ast.isAssignmentLHS(node)
&& !isAllowedAssignment(node)
)
Expand All @@ -121,7 +122,7 @@ module.exports = {
},

ObjectPattern(node) {
const propTypesNode = node.properties.find((property) => property.type === 'Property' && property.key.name === 'propTypes');
const propTypesNode = node.properties.find((property) => property.type === 'Property' && property.key.type === 'Identifier' && property.key.name === 'propTypes');

if (propTypesNode) {
report(context, messages.forbiddenPropType, 'forbiddenPropType', {
Expand Down
14 changes: 10 additions & 4 deletions lib/rules/forbid-prop-types.js
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ const messages = {
forbiddenPropType: 'Prop type "{{target}}" is forbidden',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down Expand Up @@ -195,7 +196,7 @@ module.exports = {
}
if (node.specifiers.length >= 1) {
const propTypesSpecifier = node.specifiers.find((specifier) => (
specifier.imported && specifier.imported.name === 'PropTypes'
specifier.type === 'ImportSpecifier' && specifier.imported && specifier.imported.name === 'PropTypes'
));
if (propTypesSpecifier) {
propTypesPackageName = propTypesSpecifier.local.name;
Expand Down Expand Up @@ -230,13 +231,17 @@ module.exports = {
) {
return;
}
if (node.parent.type !== 'AssignmentExpression') {
return;
}

checkNode(node.parent.right);
},

CallExpression(node) {
if (
node.callee.object
node.callee.type === 'MemberExpression'
&& node.callee.object
&& !isPropTypesPackage(node.callee.object)
&& !propsUtil.isPropTypesDeclaration(node.callee)
) {
Expand All @@ -245,7 +250,8 @@ module.exports = {

if (
node.arguments.length > 0
&& (node.callee.name === 'shape' || astUtil.getPropertyName(node.callee) === 'shape')
&& ((node.callee.type === 'Identifier' && node.callee.name === 'shape') || astUtil.getPropertyName(node.callee) === 'shape')
&& node.arguments[0].type === 'ObjectExpression'
) {
checkProperties(node.arguments[0].properties);
}
Expand All @@ -270,7 +276,7 @@ module.exports = {

ObjectExpression(node) {
node.properties.forEach((property) => {
if (!property.key) {
if (property.type !== 'Property') {
return;
}

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
3 changes: 3 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,9 @@ module.exports = {
*/
function getTokensLocations(node) {
const sourceCode = context.getSourceCode();
// @ts-expect-error The JSXText type is not present in the estree type definitions
const opening = sourceCode.getFirstToken(node).loc.start;
// @ts-expect-error The JSXText type is not present in the estree type definitions
const closing = sourceCode.getLastTokens(node, node.selfClosing ? 2 : 1)[0].loc.start;
const tag = sourceCode.getFirstToken(node.name).loc.start;
let lastProp;
Expand Down
19 changes: 14 additions & 5 deletions lib/rules/jsx-curly-spacing.js
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@ const messages = {
spaceNeededBefore: 'A space is required before \'{{token}}\'',
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
docs: {
Expand Down Expand Up @@ -371,8 +372,10 @@ module.exports = {
}

const sourceCode = context.getSourceCode();
const first = sourceCode.getFirstToken(node);
const last = sourceCode.getLastToken(node);
// @ts-expect-error The types differ between ASTNode and ESTree.Node.
const first = /** @type {import("eslint").AST.Token} */(sourceCode.getFirstToken(node));
// @ts-expect-error The types differ between ASTNode and ESTree.Node.
const last = /** @type {import("eslint").AST.Token} */(sourceCode.getLastToken(node));
let second = sourceCode.getTokenAfter(first, { includeComments: true });
let penultimate = sourceCode.getTokenBefore(last, { includeComments: true });

Expand All @@ -390,11 +393,14 @@ module.exports = {
const isObjectLiteral = first.value === second.value;
const spacing = isObjectLiteral ? config.objectLiteralSpaces : config.when;
if (spacing === SPACING.always) {
// @ts-expect-error second has a Comment type.
if (!sourceCode.isSpaceBetweenTokens(first, second)) {
reportRequiredBeginningSpace(node, first);
} else if (!config.allowMultiline && isMultiline(first, second)) {
} else if (!config.allowMultiline && isMultiline(first, second)
) {
reportNoBeginningNewline(node, first, spacing);
}
// @ts-expect-error penultimate has a Comment type.
if (!sourceCode.isSpaceBetweenTokens(penultimate, last)) {
reportRequiredEndingSpace(node, last);
} else if (!config.allowMultiline && isMultiline(penultimate, last)) {
Expand All @@ -405,13 +411,16 @@ module.exports = {
if (!config.allowMultiline) {
reportNoBeginningNewline(node, first, spacing);
}
// @ts-expect-error second has a Comment type.
} else if (sourceCode.isSpaceBetweenTokens(first, second)) {
reportNoBeginningSpace(node, first);
}
if (isMultiline(penultimate, last)) {
if (!config.allowMultiline) {
reportNoEndingNewline(node, last, spacing);
if (config.allowMultiline) {
return;
}
reportNoEndingNewline(node, last, spacing);
// @ts-expect-error penultimate has a Comment type.
} else if (sourceCode.isSpaceBetweenTokens(penultimate, last)) {
reportNoEndingSpace(node, last);
}
Expand Down
3 changes: 2 additions & 1 deletion 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 @@ -60,7 +61,7 @@ module.exports = {
}

const sourceCode = context.getSourceCode();
const equalToken = sourceCode.getTokenAfter(attrNode.name);
const equalToken = /** @type {import("eslint").AST.Token} */ (sourceCode.getTokenAfter(attrNode.name));
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
2 changes: 2 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,7 @@ module.exports = {
* @return {Number} Indent
*/
function getNodeIndent(node) {
// @ts-expect-error The types differ between ASTNode and ESTree.Node.
let src = context.getSourceCode().getText(node, node.loc.start.column + extraColumnStart);
const lines = src.split('\n');
src = lines[0];
Expand Down
9 changes: 9 additions & 0 deletions lib/rules/jsx-indent.js
View file
Open in desktop
Expand Up @@ -45,6 +45,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 @@ -116,6 +117,7 @@ module.exports = {
}

if (node.type === 'ReturnStatement') {
// @ts-expect-error The types differ between ASTNode and ESTree.Node.
const raw = context.getSourceCode().getText(node);
const lines = raw.split('\n');
if (lines.length > 1) {
Expand Down Expand Up @@ -168,6 +170,7 @@ module.exports = {
* @return {Number} Indent
*/
function getNodeIndent(node, byLastLine, excludeCommas) {
// @ts-expect-error The types differ between ASTNode and ESTree.Node.
let src = context.getSourceCode().getText(node, node.loc.start.column + extraColumnStart);
const lines = src.split('\n');
if (byLastLine) {
Expand Down Expand Up @@ -215,6 +218,7 @@ module.exports = {
&& node.parent.parent
&& node.parent.parent.type === 'ConditionalExpression'
&& node.parent.parent.alternate === node.parent
// @ts-expect-error The types differ between ASTNode and ESTree.Node.
&& context.getSourceCode().getTokenBefore(node).value !== '('
);
}
Expand Down Expand Up @@ -335,24 +339,29 @@ module.exports = {

function handleOpeningElement(node) {
const sourceCode = context.getSourceCode();
/** @type {import("estree").Node | import("eslint").AST.Token | import("estree").Comment} */
let prevToken = sourceCode.getTokenBefore(node);
if (!prevToken) {
return;
}
// Use the parent in a list or an array
if (prevToken.type === 'JSXText' || ((prevToken.type === 'Punctuator') && prevToken.value === ',')) {
prevToken = sourceCode.getNodeByRangeIndex(prevToken.range[0]);
// @ts-expect-error The JSX syntax type does not exist within prevToken.
prevToken = prevToken.type === 'Literal' || prevToken.type === 'JSXText' ? prevToken.parent : prevToken;
// Use the first non-punctuator token in a conditional expression
} else if (prevToken.type === 'Punctuator' && prevToken.value === ':') {
do {
prevToken = sourceCode.getTokenBefore(prevToken);
} while (prevToken.type === 'Punctuator' && prevToken.value !== '/');
prevToken = sourceCode.getNodeByRangeIndex(prevToken.range[0]);
// @ts-expect-error The JSX syntax type does not exist within prevToken.
while (prevToken.parent && prevToken.parent.type !== 'ConditionalExpression') {
// @ts-expect-error The JSX syntax type does not exist within prevToken.
prevToken = prevToken.parent;
}
}
// @ts-expect-error The JSX syntax type does not exist within prevToken.
prevToken = prevToken.type === 'JSXExpressionContainer' ? prevToken.expression : prevToken;
const parentElementIndent = getNodeIndent(prevToken);
const indent = (
Expand Down
15 changes: 9 additions & 6 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);

const nodeParent = /** @type {import("estree").VariableDeclaration} */(node.parent);
if (
blockAncestors.length > 0
&& variableViolationType
&& node.parent.kind === 'const' // only support const right now
&& nodeParent.kind === 'const' // only support const right now
&& node.id.type === 'Identifier'
) {
addVariableNameToSet(
variableViolationType, node.id.name, blockAncestors[0].range[0]
Expand Down