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

[docs] Drop IE 11 official support #41611

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

iammminzzy
Copy link
Contributor

@iammminzzy iammminzzy commented Mar 23, 2024

This is the first step towards #14420. This PR official communicates that IE 11 is no longer supported. This is a breaking change.

More changes will be required to close the issue, but they won't be breaking changes in theory as IE 11 is no longer supported #41611 (comment).

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 Thank you for the pull request. Apart from the code updates, we should also remove the legacy bundle build, as discussed in #41568 (refer to #41568 (comment)). Additionally, we need to remove ie 11 from the .browserslistrc file. Perhaps we should also update the docs.

@ZeeshanTamboli ZeeshanTamboli added breaking change core Infrastructure work going on behind the scenes v6.x labels Mar 25, 2024
@iammminzzy
Copy link
Contributor Author

@iammminzzy Thank you for the pull request. Apart from the code updates, we should also remove the legacy bundle build, as discussed in #41568 (refer to #41568 (comment)). Additionally, we need to remove ie 11 from the .browserslistrc file. Perhaps we should also update the docs.

Thank you for your advice. Should I push these changes to this PR or can this PR be landed as an incremental improvement, since there are still many more things to handle to remove IE 11 support completely?

@ZeeshanTamboli
Copy link
Member

@iammminzzy Thank you for the pull request. Apart from the code updates, we should also remove the legacy bundle build, as discussed in #41568 (refer to #41568 (comment)). Additionally, we need to remove ie 11 from the .browserslistrc file. Perhaps we should also update the docs.

Thank you for your advice. Should I push these changes to this PR or can this PR be landed as an incremental improvement, since there are still many more things to handle to remove IE 11 support completely?

This should be done in a single PR; otherwise, it might remain incomplete if not addressed in separate PRs.

@iammminzzy
Copy link
Contributor Author

@iammminzzy Thank you for the pull request. Apart from the code updates, we should also remove the legacy bundle build, as discussed in #41568 (refer to #41568 (comment)). Additionally, we need to remove ie 11 from the .browserslistrc file. Perhaps we should also update the docs.

Thank you for your advice. Should I push these changes to this PR or can this PR be landed as an incremental improvement, since there are still many more things to handle to remove IE 11 support completely?

This should be done in a single PR; otherwise, it might remain incomplete if not addressed in separate PRs.

I’m happy to look into how the packages get built, but I feel like a core maintainer would be better positioned to address these things in a follow-up PR

@iammminzzy
Copy link
Contributor Author

@ZeeshanTamboli I followed the approach in #41568 and removed legacy bundle and updated the docs. Feel free to push any further updates to this 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.

There were many style-related references but I thought they were riskier to change, so I left those alone.

Could you try to replace them?

There was also

// IE11 support, replace with Number.isNaN
// eslint-disable-next-line no-restricted-globals
if (isNaN(indicatorStyle[startIndicator]) || isNaN(indicatorStyle[size])) {
but changing isNaN to Number.isNaN seemed wrong despite what the comment says (I believe typeof indicatorStyle[startIndicator] === 'number' should be used).

Why Number.isNaN seems wrong from typeof number?

Additionally, I wasn't sure if stableSort() was safe to drop because I'm not sure if 2020 browsers are meant to be supported:

// Since 2020 all major browsers ensure sort stability with Array.prototype.sort().
// stableSort() brings sort stability to non-modern browsers (notably IE11). If you
// only support modern browsers you can replace stableSort(exampleArray, exampleComparator)
// with exampleArray.slice().sort(exampleComparator)
function stableSort<T>(array: readonly T[], comparator: (a: T, b: T) => number) {

I believe we can safely remove it. It's also included in a demo, and we are supporting browsers that fully support Array.prototype.sort().

babel.config.js Outdated Show resolved Hide resolved
@ZeeshanTamboli ZeeshanTamboli requested a review from a team March 28, 2024 10:16
@ZeeshanTamboli ZeeshanTamboli changed the title [core] Remove some IE11 support [core] Remove IE11 support Mar 28, 2024
@NMinhNguyen
Copy link
Contributor

@ZeeshanTamboli I’m curious as to why the scope of this PR has to keep increasing, when the author evidently intended to only help clean up some JS references. The original PR that updated browserslist also didn’t try to solve the whole old browser support issue and was landed in an incremental fashion. Surely all these other issues can be addressed in follow-up PRs by a core member?

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Mar 29, 2024

@ZeeshanTamboli I’m curious as to why the scope of this PR has to keep increasing, when the author evidently intended to only help clean up some JS references.

@NMinhNguyen I believe we shouldn't remove the old code references until the browser support has been officially removed. If we ship the replaced code without actually removing the browser support, it could lead to potential issues. What's the guarantee that the removal of IE 11 browser will be in the same release as this? We need to ensure that the removal of IE 11 browser support coincides with this change.

The original PR that updated browserslist also didn’t try to solve the whole old browser support issue and was landed in an incremental fashion.

It only updated the supported browsers list because the removal of IE 11 could be done separately. In my opinion, they were not interconnected.

Surely all these other issues can be addressed in follow-up PRs by a core member?

If you want us to handle it, you can close this PR.

@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented Mar 29, 2024

What's the guarantee that the removal of IE 11 browser will be in the same release as this?

There is a stated goal of removing IE11 support as part of v6, and it's relatively recent that the repo has switched from master to next as the main branch, so I would say that there is more than enough time for this work to be picked up subsequently, without needing to block this contribution.

it could lead to potential issues.

What issues exactly? V6 states that IE11 is no longer supported, so any issues arising from not having polyfills for the native JS APIs that this PR introduces, are expected.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 29, 2024

Surely all these other issues can be addressed in follow-up PRs by a core member?

The smallest possible scope of this PR is to update all docs to not mention IE 11 anymore. It's like a 10 lines removal.

Closing #14420 requires updating all the code. So this could be one or many PRs, whatever is smoother.

@NMinhNguyen
Copy link
Contributor

Surely all these other issues can be addressed in follow-up PRs by a core member?

The smallest possible scope of this PR is to update all docs to not mention IE 11 anymore. It's like a 10 lines removal.

Closing #14420 requires updating all the code. So this could be one or many PRs, whatever is smoother.

Sounds good, I think this PR should definitely have a limited scope and not close the linked issue, merely reference, which is what it did before @ZeeshanTamboli ’s edit.

Let’s split out the JS reference changes into a separate PR that also doesn’t close the linked issue, merely reference.

Finally, once all the IE references are removed (CSS etc.), then the issue can be closed in that PR.

@iammminzzy iammminzzy changed the title [core] Remove IE11 support [docs] Stop mentioning IE11 Mar 29, 2024
@iammminzzy iammminzzy changed the title [docs] Stop mentioning IE11 [docs] Stop mentioning IE 11 Mar 29, 2024
@iammminzzy iammminzzy force-pushed the remove-ie11-support branch 3 times, most recently from 414e19d to c58af61 Compare March 29, 2024 23:51
@iammminzzy
Copy link
Contributor Author

Thanks for your input, everyone!
I have removed all IE references from .md files and reverted all other changes.

Please let me know if this is in a mergeable state now.

@iammminzzy iammminzzy force-pushed the remove-ie11-support branch 2 times, most recently from 55425f4 to 24de0fd Compare March 30, 2024 00:06
@iammminzzy iammminzzy changed the title [docs] Stop mentioning IE 11 [docs] Remove IE 11 references Mar 30, 2024
Signed-off-by: Olivier Tassinari <[email protected]>
@oliviertassinari oliviertassinari changed the title [docs] Remove IE 11 references [docs] Drop IE 11 official support Mar 30, 2024
@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Mar 30, 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.

I still don't fully agree with the decision to separate it, but if that's what's been decided, then I'm okay with it. As a community contributor rather than a team member, my suggestions may not carry much weight. Anyway you have already created another PR to drop the code: #41709, so it's fine. 👍

I've identified two other places where we need to remove it:

  1. From the stickyHeader prop of the Table component. You can find it here: Table Prop - stickyHeader. Once text is removed from the type definition, remember to run pnpm proptypes && pnpm docs:api to update the Proptypes and API docs.

  2. Not directly related to Material UI, but we can also remove it from Joy UI:

@NMinhNguyen
Copy link
Contributor

@ZeeshanTamboli thanks for the pointers, I believe the API docs have now been updated too, so there shouldn't be any remaining references to IE in the v6 docs.

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. Since it is a breaking change, I'd like to have others review it as well before merging.

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 no problem with splitting this.

@ZeeshanTamboli ZeeshanTamboli merged commit ebe293c into mui:next Apr 5, 2024
22 checks passed
@ZeeshanTamboli ZeeshanTamboli mentioned this pull request Apr 8, 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 docs Improvements or additions to the documentation v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants