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

perf: don't use grapheme-splitter on ASCII strings in key-spacing rule #17122

Merged
merged 3 commits into from Apr 26, 2023
Merged

perf: don't use grapheme-splitter on ASCII strings in key-spacing rule #17122

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5b80a8
perf: don't use `grapheme-splitter` on ASCII strings in key-spacing rule
mdjermanovic Apr 25, 2023
9b0c49a
extract to string-utils
mdjermanovic Apr 26, 2023
850014d
remove extra blank line
mdjermanovic Apr 26, 2023
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
27 changes: 24 additions & 3 deletions lib/rules/key-spacing.js
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this! Since this code is so similar to what we have in id-length.js would it be possible to move getStringLength() to ast-utils or to another shared file in order to avoid duplications? The different regex range should not be a problem: we could just use \u0000-\u007f and be safe in all cases, even if id-length would never use the lower part of the range, because it's not relevant to identifiers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks for the suggestion!

Expand Up @@ -11,12 +11,33 @@
const astUtils = require("./utils/ast-utils");
const GraphemeSplitter = require("grapheme-splitter");

const splitter = new GraphemeSplitter();

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

// eslint-disable-next-line no-control-regex -- intentionally including control characters
const ASCII_REGEX = /^[\u0000-\u007f]*$/u;

/** @type {GraphemeSplitter | undefined} */
let splitter;

/**
* Gets the length of the string. If the string is not in ASCII, counts graphemes.
* @param {string} value A string to get the length.
* @returns {number} The length of `value`.
*/
function getStringLength(value) {
if (ASCII_REGEX.test(value)) {
return value.length;
}

if (!splitter) {
splitter = new GraphemeSplitter();
}

return splitter.countGraphemes(value);
}

/**
* Checks whether a string contains a line terminator as defined in
* http://www.ecma-international.org/ecma-262/5.1/#sec-7.3
Expand Down Expand Up @@ -523,7 +544,7 @@ module.exports = {
const startToken = sourceCode.getFirstToken(property);
const endToken = getLastTokenBeforeColon(property.key);

return splitter.countGraphemes(sourceCode.getText().slice(startToken.range[0], endToken.range[1]));
return getStringLength(sourceCode.getText().slice(startToken.range[0], endToken.range[1]));
}

/**
Expand Down