nextcloud / standards

1 stars 0 forks source link

Supported node/npm versions #1

Closed ChristophWurst closed 2 years ago

ChristophWurst commented 2 years ago

Hello,

Let's get this repo started.

Something I wanted to have discussed is our current de facto "standard" that is spreading in the ecosystem where we lock down node/npm versions to one specific major version each. I've seen people fall into the trap of using the node/npm that comes with their setup. Sometimes that is a bit older, sometimes it's the latest LTS version, other times it's bleeding edge. Npm warns you when you use an incompatible engine, and so if people clone one of our repos and install npm dependencies, they are greeted with a flood of warnings. Even worse, some repos don't even build with the latest versions of node.

https://github.com/nextcloud/nextcloud-axios/issues/424 is an example.

Some questions off the top off my head

1) Who gets to decide what versions of Node/npm "we" support? 2) Are specific node/npm versions a recommendation or requirement? 3) Does it even make sense to define this globally? We are also not locking down php versions. Everyone is free to run anything from PHP7.4 to PHP8.1 mostly. Some repos have to restrict this based on their special needs. Calendar still support Nextcloud 21+, so we still allow PHP7.3. Talk's main branch only targets the upcoming server release, so it is PHP7.4. 4) Why can't we allow wider ranges for repos where it doesn't cause any known problems? If server only works with Node >=14<=16 then I'm fine with that. But if the axios lib can work with 18 as well then I don't see why it needs to be disabled. The restriction would be artificial.

skjnldsv commented 2 years ago
  • Who gets to decide what versions of Node/npm "we" support?

I'd say decision should be taken by impacted or maintenance people. Should be taken globally for the whole Nextcloud. If an app have issues, then we should help it upgrade to the same as others.

  • Are specific node/npm versions a recommendation or requirement?

This is a warning telling "We can only certify this works on this version". You can still use above versions, but we are not testing against all of the above ones nor potential upcoming releases https://docs.npmjs.com/cli/v8/configuring-npm/package-json#engines

Unless the user has set the engine-strict config flag, this field is advisory only and will only produce warnings when your package is installed as a dependency.


  • Does it even make sense to define this globally? We are also not locking down php versions. Everyone is free to run anything from PHP7.4 to PHP8.1 mostly. Some repos have to restrict this based on their special needs. Calendar still support Nextcloud 21+, so we still allow PHP7.3. Talk's main branch only targets the upcoming server release, so it is PHP7.4.
  • Why can't we allow wider ranges for repos where it doesn't cause any known problems? If server only works with Node >=14<=16 then I'm fine with that. But if the axios lib can work with 18 as well then I don't see why it needs to be disabled. The restriction would be artificial.

Aren't you annoyed to switch from npm 7 to npm 6 every time you want to work on mail? This is the real issue imho. It's far easier to have a common ecosystem. You mentioned php. If some apps say php 7.4 only, another says 8.1 only. This create quite an issue to properly work on Nextcloud apps. Most of us switch many repos per day.

skjnldsv commented 2 years ago

Quoting myself on another issue:

Yes, we actually went from the >= operator to ^. We cannot test all environments and node evolves quite quickly. We (devs) often change from apps to apps at nextcloud, and it's safer/easier to properly target a version and ensure compatibility with it, rather than switching from one to the other for each repo where some might be compatible or not (we already have a few repo stuck on npm 6)

To add on top of that, we are not using dependencies/devDependencies with a >= operator for obvious reasons. Every time a new dependency is released, we cannot assume it will be compatible. Actually from experience, I've seen more major breaking than one magically passing.

If we would support multiple versions, this would have to bedefined with ^, so something like this

    "engines": {
        "node": "^14.0.0 || ^15.0.0 || ^16.0.0 || ^17.0.0",
        "npm": "^7.0.0 || ^8.0.0"
    }

Which make the maintenance even more complicated. I'm usually the one that end up doing all of this because no one want/will/know how to. Who is gonna test for all those new versions? :shrug:

ChristophWurst commented 2 years ago

Aren't you annoyed to switch from npm 7 to npm 6 every time you want to work on mail? This is the real issue imho. It's far easier to have a common ecosystem. You mentioned php. If some apps say php 7.4 only, another says 8.1 only. This create quite an issue to properly work on Nextcloud apps. Most of us switch many repos per day.

That is exactly my point. I'm not arguing that some apps should bump their minimum version and restrict the node/npm versions, rather to widen the range so Mail can work with node 14 and all the way up to 18. If you stay on 14 and jump around repos it wouldn't have any disadvantage for you if an app supports more.

skjnldsv commented 2 years ago

If you stay on 14 and jump around repos it wouldn't have any disadvantage for you if an app supports more.

:100: agree. Then we still need to have an official min-supported version for the Nextcloud ecosystem. And having only one engine version defined is the easiest way to do this I think. Maybe there are other way to do that, but I am not aware of any :confused: .

ChristophWurst commented 2 years ago

Which make the maintenance even more complicated. I'm usually the one that end up doing all of this because no one want/will/know how to. Who is gonna test for all those new versions? shrug

Is this the reasoning for

need to have an official min-supported version for the Nextcloud ecosystem

?

This seems like a topic of its own and a problem we need to solve.

To me it also feels like there are attempts to synchronize everything across all apps. So it comes natural that individuals don't even dare going their own paths. People are told that they should use the workflow templates. The templates make assumptions and any adaptations to the templates are overwritten by 1:1 copies of new workflows. So what should have been a template in the original meaning of the word has become a copy. https://github.com/nextcloud/nextcloud-axios/pull/380 shows this. In many cases none of the decisions are documented either, so it's very hard to follow why things are done a certain way.

In my view of the ecosystem I rather have people update platform dependencies of their software individually. It's a bit more work but then developers also understand better what they are doing. As soon as someone else takes care of this aspect we start to fully pass the responsibility. And I guess that is how you became the owner of anything javascript. If one has to define a workflow that works for "all" apps, then obviously you're better off restricting what apps are allowed to do.

We managed quite well with php versions. Sure, it sometimes takes a bit of time until the latest major/minor is supported. But then we also have to fix some code. But overall we found a practical way to work across repos without ever pinning php versions.

My comment might have gone a bit off topic but I wanted to elaborate why I'm feeling uncomfortable in the lockstep way of keeping all apps the same.

skjnldsv commented 2 years ago

We managed quite well with php versions. Sure, it sometimes takes a bit of time until the latest major/minor is supported. But then we also have to fix some code. But overall we found a practical way to work across repos without ever pinning php versions.

But we do pin php versions. Apps have to comply with server's supported php versions. Currently php 7.4 and 8.0. If an app want to support 8.1, it cannot.

ChristophWurst commented 2 years ago

With pin I mean lock down to one version. You can use PHP7.4 to PHP8.0 with most components. Some even go down to 7.3 and up to 8.1.

If we did the to PHP versions what we did to node, then everyone would have to run PHP7.4. We would not allow PHP8.0 or anything else.

skjnldsv commented 2 years ago

I mean, I'm up to support above versions, but we also pin them then.

If we would support multiple versions, this would have to bedefined with ^, so something like this

  "engines": {
      "node": "^14.0.0 || ^15.0.0 || ^16.0.0 || ^17.0.0",
      "npm": "^7.0.0 || ^8.0.0"
  }

We don't do crazy unknown selectors like >= ? In the end, I am ok with whatever version we want to support, but if we claim to support some versions, we test against them then. :)

ChristophWurst commented 2 years ago

That is now what I said. We do not have open ended support ranges, luckily.

ChristophWurst commented 2 years ago

Would could be candidates for the documentation of the recommended node/npm version(s)? Is that rather tied to a server release or time?

skjnldsv commented 2 years ago

I think since we compile, it's only for development purpose luckily. So we're not bound to server. It's really only about devs environments :)

One concern I have expanding this is also about CI. It's already saturated I am afraid we will just end up offering versions that are not tested and working. Another issue is node14 will produce different builds than node 16. Meaning for repos that ships compiled bundles, this can/will cause mismatches.

And lastly, php is doing nice because complying with server's requirements will ensure all the app you clone are working with either 7.4 or 8.0. If we copy the same method with node, it still means we need a single point of truth. So we go back to restricting devs?

Now I have a few questions:

nursoda commented 2 years ago

@ChristophWurst Thanks for extracting a separate, generic discussion/decision thread from the issues I raised. And thanks to you @skjnldsv for the important thoughts. I propose to include more Nextcloud core members here and I kindly plead to take this to some decision, not thoughts "only".

As a very inexperienced dev, I struggled many hours up to the point where I wanted to give up. (I'm quite reluctant to do so, and this is not only due to the Nextcloud apps infrastructure packages but also about vue migration and node/npm weirdness in itself.)

The main issue I rose (in the now closed issues, without a solution!) was that the Nextcloud app infrastructure depencencies do cause a deadlock situation today since the required node version does not provide the required npm version – in none of the pre-packaged node versions. So today, every NC app dev needs to manually tweak binaries within nvm directories if she/he uses nvm. That's insane. It also isn't easy to get a specific npm version (e.g. for Arch), at least I don't know how.

While I do understand the issues with requirements to different development environments, I strongly advocate to at least support the latest node LTS version and it's packaged-with npm. I'd even ask to make sure to support the next to-be-LTS branch at least some weeks before it actually becomes the next LTS.

We should not include the PHP discussion here. I have a similar attitude towards that but this issue is about node/npm.

max-nextcloud commented 2 years ago

The main issue I rose (in the now closed issues, without a solution!) was that the Nextcloud app infrastructure depencencies do cause a deadlock situation today since the required node version does not provide the required npm version – in none of the pre-packaged node versions. So today, every NC app dev needs to manually tweak binaries within nvm directories if she/he uses nvm. That's insane. It also isn't easy to get a specific npm version (e.g. for Arch), at least I don't know how.

I'm not using nvm so i am not familiar with the problem. My approach would be

nvm install 14.19.0
nvm use 14.19.0
npm install -g npm@7

Doesn't that do the trick?

nursoda commented 2 years ago

Thanks for the tip npm install -g npm@7 – probably solves my 'deadlock'. I'm unexperienced, and new to node. Currently I won't fiddle with my setup, but I will try next time I come across it. We should limit support/forum style questions/answers here.

The issue itself remains. I think it is somewhat illogical to request versions that need to be manually adjusted in such way. Either require a certain node version and allow for all npm versions that come with that node version (preferred way since forward dependency), or require a certain npm version, allowing for all node versions that provide it (more of a hassle since that would require to find out first what the latest (stable) version of node is that sports a certain npm version). If one needs to restrict both, then it should only a combination that may be provided "out of the box". In that case, one should define upper and lower bounds for both.

However my personal approach is that all packages should be adopted in a way that they may be built with the latest (release/LTS) version at least. I consider the delay of such adaption a technical debt.

skjnldsv commented 2 years ago

For me there are multiple issues:

  1. Technical debt of being behind
  2. Having only one version in engines

While I agree than 1 should be improved, I think 2 is not an issue as you can still try and build with newer versions and the warning is not blocking.

vinicius73 commented 2 years ago

Aren't you annoyed to switch from npm 7 to npm 6 every time you want to work on mail? @skjnldsv

It is easily fixable using corepack

Also, by using yarn (my personal preference) we can ensure the version of yarn inside the repository, and never have problems with it.

From my point of view, there is no hard reason to restrict any of or packages or apps with node version usage. Or projects aren't using nodejs in production.

We only must keep supporting LTS versions, or the last LTS version. JavaScript ecosystem moves quickly and sometimes can be dangerous. Because it is important to keep the dependencies updated and tested.

skjnldsv commented 2 years ago

We only must keep supporting LTS versions, or the last LTS version.

I concur. That's why I think it's good to have the warning if you use above versions but still test on the min-supported. Which is what the ^7 do :)

We just have to make sure to upgrade the package.json when the last supported lts changes (14 is dropped in 2023) Shall we close this ticket?

ChristophWurst commented 2 years ago

Does that now mean we will be allowed to use newer node/npm than LTS or is there still an upper bound? Can Mail, for example, work with node 14-18 or must the package.json require 14 exclusively?

skjnldsv commented 2 years ago

It means you have to support the last LTS as the whole Nextcloud ecosystem does. But as a dev you can use more recent versions and it won't cause an issue.

vinicius73 commented 2 years ago

https://github.com/nextcloud/text/pull/2519#issuecomment-1164495163

Related problem.

What we can do about it? Can we move forward to Node 16 and NPM 8?

Node NPM
14 6
16 8
18 8

Ref.: https://nodejs.org/en/download/releases/

vinicius73 commented 2 years ago

My preference is to use corepack to handle it. Also use yarn to have strict control of the package manager in each project.

It will solve direct and indirect problems.

skjnldsv commented 2 years ago

Npm is deep rooted into Nextcloud now and not at discussion is here I would say @vinicius73, sorry

Can we move forward to Node 16 and NPM 8?

Node 14 is still maintained as last LTS for 10 more months endoflife.date/nodejs

I would be also ok to stop using security-only LTS and move to active one only, meaning we move to 16 and then 18 in 3 months and 3 weeks (18 Oct 2022)

skjnldsv commented 2 years ago

My preference is to use corepack to handle it.

Corepack is an experimental tool, I'm not in favour of using this at all

skjnldsv commented 2 years ago

https://github.com/nextcloud/standards/issues/5