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] Begin removing IE 11-related code #41709

Merged
merged 9 commits into from
Apr 16, 2024

Conversation

iammminzzy
Copy link
Contributor

@iammminzzy iammminzzy commented Mar 30, 2024

See #14420.
Per #41611 (comment) I have cherry-picked code changes from #41611 into a separate PR:

  • remove legacy bundle
  • remove workarounds for IE 11

@mui-bot
Copy link

mui-bot commented Mar 30, 2024

Netlify deploy preview

https://deploy-preview-41709--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 974c9ec

@ZeeshanTamboli ZeeshanTamboli added breaking change core Infrastructure work going on behind the scenes labels Mar 30, 2024
@zannager zannager requested a review from siriwatknp April 1, 2024 13:52
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 3, 2024
@iammminzzy iammminzzy force-pushed the remove-ie11-js-references branch from fe72fc3 to 3663cca Compare April 5, 2024 10:04
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 5, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Could you please remove the no IE 11 support comment statement from the .eslintrc.js file at https://github.com/mui/material-ui/blob/next/.eslintrc.js#L167. We can handle this in the current PR.

Also, I've noticed that you haven't removed all instances of IE11 from the JS/TS code (not CSS), but I believe we can address that in separate PRs.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@iammminzzy Apologies for not mentioning this in the previous review, but could you also address the following tasks in this PR:

  1. Replace stableSort in the following locations:

  2. Remove the comment text The property isn't supported by IE11 from:

  3. Remove the comment text ⚠️ sticky is not supported by IE11. from:

  4. Remove the comment text The property isn't supported by IE11. from:

  5. Remove the comment from:

  6. Replace the usage of isNaN with Number.isNaN at:

I believe these tasks can be addressed in this PR. The remaining tasks can be handled separately with thorough testing.

@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented Apr 9, 2024

Replace the usage of isNaN with Number.isNaN

Per the original PR description, it’s not as simple as that. If you actually analyse the code, you’ll see that what gets stored is always a number, never a NaN. The only reason isNaN works there is because it treats undefined (indicatorStyle is an empty object initially, keying into it will result in undefined) as “not a number”, but Number.isNaN won’t (it only returns true for an actual NaN). A typeof check was suggested (because it would skip over undefined) but dismissed in the original review.

@iammminzzy iammminzzy force-pushed the remove-ie11-js-references branch from 1c03796 to 382f3c3 Compare April 9, 2024 11:12
@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Apr 10, 2024

Replace the usage of isNaN with Number.isNaN

Per the original PR description, it’s not as simple as that. If you actually analyse the code, you’ll see that what gets stored is always a number, never a NaN. The only reason isNaN works there is because it treats undefined (indicatorStyle is an empty object initially, keying into it will result in undefined) as “not a number”, but Number.isNaN won’t (it only returns true for an actual NaN). A typeof check was suggested (because it would skip over undefined) but dismissed in the original review.

You're correct. It seems that the value never becomes NaN. We can simplify this by using a typeof check instead:

--- a/packages/mui-material/src/Tabs/Tabs.js
+++ b/packages/mui-material/src/Tabs/Tabs.js
@@ -420,9 +420,10 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
       [size]: tabMeta ? tabMeta[size] : 0,
     };

-    // IE11 support, replace with Number.isNaN
-    // eslint-disable-next-line no-restricted-globals
-    if (isNaN(indicatorStyle[startIndicator]) || isNaN(indicatorStyle[size])) {
+    if (
+      typeof indicatorStyle[startIndicator] !== 'number' ||
+      typeof indicatorStyle[size] !== 'number'
+    ) {
       setIndicatorStyle(newIndicatorStyle);
     } else {
       const dStart = Math.abs(indicatorStyle[startIndicator] - newIndicatorStyle[startIndicator]);

Looking back at the history, I'm not sure why it was added in #8831. It appears that it never actually became the global NaN type. Changing Number.isNaN to the global isNaN was done in #11832 for IE 11 support.

I believe using a typeof check should be safe. If you're unsure about this change, we can revert it and address it separately in another PR.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Looks good to me. We just need approval from @mui/code-infra before merging.

@ZeeshanTamboli ZeeshanTamboli requested a review from a team April 10, 2024 07:54
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.

I see nothing out of the ordinary, but let's wait for @michaldudak 's review before merging.

Some comments that are out of scope for this PR:

  • Are we polyfilling .toSorted already? To do data.toStorted() instead of [...data].sort().
  • Intuitively I'd expect an array sort in a render to be memoized in 99% of the cases.

@NMinhNguyen
Copy link
Contributor

  • Are we polyfilling .toSorted already? To do data.toStorted() instead of [...data].sort().

We did consider that but TypeScript seems to be configured with lib es2020, so using toSorted led to an error.

@iammminzzy iammminzzy force-pushed the remove-ie11-js-references branch from 2e098c4 to 974c9ec Compare April 11, 2024 14:40
@ZeeshanTamboli ZeeshanTamboli merged commit bba4f7c into mui:next Apr 16, 2024
22 checks passed
@DiegoAndai
Copy link
Member

Hey @iammminzzy! Thanks for working on this.

We should add any breaking changes to the migration guide: https://github.com/iammminzzy/material-ui/blob/98494bd51919f4b299ffa864c0097f3bea134d75/docs/data/material/migration/migration-v5/migration-v5.md

May I ask you to add a section summarizing the changes on this PR and #41611

@iammminzzy iammminzzy deleted the remove-ie11-js-references branch April 23, 2024 16:27
@iammminzzy iammminzzy restored the remove-ie11-js-references branch April 23, 2024 16:28
@iammminzzy iammminzzy deleted the remove-ie11-js-references branch April 23, 2024 16:28
@iammminzzy iammminzzy mentioned this pull request Apr 23, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change core Infrastructure work going on behind the scenes v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants