[core] Require node@v18 for development

[dulmandakh]

First of all, thank you for MUI X.

This PR adds Node.js 18 requirement for mui-x developers, just to make it clear.

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Deploy preview: https://deploy-preview-10997--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against f04e3b6

[flaviendelangle]

Also I propose to support only React 18.

Why prevent people from using our libraries with React 17 if it's working fine?

Same for NodeJS, we are upgrading the minimal version when we are forced to do it.
Otherwise, we create friction with our community when there is no need for it.

[LukasTy]

Same for NodeJS, we are upgrading the minimal version when we are forced to do it.

As for the Node version, I do agree with the proposal of bumping the minimum version.
For what reason? Specifically to urge anyone still using older than node v18 to upgrade because any prior version is already outside of the maintenance window. 🙈
https://nodejs.org/en/about/previous-releases
The current active/LTS version is 20.

[flaviendelangle]

For what reason? Specifically to urge anyone still using older than node v18 to upgrade

I don't think that's our role.
If some of our users are blocked on Node 18 because they have a dependency that is not yet compatible with Node 20, then they will be blocked.

Btw the core is still using >= 12.0.0

[LukasTy]

Btw the core is still using >= 12.0.0

Maybe because their major is old? 🤔
From what I understood on mui/material-ui#37173 it seemed that they do plan to bump the minimum node version.
If we agree that we keep using node 14, then IMHO #9107 is incorrect. With the current setup, we are never sure if we are not unknowingly introducing any API that doesn't work on Node <=v17. 🙈

[dulmandakh]

With major version change, it seems to be better to bump Node.js requirements just to reserve possibility to bump dependencies later on. For example, eslint 9 will support only Node.js 18 and above, see https://eslint.org/blog/2023/11/whats-coming-in-eslint-9.0.0/. So we'll reserve possibility of using eslint 9 when released, otherwise we'll need to wait for another major version for eslint 9. There might be other dependencies too, in the future.

Also, CI runs Node.js 18 only so we'll never now if we broke something on older Node.js versions.

[dulmandakh]

Btw the core is still using >= 12.0.0

MUI need to bump it with next major version, because Node.js requirement is breaking change.

[flaviendelangle]

So we'll reserve possibility of using eslint 9 when released, otherwise we'll need to wait for another major version for eslint 9. There might be other dependencies too, in the future.

Here we are mixing two things: the Node version we say we are compatible with and the Node version we are using in our tooling.

[dulmandakh]

I expect that it'll take some time until stable release, and by then Node.js 18 or maybe 20 will be commonly used version.

[dulmandakh]

So we'll reserve possibility of using eslint 9 when released, otherwise we'll need to wait for another major version for eslint 9. There might be other dependencies too, in the future.

Here we are mixing two things: the Node version we say we are compatible with and the Node version we are using in our tooling.

it there a way to separate the two? or maybe we can require Node.js 18 on the top level, and 16 in packages?

[flaviendelangle]

it there a way to separate the two?

We are separating the two, I'm using 18 (should move to 20) to work with this repo.
Maybe there is a way to force a minimal version of Node (on the root package.json file?).

---

To stick with the modification done in this PR.
For me, if the core plan to always stick with the oldest supported LTS when they release a major, then I'm fine doing the same.
I expressed my main concern above, which is to have some users blocked between us and some other dependencies with no common Node.JS version.
If most of the big names in the space (ESLint, NextJS, Prettier, etc...) are following this rule, I guess the risk is fairly small to block users since they would already be blocked.
But I'd be very careful not to be too aggressive with this rule, I don't think our role is to push people to upgrade their Node version unless it prevents us from releasing something (this could occur with the codemod package for instance).

[dulmandakh]

To stick with the modification done in this PR. For me, if the core plan to always stick with the oldest supported LTS when they release a major, then I'm fine doing the same. I expressed my main concern above, which is to have some users blocked between us and some other dependencies with no common Node.JS version. If most of the big names in the space (ESLint, NextJS, Prettier, etc...) are following this rule, I guess the risk is fairly small to block users since they would already be blocked.

Next.js 14 supports 18.17 and above, see https://nextjs.org/blog/next-14#other-changes
Next 13 supports 14.6.0 and above, see https://nextjs.org/blog/next-13#breaking-changes
Also Next 13 bumped React version to 18.2.0

[flaviendelangle]

Also Next 13 bumped React version to 18.2.0

For React, they might have some reason to bump it since there is some very small BC between the two.
As far as I'm aware, we should be compatible with both 👍

[dulmandakh]

Also Next 13 bumped React version to 18.2.0

For React, they might have some reason to bump it since there is some very small BC between the two. As far as I'm aware, we should be compatible with both 👍

React 18 supports concurrent rendering and suspense 😉

[flaviendelangle]

React 18 supports concurrent rendering and suspense

Sure, but if you can support Concurrent Rendering and Suspense for your user that have React 18 and still let people use React 17 if they did not have the time to migrate (without the new features ofc), then you are just covering a broader part of the community.

[dulmandakh]

I'm adding Node.js 18 requirements on top level package.json or for developers. Leaving Node.js 14 as is for packages considering that there might be Next.js 13 users who wants to use MUI-X 7.x.

Thanks for suggestions 🙏

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[LukasTy]

@mui/code-infra What's your take on this one?
Would you see @mui/material having engines: { node: '>=18.0.0' } or something similar on the next major? 🤔

But I'm not sure if that would benefit anyone, because without a change in how we build/ship packages, nothing will change in regards to the delivered packages, at least based on my testing modifying the engines entry on the x-date-pickers package and building it.

[Janpot]

Would you see @mui/material having engines: { node: '>=18.0.0' } or something similar on the next major?

On the monorepo, yes. I think it's reasonable to expect from contributors to be on at least the current LTS release line of Node.js. The goal is to communicate both internally and to contributors which APIs we expect the runtime to support for our scripts. This avoids wasting time on debugging platform issues with the scripts. Oh, and it doesn't have to be on the next major if it only affects our tooling.
On the individual packages, no. They don't target Node.js (at the moment), so there is no reason to force a minimum version. When the zero-runtime comes out it might make sense to constrain the version. But probably still only on the zero-runtime package.

For me, if the core plan to always stick with the oldest supported LTS when they release a major, then I'm fine doing the same.

We should absolutely do it for both repos

[LukasTy]

We should absolutely do it for both repos

Thank you for the confirmation @Janpot. 🙏
Then we are going forward with the proposal in this PR. 👌

[LukasTy]

Thank you for your contribution and patience reacting to feedback @dulmandakh! 🙏
Could you please merge with latest upstream/next to resolve the conflicts? 🤔

[oliviertassinari]

Personally, I don't get the value of this change:

1. We support older versions of Node, how can we test the support if we merge this?
2. Next.js engine dependency has a clear error message, so what problem is there to solve?

[Janpot]

We may support older versions, but we have dependencies in our monorepo that require node >18. The engines resriction is already de facto in place. You can't install on node <18 already. Ironically we don't support newer versions very well, e.g. can't import MUI components.
The PR doesn't add much in practice, you couldn't test on node < 18 before, and neither after 🤷 . Nor does it remove anything. It just confirms the restriction that is already there. Most value I personally get out of it is that it tells me which node APIs we all decided are supported for our tooling.
To note: Some tools like render.com look at the engine field to set up an environment.

[LukasTy]

@oliviertassinari @Janpot Since https://github.com/mui/mui-x/pull/11366/files/ecaa7806ccbf2184bae4f898484da0b4ce6c655e#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47 the minimal version that the project can start on is 18.17.
Don't you think it makes sense to keep this limit synced for all the tooling?
Doing this might cause less confusion, when people wight expect the project to work on older node versions (and some things do work), but starting the docs would fail... 🤔 🤷
IMHO, I don't think that this explicit limit hurts in any way.
Do you see any potential problems with it?

[oliviertassinari]

Don't you think it makes sense to keep this limit synced for all the tooling?

@LukasTy Today we have v14 in MUI X as the minimum

mui-x/packages/x-tree-view/package.json

Line 66 in 57c6f9d

"node": ">=14.0.0"

and v12 in Material UI https://github.com/mui/material-ui/blob/bc212467e7ebc75bec71cb6cd951041ec1c01a22/packages/mui-joy/package.json#L90

I think:

1. we shouldn't touch the node version until we face a problem we can solve by increasing the minimum node version. Maybe ESM support will be one of this, but unclear to me. The less we touch it, the better. The goal is to have the adoption of the components as friction-less as possible for projects on older versions of node.js. If developers need to upgrade node before they can adopt a component, they might go with another.
   I wish we could get a real-life Node.js version stats but I couldn't find it, the closest I could find:

• Next.js: the most used version is v12: https://tools-public.mui.com/prod/pages/npmVersion. That Next.js version works down to node v12
• Webpack: there are 50% of the users on v4 which supports node down to v6
• Vite: most of the users are on v4 which supports node down to v14

2. we should bump the node version when it's pointless to support older version. For example, supporting node v10? This sounds absurd, very few bundlers in use today support it, it was released 4 years ago, so real state is likely super low, so no breaking changes pain for developers.
3. we should lower the minimum version of node down to v12 to match Material UI. I'm not aware of plans to bump it up in Material UI, consistency would help.
4. we should close this PR to be consistent with the oldest version of node we want to support. The docs is only one part of the project, we could have a folder to be able to test the logic with the oldest version of node.

[Janpot]

@oliviertassinari This PR is about setting the engines field of the monorepo. It shouldn't show up in any of the published packages. End-users shouldn't at all affected by this . The only people that are affected by this are contributors trying to run our Tooling, and they won't be able to run that on <18 because our Tooling doesn't run on these node.js versions.

100% agree that our end-users shouldn't get this restriction, but that's not what this PR does.

Not gonna die on this hill though 🙂

[LukasTy]

@oliviertassinari But we are not touching node versions of the packages we expose, it's only the root package that we are discussing the version change of.
The only effect it would have is to stop developers with older node versions than 18 from even installing the project.
If we want to be completely correct, we could specify the engines field on docs/package.json. WDYT?
FYI, I just tried installing the project with node@16 and got this:

I propose to set engines to node: ">=18.17" on the docs package, which would produce the following when starting docs with an unsupported version:

[oliviertassinari]

This PR is about setting the engines field of the monorepo. It won't show up in any of the published packages. End-users are not at all affected by this.

@Janpot Agree, I covered this in point 4. I thought we first needed to have clarity on what's the expected experience we want end-users, to figure from there, how we can design the experience for the people who build that experience.

If the root repository minimum node version is set to v18, then how are we supposed to work on v12?

tried installing the project with node@16 and got this:

@LukasTy It seems that pnpm behaves the opposite of npm and yarn, it only warns about engine mismatches. Edit, ah no, my bad: https://pnpm.io/package_json#engines

I propose to set engines to node: ">=18.17" on the docs package, which would produce the following when starting docs with an unsupported version:

This makes more sense to me, but how is the error Next.js gives not clear enough? Why have to maintain such an engine, to keep it up to date?
Because of https://pnpm.io/package_json#engines, it sounds like the value is to have an install error vs. an install warning, so people who want to test other parts of the package. But then, it doesn't seem that pnpm have ignore-engine like npm or yarn, so we are stuck if we want to test older engines.

Not gonna die on this hill though

#plea for the npm package. #suggestion for the dev package, I think the bulk of my thought is raised just ⬆️.

I'm unsubscribing, I think I contributed enough 😄

[LukasTy]

@dulmandakh Thank you for your contribution, but we are closing this PR. 🙇
We have steered very far from the original proposal and the extra strictness does not seem to have tangible benefit.
As discussed, our docs can not run on anything <18.17, but we might need extra freedom to test the library with older engine versions. In such a case we do not want to fight the extra steps of removing the config. 👌

Feel free to contribute in the future and thank you for your patience! 🙏

[michaldudak]

we could have a folder to be able to test the logic with the oldest version of node.

This will become increasingly difficult as testing tools drop support for older Node versions as well (for example https://github.com/chaijs/chai/releases/tag/v5.0.0).

Why do want to ensure our libraries run on unmaintained versions of Node anyway?

[romgrk]

I also agree that the use-case doesn't make sense to me. If we were already testing those older nodejs versions, I would understand that we keep them working. But we're not, iiuc? And it should also be noted that v18 is already in maintenance mode, the current LTS is v20. Even older versions should definitely not be used.

[LukasTy]

FWIW, I'm also on the same camp—I'm not sure what value we bring by releasing the latest version of the library and still shipping it with ancient API and declaring support down to Node 12 when we actually do not even test it, even our dev @types/node is at v18, not v12, so we might have a bunch of cases where we use API that is not supported on the previous version just because we are not testing it. 🤷

I do understand this position for @mui/[email protected], but given all the other industry players actions, I'd also just force users to move to at least Node 18 and call it a day. 🙈