Skip to content

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented Aug 21, 2025

See context - mui/mui-public#556

Major changes -

  1. @typescript-eslint/no-non-null-asserted-optional-chain has been turned on and will report error. So no more of doing object?.property! this. I have disabled the current issues and fixed in places where I could.
  2. No V extends any since it doesn't actually do anything.
  3. class-methods-use-this has been turned off since we endup always disabling it anyway.

📈 BENCHMARK RESULTS

Command: pnpm eslint:ci

This PR master
⏱️ Average 1:07.68 1:13.31
🚀 Fastest 1:04.33 1:07.25
🐌 Slowest 1:09.33 1:21.43
📊 Std Dev 1.32s 4.04s

@brijeshb42 brijeshb42 added scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Aug 21, 2025
@mui-bot
Copy link

mui-bot commented Aug 21, 2025

Deploy preview: https://deploy-preview-19269--material-ui-x.netlify.app/

Bundle size report

Bundle Parsed size Gzip size
@mui/x-data-grid ▼-78B(-0.02%) ▼-18B(-0.02%)
@mui/x-data-grid-pro ▼-78B(-0.02%) ▼-12B(-0.01%)
@mui/x-data-grid-premium ▼-67B(-0.01%) ▼-11B(-0.01%)
@mui/x-charts ▼-2B(0.00%) ▼-1B(0.00%)
@mui/x-charts-pro ▼-2B(0.00%) 🔺+1B(0.00%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 🔺+7B(+0.01%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 🔺+3B(+0.01%)

Details of bundle changes

Generated by 🚫 dangerJS against edaa791

/* I use this method to parse whatever component props are passed in and format them for the code example,
so the code example includes the same props as the rendered component. e.g. the views={['month']} */
// eslint-disable-next-line @typescript-eslint/no-wrapper-object-types
function formatComponentProps(componentProps?: Object, spacing: number = 1) {
Copy link
Member

Choose a reason for hiding this comment

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

We can't just update the type here?

// TODO move to @mui/internal-code-infra/eslint, these are false positive
'react/no-unstable-nested-components': ['error', { allowAsProps: true }],
// migration rules
'@typescript-eslint/no-namespace': 'off',
Copy link
Member

Choose a reason for hiding this comment

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

We already have

mui-x/eslint.config.mjs

Lines 417 to 423 in c478774

{
// TODO: typescript namespaces found to be harmful. Refactor to different patterns. More info: https://github.com/mui/mui-x/pull/19071
ignores: ['packages/x-scheduler/**/*', 'packages/x-virtualizer/**/*'],
rules: {
'@typescript-eslint/no-namespace': 'error',
},
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are different. By default, the rule is turned off and only enabled for x-scheduler and x-virtualizer packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake. Its ignores and not files.

): GridValidRowModel[] => {
let filteredRows = [...rows];
if (filterModel && filterModel.quickFilterValues?.length! > 0) {
if (filterModel && (filterModel.quickFilterValues?.length as number) > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

If quickFilterValues can be nullable, then why not just handle it like

Suggested change
if (filterModel && (filterModel.quickFilterValues?.length as number) > 0) {
if (filterModel?.quickFilterValues && filterModel.quickFilterValues.length > 0) {

}

// eslint-disable-next-line @typescript-eslint/no-non-null-asserted-optional-chain
const getOptionValue = resolvedColumn?.getOptionValue!;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, Can't unsee now how ridiculous this pattern actually is 😄

@brijeshb42 brijeshb42 force-pushed the migrate-eslint branch 3 times, most recently from 685dbad to 507e709 Compare August 21, 2025 14:47
@brijeshb42 brijeshb42 added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. and removed type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Aug 21, 2025
@brijeshb42 brijeshb42 added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. and removed type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Aug 21, 2025
@brijeshb42 brijeshb42 requested a review from Janpot August 21, 2025 16:45
@brijeshb42 brijeshb42 marked this pull request as ready for review August 22, 2025 04:58
@brijeshb42 brijeshb42 requested a review from a team August 22, 2025 04:58
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

@brijeshb42 brijeshb42 added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. and removed type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Aug 22, 2025

const metrics = (rotationScale?.domain() as (string | number)[]) ?? [];
// eslint-disable-next-line @typescript-eslint/no-non-null-asserted-optional-chain
const angles = metrics.map((key) => rotationScale?.(key)!);
Copy link
Member

Choose a reason for hiding this comment

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

@alexfauquette should do something if rotationScale is not present instead? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

is not if rotationScale is not present. It's if key is not part of rotationScale domain. Which is not possible

if (rotationScale === undefined) { return []; }

const metrics = rotationScale.domain() as (string | number)[];
const angles = metrics.map((key) => rotationScale(key)!);

To remove the ! we should support the case where scale return undefined but it feels weirds to handle a case that should not exist just to please TS

Copy link
Member

Choose a reason for hiding this comment

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

The eslint rule issue is with forcing an optional rotationScale(key)! would be acceptable.

Copy link
Member

@Janpot Janpot Aug 25, 2025

Choose a reason for hiding this comment

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

And if you're certain that rotationScale can't have nullish values, you should express that as rotationScale!(key)!, not rotationScale?.(key)!.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I have changed it to rotationScale!.(key)! to satisfy TS without any runtime changes.

const isOptionEqualToValue = React.useCallback(
(option: ValueOptions, value: ValueOptions) => getOptionValue(option) === getOptionValue(value),
(option: ValueOptions, value: ValueOptions) =>
getOptionValue?.(option) === getOptionValue?.(value),
Copy link
Member

@Janpot Janpot Aug 22, 2025

Choose a reason for hiding this comment

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

tbh, this still feels wrong, this would always return true if getOptionValue is undefined, even if value and option are not equivalent. If there's a valid reason where resolvedColumn?.getOptionLabel can be undefined, shouldn't it be more like:

const getOptionValue = resolvedColumn?.getOptionValue ?? (value) => value;
const getOptionLabel = resolvedColumn?.getOptionLabel ?? (label) => label;

?

Otherwise can we express it correctly in the types? I don't have enough context to fully judge this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to meddle too much with the runtime because of the same reason. @cherniavskii Could you get this resolved ?

Copy link
Member

Choose a reason for hiding this comment

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

OK, should be done.
Basically, if resolvedColumn is not a single select column, we should just return null.
I've made getOptionLabel and getOptionValue non-nullable, because we assign default ones anyway.

typeof scrollCoordinates.left !== undefined ||
typeof scrollCoordinates.top !== undefined
typeof scrollCoordinates.left !== 'undefined' ||
typeof scrollCoordinates.top !== 'undefined'
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

@brijeshb42 brijeshb42 added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. and removed type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Aug 25, 2025
@brijeshb42 brijeshb42 added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. and removed type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Aug 26, 2025
@brijeshb42 brijeshb42 requested a review from Janpot August 26, 2025 04:33
@brijeshb42 brijeshb42 force-pushed the migrate-eslint branch 2 times, most recently from ec9a539 to 53482d3 Compare August 26, 2025 04:39
@brijeshb42 brijeshb42 added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. and removed type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Aug 26, 2025
@brijeshb42 brijeshb42 merged commit 90d94c7 into mui:master Aug 26, 2025
22 of 23 checks passed
@brijeshb42 brijeshb42 deleted the migrate-eslint branch August 26, 2025 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants