Skip to content

Conversation

@alexdima
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings October 31, 2025 19:50
@alexdima alexdima enabled auto-merge (squash) October 31, 2025 19:50
@alexdima alexdima self-assigned this Oct 31, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new ESLint rule code-no-bracket-notation-for-identifiers to discourage the use of bracket notation for accessing properties that are valid JavaScript identifiers. The rule aims to prevent bypassing TypeScript's type checking and access modifiers by using bracket notation (e.g., obj['_privateMember'] instead of obj._privateMember).

Key changes:

  • New ESLint rule implementation with auto-fix capability
  • Rule is enabled globally with 'warn' level in the ESLint configuration

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
eslint.config.js Enables the new rule code-no-bracket-notation-for-identifiers at the 'warn' level
.eslint-plugin-local/code-no-bracket-notation-for-identifiers.ts Implements the ESLint rule to detect and auto-fix bracket notation usage for valid identifiers

Comment on lines +73 to +77
// We need to replace the ['property'] part with .property
const sourceCode = context.getSourceCode();
const objectText = sourceCode.getText(memberExpr.object as unknown as eslint.Rule.Node);
const dotNotation = `${objectText}.${propertyName}`;
return fixer.replaceText(node, dotNotation);
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The fix implementation replaces the entire MemberExpression node with reconstructed text. This approach fails to preserve whitespace, comments, and complex expressions properly. It should use fixer.replaceTextRange targeting only the bracket portion (from the opening bracket to the closing bracket) and replacing it with .${propertyName}. For example, replace the range from the start of '[' to the end of ']' instead of the entire node.

Suggested change
// We need to replace the ['property'] part with .property
const sourceCode = context.getSourceCode();
const objectText = sourceCode.getText(memberExpr.object as unknown as eslint.Rule.Node);
const dotNotation = `${objectText}.${propertyName}`;
return fixer.replaceText(node, dotNotation);
// Only replace the ['property'] part with .property, preserving whitespace/comments
const sourceCode = context.getSourceCode();
const leftBracket = sourceCode.getTokenBefore(memberExpr.property, token => token.value === '[');
const rightBracket = sourceCode.getTokenAfter(memberExpr.property, token => token.value === ']');
if (!leftBracket || !rightBracket) {
return null;
}
return fixer.replaceTextRange(
[leftBracket.range[0], rightBracket.range[1]],
'.' + propertyName
);

Copilot uses AI. Check for mistakes.
Comment on lines +41 to +44
// Check if it's a valid JavaScript identifier
// Must start with letter, underscore, or dollar sign
// Can contain letters, digits, underscores, or dollar signs
return /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(str);
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The regular expression doesn't account for reserved JavaScript keywords. Properties like 'class', 'const', 'function', etc. are valid in bracket notation but cannot be used with dot notation. Add a check to exclude reserved keywords: const reservedWords = new Set(['break', 'case', 'catch', 'class', 'const', 'continue', 'debugger', 'default', 'delete', 'do', 'else', 'export', 'extends', 'finally', 'for', 'function', 'if', 'import', 'in', 'instanceof', 'new', 'return', 'super', 'switch', 'this', 'throw', 'try', 'typeof', 'var', 'void', 'while', 'with', 'yield', 'let', 'static', 'enum', 'await', 'implements', 'interface', 'package', 'private', 'protected', 'public']); return /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(str) && !reservedWords.has(str);

Suggested change
// Check if it's a valid JavaScript identifier
// Must start with letter, underscore, or dollar sign
// Can contain letters, digits, underscores, or dollar signs
return /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(str);
// Check if it's a valid JavaScript identifier and not a reserved word
// Must start with letter, underscore, or dollar sign
// Can contain letters, digits, underscores, or dollar signs
const reservedWords = new Set([
'break', 'case', 'catch', 'class', 'const', 'continue', 'debugger', 'default', 'delete', 'do', 'else',
'export', 'extends', 'finally', 'for', 'function', 'if', 'import', 'in', 'instanceof', 'new', 'return',
'super', 'switch', 'this', 'throw', 'try', 'typeof', 'var', 'void', 'while', 'with', 'yield', 'let',
'static', 'enum', 'await', 'implements', 'interface', 'package', 'private', 'protected', 'public'
]);
return /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(str) && !reservedWords.has(str);

Copilot uses AI. Check for mistakes.
@alexdima alexdima disabled auto-merge October 31, 2025 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants