-
Notifications
You must be signed in to change notification settings - Fork 31k
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
lib: limit split function calls to prevent excessive array length #57501
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57501 +/- ##
=======================================
Coverage 90.22% 90.23%
=======================================
Files 629 629
Lines 184847 184846 -1
Branches 36207 36205 -2
=======================================
+ Hits 166780 166797 +17
- Misses 11015 11019 +4
+ Partials 7052 7030 -22
🚀 New features to boost your workflow:
|
@@ -360,7 +360,7 @@ function resolveObjectURL(url) { | |||
try { | |||
const parsed = new URL(url); | |||
|
|||
const split = StringPrototypeSplit(parsed.pathname, ':'); | |||
const split = StringPrototypeSplit(parsed.pathname, ':', 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to improve the performance, we should actually do the splitting manually by checking with indexOf and slicing out the variables we need. The same applies to multiple other places here.
This particular case will only improve the performance in case the pathname is rather untypical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to do that in another pass, same for replacements where we can just use replaceAll
Can I do that or would you like to combine them all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And btw current behavior rejects if more than 1 occurrence of :
is found, so using split here kinda makes sense to get that number before continuing further
This reverts commit 15a414e.
Speeds up split calls by returning early when we reach the desired length
Some of these split calls are used for string replacement, I'll do another pass converting those to
StringPrototypeReplaceAll
afterward