-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[core] Replace indexOf
with includes
#42883
Conversation
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.
Tests are failing. Some conditions must be negated depending on whether the searched term should be present or not:
arr.indexOf(term) === -1
->!array.includes(term)
arr.indexOf(term) !== -1
->array.includes(term)
The same applies when using .indexOf
on strings.
includes
method instead of indexindexOf
with includes
Netlify deploy previewhttps://deploy-preview-42883--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
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.
Looks good after the last changes, but I'd like an additional review from @mui/code-infra since this PR touches many different areas.
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'm not combing through each and every individual line change, but I'm good with the approach. Two things to note:
- Are we aware of any microbenchmarks that compare both approaches?
- In a similar fashion we could look into replacing
someString.indexOf(someOtherString) === 0
withsomeString.startsWith(someOtherString)
. Perhaps we're also using some other pattern ofindexOf
that can be replaced withendsWith
?
edit: Just doing a regex search (.indexOf\([^)]
) and it seems like I can still find occurences not covered by this PR
Looking through the changes here I don't think any of them is important enough to worry about the performance of those methods. |
Absolutely, and my expectation is that there won't be any significant impact, It's not a blocker, I'm just curious. |
The general discourse seems that
Great idea. @k-rajat19 could you update the PR converting ![]()
I did a quick search and couldn't find this pattern. |
Hey @k-rajat19, are you available to make the requested changes? |
Sorry I forgot about this, I will update it soon |
557e93c
to
07e2911
Compare
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.
Thanks @k-rajat19! As @Janpot pointed out there are lots of .indexOf
occurrences not covered by this PR. We can merge this one and if you are up for it, feel free to open an additional PRs to cover all occurrences.
Fixes #42826