-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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] Remove outdated babel proposal plugins #36795
Conversation
Why the CircleCI failed by |
Hi there? |
Netlify deploy previewhttps://deploy-preview-36795--material-ui.netlify.app/ @material-ui/core: parsed: -3.65% 😍, gzip: -3.68% 😍 Bundle size report |
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.
@kkocdko It's a great first pull request at MUI. Thanks a lot!
For others to see:
class-properties
andprivate-methods
were supported by default inbabel-preset-env
in7.14.0
: https://babel.dev/blog/2021/04/29/7.14.0
you can remove @babel/plugin-proposal-class-properties and @babel/plugin-proposal-private-methods, since they are now enabled by default in @babel/preset-env.
private-property-in-object
was supported by default in7.15.0
- https://babel.dev/blog/2021/07/26/7.15.0object-rest-spread
is also included in ES2018: Add initial support for ES2018 in preset-env babel/babel#7658.
Co-authored-by: ZeeshanTamboli <[email protected]>
This seems to break Safari compatibility: https://mui.com/material-ui/getting-started/supported-platforms/#browser, or at minimum the CI is always red now: ![]() I think that we need to fix this. I landed here from https://www.notion.so/mui-org/KPIs-1ce9658b85ce4628a2a2ed2ae74ff69c?pvs=4#f3c0c606c62148f1bd06784f367bd8c4 ![]() |
These test run on Safari 13.1.2, which we do not support. |
We run the browser tests in master branch on Safari 13.1.2, while in the documentation we mention to support Safari (macOS) >=14. Also in |
Agree. They should be run on PRs to be visible. @oliviertassinari, would it significantly impact us financially if we enabled BrowserStack tests on each PR? |
The docs says we support Safari from v12.5: https://mui.com/material-ui/getting-started/supported-platforms/#browser ![]() which I believe is the same between browsers and mobiles.
No impact, we have a community plan. The main limit is the resources available in BrowserStack under this plan. https://www.browserstack.com/accounts/profile ![]() We could also maybe use LambdaTest, they were happy to sponsor us: https://www.notion.so/mui-org/Accounts-945f0ccb9f7749ccb9e84b9eb9b13ec7?pvs=4#9be9ee4c9a5f428fabd98a67d0825cc3 However, there are browsers that we run in BrowserStack that we could run directly inside the CircleCI runtime, maybe inside a local Selenium instance. Safari seems to the be only one that needs BrowserStack. I moved Firefox to Circle CI runtime with #34764. This PR comes with the cost of no longer testing the right version of Firefox. I suspect that the real solution is with Docker. One thing that could work is to run it based on a modulo of the PR number. Like for 1/3 of the PR and see how it goes. But then, we need a way to run it on a case by case for PRs, e.g. for PRs like this one. |
How about we enable BrowserStack on all PRs (and perhaps disable them on master) and see what is the impact, then we can adjust if necessary? |
This reverts commit 5582ad1.
@michaldudak This is how we used to do it in the past. No objections. I'm not aware of fundamental changes that would make it work this time, so I think it's great to fully experience the problem, as a first step toward a solution. |
@oliviertassinari It did cause me some issues, but I was able to work around them. My understanding was that this PR removed the plugins because the resulting polyfills were already included / could be enabled without them. I thought the extra info that |
Close #36794
https://deploy-preview-36795--material-ui.netlify.app/