prefer-string-replace-all: Check pattern even if it's already using…

… `.replaceAll` (#1981)
sindresorhus · Nov 20, 2022 · e8c5156 · e8c5156
1 parent 5d90d73
commit e8c5156
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 9 deletions.
diff --git a/docs/rules/prefer-string-replace-all.md b/docs/rules/prefer-string-replace-all.md
@@ -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
@@ -44,3 +48,7 @@ string.replaceAll('string', '');
 ```js
 string.replaceAll(/\s/, '');
 ```
+
+```js
+string.replaceAll('foo', 'bar');
+```
diff --git a/rules/prefer-string-replace-all.js b/rules/prefer-string-replace-all.js
@@ -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;
 	}
@@ -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) => {
@@ -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',
+				},
+				/** @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);
 			},
 		};
 	},

diff --git a/test/prefer-string-replace-all.mjs b/test/prefer-string-replace-all.mjs
@@ -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)',
@@ -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)',
@@ -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, _)`,
 	],
 });
diff --git a/test/snapshots/prefer-string-replace-all.mjs.md b/test/snapshots/prefer-string-replace-all.mjs.md
@@ -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.␊
+    `
diff --git a/test/snapshots/prefer-string-replace-all.mjs.snap b/test/snapshots/prefer-string-replace-all.mjs.snap