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.

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

prefer-string-replace-all: Check pattern even it's already using .replaceAll #1981

Merged
merged 8 commits into from Nov 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions docs/rules/prefer-string-replace-all.md
Expand Up @@ -27,6 +27,10 @@ string.replace(/\(It also checks for escaped regex symbols\)/g, '');
string.replace(/Works for u flag too/gu, '');
```

```js
string.replaceAll(/foo/g, 'bar');
```

## Pass

```js
Expand All @@ -44,3 +48,7 @@ string.replaceAll('string', '');
```js
string.replaceAll(/\s/, '');
```

```js
string.replaceAll('foo', 'bar');
```
41 changes: 34 additions & 7 deletions rules/prefer-string-replace-all.js
Expand Up @@ -5,17 +5,19 @@ const escapeString = require('./utils/escape-string.js');
const {methodCallSelector} = require('./selectors/index.js');
const {isRegexLiteral, isNewExpression} = require('./ast/index.js');

const MESSAGE_ID = 'prefer-string-replace-all';
const MESSAGE_ID_USE_REPLACE_ALL = 'method';
const MESSAGE_ID_USE_STRING = 'pattern';
const messages = {
[MESSAGE_ID]: 'Prefer `String#replaceAll()` over `String#replace()`.',
[MESSAGE_ID_USE_REPLACE_ALL]: 'Prefer `String#replaceAll()` over `String#replace()`.',
[MESSAGE_ID_USE_STRING]: 'This pattern can be replaced with a string {{replacement}}.',
};

const selector = methodCallSelector({
method: 'replace',
methods: ['replace', 'replaceAll'],
argumentsLength: 2,
});

function * convertRegExpToString(node, fixer) {
function getPatternReplacement(node) {
if (!isRegexLiteral(node)) {
return;
}
Expand All @@ -39,7 +41,7 @@ function * convertRegExpToString(node, fixer) {
// TODO: Preserve escape
const string = String.fromCodePoint(...parts.map(part => part.codePoint));

yield fixer.replaceText(node, escapeString(string));
return escapeString(string);
}

const isRegExpWithGlobalFlag = (node, scope) => {
Expand Down Expand Up @@ -82,13 +84,38 @@ const create = context => ({
return;
}

const methodName = property.name;
const patternReplacement = getPatternReplacement(pattern);

if (methodName === 'replaceAll') {
if (!patternReplacement) {
return;
}

return {
node: pattern,
messageId: MESSAGE_ID_USE_STRING,
data: {
// Show `This pattern can be replaced with a string literal.` for long strings
replacement: patternReplacement.length < 20 ? patternReplacement : 'literal',
sindresorhus marked this conversation as resolved.
Show resolved Hide resolved
},
/** @param {import('eslint').Rule.RuleFixer} fixer */
fix: fixer => fixer.replaceText(pattern, patternReplacement),
};
}

return {
node: property,
messageId: MESSAGE_ID,
messageId: MESSAGE_ID_USE_REPLACE_ALL,
/** @param {import('eslint').Rule.RuleFixer} fixer */
* fix(fixer) {
yield fixer.insertTextAfter(property, 'All');
yield * convertRegExpToString(pattern, fixer);

if (!patternReplacement) {
return;
}

yield fixer.replaceText(pattern, patternReplacement);
},
};
},
Expand Down
17 changes: 15 additions & 2 deletions test/prefer-string-replace-all.mjs
Expand Up @@ -7,25 +7,36 @@ test.snapshot({
valid: [
// No global flag
'foo.replace(/a/, bar)',
'foo.replaceAll(/a/, bar)',
// Not regex literal
'foo.replace("string", bar)',
'foo.replaceAll("string", bar)',
// Not 2 arguments
'foo.replace(/a/g)',
'foo.replaceAll(/a/g)',
'foo.replace(/\\\\./g)',
'foo.replaceAll(/\\\\./g)',
// Not `CallExpression`
'new foo.replace(/a/g, bar)',
'new foo.replaceAll(/a/g, bar)',
// Not `MemberExpression`
'replace(/a/g, bar)',
'replaceAll(/a/g, bar)',
// Computed
'foo[replace](/a/g, bar);',
'foo[replaceAll](/a/g, bar);',
// Not replace
'foo.methodNotReplace(/a/g, bar);',
// `callee.property` is not a `Identifier`
'foo[\'replace\'](/a/g, bar)',
'foo[\'replaceAll\'](/a/g, bar)',
// More or less argument(s)
'foo.replace(/a/g, bar, extra);',
'foo.replaceAll(/a/g, bar, extra);',
'foo.replace();',
'foo.replaceAll();',
'foo.replace(...argumentsArray, ...argumentsArray2)',
'foo.replaceAll(...argumentsArray, ...argumentsArray2)',
// Unknown/non-regexp/non-global value
'foo.replace(unknown, bar)',
'const pattern = new RegExp("foo", unknown); foo.replace(pattern, bar)',
Expand All @@ -36,8 +47,6 @@ test.snapshot({
'const pattern = "not-a-regexp"; foo.replace(pattern, bar)',
'const pattern = new RegExp("foo", "i"); foo.replace(pattern, bar)',
'foo.replace(new NotRegExp("foo", "g"), bar)',
// We are not checking this
'foo.replaceAll(/a/g, bar)',
],
invalid: [
'foo.replace(/a/g, bar)',
Expand Down Expand Up @@ -90,5 +99,9 @@ test.snapshot({
'foo.replace(/\\u{1f600}/gu, _)',
'foo.replace(/\\n/g, _)',
'foo.replace(/\\u{20}/gu, _)',

'foo.replaceAll(/a]/g, _)',
'foo.replaceAll(/\\r\\n\\u{1f600}/gu, _)',
`foo.replaceAll(/a${' very'.repeat(30)} string/g, _)`,
],
});
48 changes: 48 additions & 0 deletions test/snapshots/prefer-string-replace-all.mjs.md
Expand Up @@ -565,3 +565,51 @@ Generated by [AVA](https://avajs.dev).
> 1 | foo.replace(/\\u{20}/gu, _)␊
| ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊
`

## Invalid #35
1 | foo.replaceAll(/a]/g, _)

> Output

`␊
1 | foo.replaceAll('a]', _)␊
`

> Error 1/1

`␊
> 1 | foo.replaceAll(/a]/g, _)␊
| ^^^^^ This pattern can be replaced with a string 'a]'.␊
`

## Invalid #36
1 | foo.replaceAll(/\r\n\u{1f600}/gu, _)

> Output

`␊
1 | foo.replaceAll('\\r\\n😀', _)␊
`

> Error 1/1

`␊
> 1 | foo.replaceAll(/\\r\\n\\u{1f600}/gu, _)␊
| ^^^^^^^^^^^^^^^^^ This pattern can be replaced with a string '\\r\\n😀'.␊
`

## Invalid #37
1 | foo.replaceAll(/a very very very very very very very very very very very very very very very very very very very very very very very very very very very very very very string/g, _)

> Output

`␊
1 | foo.replaceAll('a very very very very very very very very very very very very very very very very very very very very very very very very very very very very very very string', _)␊
`

> Error 1/1

`␊
> 1 | foo.replaceAll(/a very very very very very very very very very very very very very very very very very very very very very very very very very very very very very very string/g, _)␊
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This pattern can be replaced with a string literal.␊
`
Binary file modified test/snapshots/prefer-string-replace-all.mjs.snap
Binary file not shown.