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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃搸 Implement rule useTopLevelRegex #2148

Closed
ematipico opened this issue Mar 21, 2024 · 4 comments 路 Fixed by #2794
Closed

馃搸 Implement rule useTopLevelRegex #2148

ematipico opened this issue Mar 21, 2024 · 4 comments 路 Fixed by #2794
Assignees
@dyc3
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Feature Status: new feature to implement S-Help-wanted Status: you're familiar with the code base and want to help the project

Comments

@ematipico
Copy link
Member

Description

I don't know if there's a similar rule out there, we can add a reference later.

I came up with this rule while looking at many JS PRs out there, and I was surprised that there are many developers that don't adopt the following practice. Given the following code, the rule should trigger a case similar to this:

function foo(someString) {
	return /[a-Z]*/.test(someString)
}

The rule should suggest moving the regex in a top-level variable:

const REGEX = /[a-Z]*/;

function foo(someString) {
	return REGEX.test(someString)
}

The rule should not provide a code action; it should only suggest moving the regex in a variable at the top level of the module.

The reason for this rule is simple: regex requires a parsing phase, which could be expensive for some long ones. V8 and other engines have a long history of parsing, so I'm sure there is some optimisation, but recreating a new regex every time we call the function foo is definitely not optimal.
@ematipico ematipico added S-Help-wanted Status: you're familiar with the code base and want to help the project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Feature Status: new feature to implement labels Mar 21, 2024
@arendjr
Copy link
Contributor

arendjr commented Mar 21, 2024

I like the suggestion 馃憤

Of course there is an obvious downside to it: Placing all regular expressions in the top-level scope will cause startup performance of the bundle to suffer. Usually this is negligible, and if the regexes are used inside a hot loop, it's absolutely the trade-off you want to make. But forcing all regexes to be top-level may swing performance too much in the other direction, because for code paths that are rarely taken, it does make sense to delay the compilation cost.

So I'm not sure I would recommend enabling the rule by default, even if it's a valuable rule to have in our arsenal :)

@rubiesonthesky
Copy link

rubiesonthesky commented Mar 22, 2024 •
edited

Many projects use bundlers, and I wonder will they inline that regex or not. For example webpack. If it inlines it when bundling, then I think this rule can be useless for those users. But on the other hand, you could then argue that it鈥檚 a bug in the bundler.

Tho it may be that bundlers don鈥檛 do this and then there is nothing to worry about.

This is not opposing the rule. It鈥檚 just noting that will this actually have any effect or is there too many cases when this would not be desirable. :)

Edit: Tested with webpack. It seems to not inline the regex, so this rule would still work.

@RiESAEX
Copy link

RiESAEX commented Mar 27, 2024

Because Regex has internal state, lifting or inlining it is a danger task for both humans and compilers.
FYI: https://v8.dev/blog/regexp-tier-up

@dyc3
Copy link
Contributor

dyc3 commented May 9, 2024

I'd like to give this a shot.

@dyc3 dyc3 mentioned this issue May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dyc3 dyc3

Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Feature Status: new feature to implement S-Help-wanted Status: you're familiar with the code base and want to help the project
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(lint): add new js lint: useTopLevelRegex dyc3/biome
5 participants
@arendjr @ematipico @dyc3 @rubiesonthesky @RiESAEX