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] Upgrade node to v14 #4999

Merged
merged 3 commits into from
Aug 2, 2022
Merged

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented May 24, 2022

Node 12 support ended on April 30th
Node 14 is currently Maintenance LTS, Node 16 - Active LTS (see https://nodejs.org/en/about/releases/)

I didn't update engines entry in our packages, because:

  • I'm not sure if it should be considered as breaking change
  • technically nothing changes in our packages - we'll just use node v14 in CI and deployment environments

@cherniavskii cherniavskii mentioned this pull request May 24, 2022
1 task
@mui-bot
Copy link

mui-bot commented May 24, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 243.8 467.4 346.1 339.5 79.238
Sort 100k rows ms 458.8 814.5 801.5 713 137.297
Select 100k rows ms 134.2 257.7 213.5 210.78 44.743
Deselect 100k rows ms 118.8 211 170.3 169.56 35.173

Generated by 🚫 dangerJS against 8a9da27

@oliviertassinari
Copy link
Member

I believe it's a breaking change, we list it in #3287

Screenshot 2022-05-24 at 23 41 25

@flaviendelangle
Copy link
Member

Equivalent on the core: mui/material-ui#32546

@cherniavskii
Copy link
Member Author

I believe it's a breaking change, we list it in #3287

But if we don't update engines entry in package.json it's not a breaking change, right?

@oliviertassinari
Copy link
Member

oliviertassinari commented May 25, 2022

But if we don't update engines entry in package.json it's not a breaking change, right?

@cherniavskii Right, yes, I have missed this.

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label May 25, 2022
@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented May 26, 2022

A question, why not directly upgrade to v16 (Active LTS) to avoid upgrading it again next year?

v16
End of life: 30th April, 2024
Maintenance LTS starts soon on 18th Oct, 2022.

v14
End of life: 30th April, 2023

Here it suggests Production applications should only use Active LTS or Maintenance LTS releases.

@cherniavskii
Copy link
Member Author

why not directly upgrade to v16 (Active LTS) to avoid upgrading it again next year?

Good point, I'll upgrade to v16 👍

@oliviertassinari
Copy link
Member

oliviertassinari commented May 27, 2022

A question, why not directly upgrade to v16 (Active LTS) to avoid upgrading it again next year?

In MUI Core, we use (& have used) the lowest possible version of Node (like in the Browser tests) to catch cases where we might start depending on an API that is not supposed by the minimum "engines" value without realizing it.

@cherniavskii
Copy link
Member Author

to catch cases where we might start depending on an API that is not supposed by the minimum "engines" value without realizing it

To be fair, the opposite can happen as well, e.g. we use node v12 but in v16 some API that we use is missing.
But AFAIK we don't use node API directly, the only thing that matters is that we provide commonjs bundle, so that it works with SSR using node v12.

But if we keep using v12, we won't be able to update some of our dependencies like playwright or danger - see dependencies PRs that are currently on hold because of node version incompatibility: https://github.com/mui/mui-x/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+label%3Adependencies+label%3A%22On+hold%22

@oliviertassinari
Copy link
Member

oliviertassinari commented May 30, 2022

@cherniavskii +1 to not use Node v12 as it doesn't receive critical bug fixes. To be clear by "API", I was thinking ECMAScript: APIs like optional chaining, Intl, etc.

@cherniavskii
Copy link
Member Author

To be clear by "API", I was thinking ECMAScript: APIs like optional chaining, Intl, etc.

Thanks for the clarification!
We do transpile the code though (except for modern bundle). So unless there's a regression in our bundling method, it should work in v12 just fine

@oliviertassinari
Copy link
Member

We do transpile the code though

@cherniavskii It helps a lot but Babel can't transpile all the features of ECMAScript: https://babeljs.io/docs/en/caveats.

@cherniavskii
Copy link
Member Author

We've decided to set minimal supported version of Node to 14 in data grid v6 alpha

@oliviertassinari
Copy link
Member

MUI Core is moving forward with upgrading Node in the different CI environments from v12 to v14 to unlock dependency updates.

.browserslistrc Outdated
@@ -3,7 +3,7 @@ last 1 chrome version
last 1 edge version
last 1 firefox version
last 1 safari version
node 14
node 16
Copy link
Member

@oliviertassinari oliviertassinari Jul 25, 2022

Choose a reason for hiding this comment

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

this looks like a breaking change for developers

Suggested change
node 16
node 14

x the other changes in this file

@michaldudak
Copy link
Member

For reference, here's the equivalent PR in Core: mui/material-ui#33642

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Jul 26, 2022

We discussed about it in one of our meetings.

We decided to move to v14 too as part of v6 #5605

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 26, 2022

@joserodolfofreitas For the developers, the same goes for MUI Core: mui/material-ui#32546. The only difference is that v6 will be released later compared to MUI X, so Node.js v16 might be more realistic as the LTS version when the release happens.

For the contributors, the change is done mui/material-ui#33642, 👍 to do the same in MUI X.

@cherniavskii cherniavskii changed the title [core] Upgrade node to v16 [core] Upgrade node to v14 Aug 2, 2022
Copy link
Member

@oliviertassinari oliviertassinari 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

@oliviertassinari oliviertassinari removed the on hold There is a blocker, we need to wait label Aug 2, 2022
@cherniavskii cherniavskii merged commit be9053a into mui:master Aug 2, 2022
@cherniavskii cherniavskii deleted the update-node branch August 2, 2022 18:02
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants