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.

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] Fixes for upcoming eslint upgrade #6667

Merged
merged 8 commits into from
Nov 2, 2022
Merged

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Oct 29, 2022

Bringing in most of the fixes necessary to pass the eslint upgrade in mui/material-ui#34642

Nothing should be breaking. Just pointing out changes to the following: https://github.com/mui/mui-x/pull/6667/files#diff-8a3d1997d458bbb8861a31ed9b2db9137c1389883964b6d546c68b42645f5c96

@Janpot Janpot added the core Infrastructure work going on behind the scenes label Oct 29, 2022
@mui-bot
Copy link

mui-bot commented Oct 29, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-6667--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 754.3 1,198.8 754.3 962.04 173.863
Sort 100k rows ms 649.2 1,194.5 881.3 918.58 173.627
Select 100k rows ms 223.4 424.6 357.1 335.84 68.398
Deselect 100k rows ms 157.2 273.9 182.5 199.78 39.958

Generated by 🚫 dangerJS against 7664fc3

@Janpot Janpot marked this pull request as ready for review October 29, 2022 16:58
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines -12 to +14
return new Promise((resolve) => setTimeout(resolve, ms));
return new Promise((resolve) => {
setTimeout(resolve, ms);
});
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I have always hated these implicit unclear returns.

@@ -258,7 +260,7 @@ export const useGridRowGrouping = (
>(() => {
const sanitizedRowGroupingModel = gridRowGroupingSanitizedModelSelector(apiRef);
const rulesOnLastRowTreeCreation =
apiRef.current.unstable_caches.rowGrouping.rulesOnLastRowTreeCreation;
apiRef.current.unstable_caches.rowGrouping.rulesOnLastRowTreeCreation || [];
Copy link
Member

Choose a reason for hiding this comment

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

Should we have behavior changes in this PR?

Suggested change
apiRef.current.unstable_caches.rowGrouping.rulesOnLastRowTreeCreation || [];
apiRef.current.unstable_caches.rowGrouping.rulesOnLastRowTreeCreation;

Copy link
Member Author

Choose a reason for hiding this comment

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

It replaces https://github.com/mui/mui-x/pull/6667/files#diff-1250ff9bde7c6785848b32e7acde51bdce6c5e7d4d82a884a287d33e0e4d8350L267 which eslint complains in https://eslint.org/docs/latest/rules/default-param-last.

I can ignore it as well

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell it's functionally equivalent, but I removed it. We can bring it back in a separate PR.

@@ -159,7 +159,7 @@ export const ClockPicker = React.forwardRef(function ClockPicker<TDate extends u
components,
componentsProps,
value,
disableIgnoringDatePartForTimeValidation,
disableIgnoringDatePartForTimeValidation = false,
Copy link
Member

@oliviertassinari oliviertassinari Oct 29, 2022

Choose a reason for hiding this comment

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

Same question

Suggested change
disableIgnoringDatePartForTimeValidation = false,
disableIgnoringDatePartForTimeValidation,

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above but for https://github.com/mui/mui-x/pull/6667/files#diff-e2061e7eec043ba1ab45746803975267576725a37d32733b469fc7f8b2079a00L40.

Happy to revert and ignore, but this felt quite safe as it didn't seem to be publicly exposed.

Copy link
Member

Choose a reason for hiding this comment

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

If eslint rules call for it, IMHO, it was a perfectly fine change.
We already default it as false in the function down the line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make a separate PR. That way we have an easy to bissect git history should it run into an unforeseen edge case

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 31, 2022
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Oct 31, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 1, 2022
@Janpot Janpot merged commit 2d912d7 into mui:next Nov 2, 2022
@Janpot Janpot deleted the eslint-upgrades branch November 2, 2022 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants