-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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 more indexOf
with includes
#43694
Conversation
@@ -9,7 +9,7 @@ function omit(input, fields) { | |||
const output = {}; | |||
|
|||
Object.keys(input).forEach((prop) => { | |||
if (fields.indexOf(prop) === -1) { | |||
if (!fields.includes(prop)) { |
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.
Just for demonstration purposes, but this is one of those hidden O(nm) complexities that can be reduced down to O(n)+O(m):
function omit(input, fields) {
const output = {};
const fieldSet = new Set(fields)
Object.keys(input).forEach((prop) => {
if (!fieldSet.has(prop)) {
output[prop] = input[prop];
}
});
return output;
}
Not to be changed unless proven to be a real performance improvement though, it adds an extra allocation. This comment is just for information only.
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
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 went through all the changes and they look correct, thanks!
This commit is a follow-up of #42883 (review). Signed-off-by: Eng Zer Jun <[email protected]>
Netlify deploy previewhttps://deploy-preview-43694--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
@@ -496,7 +496,7 @@ const attachPropsTable = ( | |||
|
|||
const requiredProp = | |||
prop.required || | |||
/\.isRequired/.test(prop.type.raw) || | |||
prop.type.raw.includes('.isRequired') || |
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.
This line is failing when raw
is not a string. I pushed this fix:
- prop.type.raw.includes('.isRequired') ||
+ prop.type.raw?.includes('.isRequired') ||
This PR is a follow-up of #42883 (review) and #42883 (review).
/cc @aarongarciah @Janpot