Skip to content

Commit 83e71d4

Browse files
committed
test(prefer-const): add original rule tests & fix code
By adding the original rule tests, I was able to identify issues with the rule, that had been caused because of the refactoring. Now, not only I think the code is more readable and maintainable, but also the behavior is as expected, and there are tests to ensure that. There's a tests that has been skipped since it reporting a false positive, which is dues to `@typescript-eslint`. I've reported the issue in typescript-eslint/typescript-eslint#10426
1 parent 3becd21 commit 83e71d4

File tree

2 files changed

+792
-58
lines changed

2 files changed

+792
-58
lines changed

packages/eslint-plugin-svelte/src/rules/prefer-const-helpers/index.ts

+62-57
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ function getDestructuringHost(reference: Reference) {
135135
let node = reference.identifier.parent;
136136
while (PATTERN_TYPE.test(node.type)) {
137137
if (!node.parent) {
138-
return null;
138+
break;
139139
}
140140

141141
node = node.parent;
@@ -278,7 +278,7 @@ function getIdentifierIfShouldBeConst(variable: Variable, ignoreReadBeforeAssign
278278
return variable.defs[0].name;
279279
}
280280

281-
return writer?.identifier;
281+
return writer?.identifier ?? null;
282282
}
283283

284284
/**
@@ -328,16 +328,13 @@ export function isInitOfForStatement(node: ASTNode): boolean {
328328
export function groupByDestructuring(
329329
variables: Variable[],
330330
ignoreReadBeforeAssign: boolean
331-
): Map<ASTNode, ASTNode[]> {
332-
const identifierMap = new Map<ASTNode, ASTNode[]>();
331+
): Map<ASTNode, (ASTNode | null)[]> {
332+
const identifierMap = new Map<ASTNode, (ASTNode | null)[]>();
333333

334334
for (let i = 0; i < variables.length; ++i) {
335335
const variable = variables[i];
336336
const references = variable.references;
337337
const identifier = getIdentifierIfShouldBeConst(variable, ignoreReadBeforeAssign);
338-
if (!identifier) {
339-
continue;
340-
}
341338

342339
let prevId: TSESTree.Identifier | TSESTree.JSXIdentifier | null = null;
343340
references.forEach((reference) => {
@@ -385,7 +382,7 @@ function calculateRetainedTextRange(
385382
}
386383

387384
type NodeReporterOptions = { destructuring: 'any' | 'all' };
388-
type NodeReporter = { report: (nodes: ASTNode[]) => void };
385+
type NodeReporter = { report: (nodes: (ASTNode | null)[]) => void };
389386

390387
/**
391388
* Creates a node reporter function that checks and reports nodes based on the
@@ -408,8 +405,10 @@ export function createNodeReporter(
408405
let checkedName = '';
409406
const shouldMatchAnyDestructuredVariable = destructuring !== 'all';
410407

411-
function checkAndReportNodes(nodes: ASTNode[]): void {
412-
const shouldCheckGroup = nodes.length && shouldMatchAnyDestructuredVariable;
408+
function checkAndReportNodes(initialNodes: (ASTNode | null)[]): void {
409+
const nodes = initialNodes.filter(Boolean) as ASTNode[];
410+
const shouldCheckGroup =
411+
nodes.length && (shouldMatchAnyDestructuredVariable || initialNodes.length === nodes.length);
413412
if (!shouldCheckGroup) {
414413
return;
415414
}
@@ -424,62 +423,70 @@ export function createNodeReporter(
424423
!isVarDecParentNull &&
425424
'declarations' in variableDeclarationParent &&
426425
variableDeclarationParent.declarations.length > 0;
427-
if (!isValidDecParent) {
428-
return;
429-
}
430-
431-
const dec = variableDeclarationParent.declarations[0];
432-
checkDeclarator(dec);
433-
434-
const shouldFix = checkShouldFix(variableDeclarationParent, nodes.length);
435-
if (!shouldFix) {
436-
return;
426+
if (isValidDecParent) {
427+
checkDeclarator(variableDeclarationParent.declarations[0]);
437428
}
438429

430+
const shouldFix =
431+
isValidDecParent &&
432+
checkShouldFix(variableDeclarationParent, nodes.length, initialNodes.length);
439433
const sourceCode = getSourceCode(context);
440434
nodes.filter(skipReactiveValues).forEach((node) => {
441-
report(sourceCode, node, variableDeclarationParent);
435+
report(sourceCode, node, variableDeclarationParent!, shouldFix);
442436
});
443437
}
444438

445439
function report(
446440
sourceCode: ReturnType<typeof getSourceCode>,
447441
node: ASTNode,
448-
nodeParent: VariableDeclaration
442+
nodeParent: ASTNode,
443+
shouldFix: boolean
449444
) {
450445
context.report({
451446
node,
452447
messageId: 'useConst',
453448
// @ts-expect-error Name will exist at point
454449
data: { name: node.name },
455-
fix: (fixer) => {
456-
const letKeywordToken = sourceCode.getFirstToken(nodeParent, {
457-
includeComments: false,
458-
filter: (t) => 'kind' in nodeParent && t.value === nodeParent.kind
459-
});
460-
if (!letKeywordToken) {
461-
return null;
462-
}
463-
464-
const [initialRange, finalRange] = calculateRetainedTextRange(
465-
letKeywordToken.range,
466-
nodeParent.range
467-
);
468-
const prefix = sourceCode.text.slice(...initialRange);
469-
const suffix = sourceCode.text.slice(...finalRange);
470-
471-
return fixer.replaceTextRange([initialRange[0], finalRange[1]], `${prefix}const${suffix}`);
472-
}
450+
fix: !shouldFix
451+
? null
452+
: (fixer) => {
453+
const letKeywordToken = sourceCode.getFirstToken(nodeParent, {
454+
includeComments: false,
455+
filter: (t) => 'kind' in nodeParent && t.value === nodeParent.kind
456+
});
457+
if (!letKeywordToken) {
458+
return null;
459+
}
460+
461+
const [initialRange, finalRange] = calculateRetainedTextRange(
462+
letKeywordToken.range,
463+
nodeParent.range
464+
);
465+
const prefix = sourceCode.text.slice(...initialRange);
466+
const suffix = sourceCode.text.slice(...finalRange);
467+
468+
return fixer.replaceTextRange(
469+
[initialRange[0], finalRange[1]],
470+
`${prefix}const${suffix}`
471+
);
472+
}
473473
});
474474
}
475475

476-
function checkShouldFix(declaration: VariableDeclaration, totalNodes: number) {
476+
function checkShouldFix(
477+
declaration: VariableDeclaration,
478+
totalNodes: number,
479+
totalInitialNodes: number
480+
) {
481+
const isParentForStatement =
482+
declaration?.parent.type === 'ForInStatement' ||
483+
declaration?.parent.type === 'ForOfStatement';
484+
const areAllDeclarationsInitialized =
485+
'declarations' in declaration && declaration.declarations.every((decl) => decl.init);
477486
const shouldFix =
487+
totalNodes === totalInitialNodes &&
478488
declaration &&
479-
(declaration.parent.type === 'ForInStatement' ||
480-
declaration.parent.type === 'ForOfStatement' ||
481-
('declarations' in declaration &&
482-
declaration.declarations.every((declaration) => declaration.init)));
489+
(isParentForStatement || areAllDeclarationsInitialized);
483490

484491
const totalDeclarationCount = checkDestructuredDeclaration(declaration, totalNodes);
485492
if (totalDeclarationCount === -1) {
@@ -490,16 +497,14 @@ export function createNodeReporter(
490497
}
491498

492499
function checkDestructuredDeclaration(declaration: VariableDeclaration, totalNodes: number) {
493-
const hasMultipleDeclarations =
494-
declaration !== null &&
495-
'declarations' in declaration &&
496-
declaration.declarations.length !== 1;
500+
const isValidDeclaration = declaration !== null && 'declarations' in declaration;
501+
const hasMultipleDeclarations = isValidDeclaration && declaration.declarations.length !== 1;
497502
if (!hasMultipleDeclarations) {
498503
return -1;
499504
}
500505

501506
const hasMoreThanOneDeclaration =
502-
declaration && declaration.declarations && declaration.declarations.length >= 1;
507+
declaration.declarations && declaration.declarations.length >= 1;
503508
if (!hasMoreThanOneDeclaration) {
504509
return -1;
505510
}
@@ -524,27 +529,27 @@ export function createNodeReporter(
524529
return;
525530
}
526531

527-
const firstDecParent = declarator.init.parent;
528-
if (firstDecParent.type !== 'VariableDeclarator') {
532+
const firstDeclaratorParent = declarator.init.parent;
533+
if (firstDeclaratorParent.type !== 'VariableDeclarator') {
529534
return;
530535
}
531536

532-
const { id } = firstDecParent;
537+
const { id } = firstDeclaratorParent;
533538
if ('name' in id && id.name !== checkedName) {
534539
checkedName = id.name;
535540
reportCount = 0;
536541
}
537542

538-
if (firstDecParent.id.type === 'ObjectPattern') {
539-
const { init } = firstDecParent;
543+
if (firstDeclaratorParent.id.type === 'ObjectPattern') {
544+
const { init } = firstDeclaratorParent;
540545
if (init && 'name' in init && init.name !== checkedName) {
541546
checkedName = init.name;
542547
reportCount = 0;
543548
}
544549
}
545550

546-
if (firstDecParent.id !== checkedId) {
547-
checkedId = firstDecParent.id;
551+
if (firstDeclaratorParent.id !== checkedId) {
552+
checkedId = firstDeclaratorParent.id;
548553
reportCount = 0;
549554
}
550555
}

0 commit comments

Comments
 (0)