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] Update Node.js version to v18 on CircleCI, CodeSandbox, and Netlify #37173

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented May 5, 2023

Node.js v14 support ended on 30th April 2023 (last week). We can at-least update node on the CI tools to unblock upgrading dependencies such as #36343, #36347, #37349 which depend on node >=16.

This is same as #33642.

Related to #32546.

Did not update "engines" setting in package.json to avoid breaking change. Should be done in the next major release.


Next.js update on MUI X repo is also blocked - mui/mui-x#8809

@ZeeshanTamboli ZeeshanTamboli added dependencies Update of dependencies core Infrastructure work going on behind the scenes labels May 5, 2023
@mui-bot
Copy link

mui-bot commented May 5, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against eb378a8

@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review May 5, 2023 10:17
@ZeeshanTamboli ZeeshanTamboli requested a review from a team May 5, 2023 10:20
@michaldudak
Copy link
Member

Node 16 is in maintenance and will reach EOL in less than four months. I propose to upgrade to the active LTS release, which is 18.

@ZeeshanTamboli ZeeshanTamboli changed the title [core] Update Node.js version to v16 on CircleCI, CodeSandbox, and Netlify [core] Update Node.js version to v18 on CircleCI, CodeSandbox, and Netlify May 16, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [core] Update Node.js version to v18 on CircleCI, CodeSandbox, and Netlify [core] Update Node.js version to v16 on CircleCI, CodeSandbox, and Netlify May 16, 2023
@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 16, 2023

@michaldudak I tried in 0bdb62a but the CI errored, so I reverted. I don't know why.

Edit: But it's failing even after I revert it back to v16. Something is off.

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 17, 2023

@michaldudak The above issue is solved. It was because of #35619 which was reverted later by Jun in #37296. Don't know why rollup v3 caused an issue. Also, I tried Node 18 again but the build is failing. Hence, I kept it to v16.

@michaldudak
Copy link
Member

I'm looking into this. IMO it should work fine. I have Node 18 installed locally, and the build succeeds.

@michaldudak michaldudak force-pushed the update-node-to-v16-ci-codesandbox-netlify branch from 04a6980 to a0abe75 Compare May 17, 2023 11:23
@ZeeshanTamboli ZeeshanTamboli changed the title [core] Update Node.js version to v16 on CircleCI, CodeSandbox, and Netlify [core] Update Node.js version to v18 on CircleCI, CodeSandbox, and Netlify May 17, 2023
@michaldudak
Copy link
Member

The build was failing with exit code 137, which indicated an out-of-memory error. I upgraded the machine used to run CircleCI tasks to medium+ and it passed. As a side effect, the tasks run faster (for example, test_bundle_size_monitor finished in 6m 25s vs ~9m in the other PRs).

@oliviertassinari, do you approve such a change?

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 18, 2023

Why Node 18 is using more memory than Node 16? Ideally, shouldn't it be more efficient?

@oliviertassinari
Copy link
Member

oliviertassinari commented May 19, 2023

The build was failing with exit code 137

@michaldudak Looking at the CircleCI resources tab, it seems that we over-allocate resources for most steps:

  • test_unit-1 has x2 more than it needs,
  • test_static has almost x2 more than it needs,
  • test_regressions-1 has more than x2 that it needs
  • test_lint has more than x2 that it needs
  • I didn't check the others

We spend $10,800 / year on CircleCI at the moment. This change could cost $5k/year.

Did you consider the opposite direction? mui/mui-x#8796 and mui/mui-x#6850 by @LukasTy

@michaldudak
Copy link
Member

The test_bundle_size_monitor utilizes CPU fully for over 50% of the time. Since this was the problematic task, we could update the resource class just for it. With the ~25% lower run times we may not see such a big increase in costs (medium+ uses 15 credits/min, where medium uses 10/min)

@oliviertassinari
Copy link
Member

oliviertassinari commented May 21, 2023

@michaldudak I would personally favor --concurrency, until this step is the bottleneck to complete the CI, but I think upgrading could work.

Also per #29005, we might be able to better use the extra 2GB of RAM available to complete the test faster.

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 23, 2023

Playwright minor dependency upgrade is also blocked (#37349):


⚠️ Breaking changes

  • npx playwright test no longer works if you install both playwright and @playwright/test. There's no need to install both, since you can always import browser automation APIs from @playwright/test directly:
// automation.ts
import { chromium, firefox, webkit } from '@playwright/test';
/* ... */

Apart from keeping only @playwright/test package, (playwright as a separate package is no longer needed), Node v14 is also dropped.

We need to decide further regarding this PR.


Edit: But looks like only their CI tooling is upgraded - microsoft/playwright#20800 (not engines setting). So maybe, migrating to @playwright/test everywhere could unblock us without this PR.

@ZeeshanTamboli ZeeshanTamboli mentioned this pull request May 23, 2023
1 task
@michaldudak michaldudak force-pushed the update-node-to-v16-ci-codesandbox-netlify branch from e5df90d to 73c0730 Compare May 23, 2023 07:13
@michaldudak
Copy link
Member

michaldudak commented May 23, 2023

Testing bundle_size_monitor job times:

Node 14, concurrency unbounded (current setup), medium resource class

Total time: 7:52
Build time: 2:34
https://app.circleci.com/pipelines/github/mui/material-ui/98444/workflows/80bd46dd-b8d0-44a6-a5e7-5d536a6be678/jobs/524127

Node 16, concurrency unbounded, medium resource class

Total time: 7:40
Build time: 2:30
https://app.circleci.com/pipelines/github/mui/material-ui/98038/workflows/ebc9c5a5-b613-4c9e-a991-79a2d1b76556/jobs/521904

Node 18, concurrency unbounded, medium+ resource class

Total time: 6:25
Build time: 2:01
https://app.circleci.com/pipelines/github/mui/material-ui/98066/workflows/bae04cf3-a226-45de-a33a-ad5483644cbf/jobs/522067

Node 18, concurrency 4, medium resource class

Total time: 8:05
Build time: 2:38
https://app.circleci.com/pipelines/github/mui/material-ui/98447/workflows/3fe71166-bb16-445f-82ce-0bf5b59d1ec3/jobs/524139/steps

Node 18, concurrency 5, medium resource class

Total time: 8:16
Build time: 2:38
https://app.circleci.com/pipelines/github/mui/material-ui/98451/workflows/4da4386c-892b-41ef-9e32-e9cc3a891e5f/jobs/524157

Node 18, concurrency 8, medium resource class

Total time: 7:08
Build time: 2:06
https://app.circleci.com/pipelines/github/mui/material-ui/98461/workflows/6bc9cf83-c1e2-4dec-b469-73f08047b677/jobs/524212/steps

Node 18, concurrency 10, medium resource class

Total time: 7:21
Build time: 2:24
https://app.circleci.com/pipelines/github/mui/material-ui/98466/workflows/3d45ff0b-2181-4b95-8f0a-e619a7887d9a/jobs/524240


Let's stick to concurrency = 8.

@michaldudak
Copy link
Member

@mui/core could you please take a look at this PR?

@michaldudak michaldudak merged commit ff1124e into mui:master Jun 9, 2023
@ZeeshanTamboli ZeeshanTamboli deleted the update-node-to-v16-ci-codesandbox-netlify branch June 9, 2023 13:09
@oliviertassinari
Copy link
Member

Fair enough, looks good 👍

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 dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants