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

Supported node/npm versions #1

Closed
ChristophWurst opened this issue Mar 3, 2022 · 24 comments
Closed

Supported node/npm versions #1

ChristophWurst opened this issue Mar 3, 2022 · 24 comments
Labels
discussion Being discussed

Comments

@ChristophWurst
Copy link
Member

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.

nextcloud-libraries/nextcloud-axios#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
Copy link
Member

skjnldsv commented Mar 4, 2022

  • 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
Copy link
Member

skjnldsv commented Mar 4, 2022

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? 🤷

@ChristophWurst
Copy link
Member Author

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
Copy link
Member

skjnldsv commented Mar 4, 2022

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

💯 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 😕 .

@ChristophWurst
Copy link
Member Author

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. nextcloud-libraries/nextcloud-axios#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
Copy link
Member

skjnldsv commented Mar 4, 2022

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
Copy link
Member Author

ChristophWurst commented Mar 4, 2022

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
Copy link
Member

skjnldsv commented Mar 4, 2022

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
Copy link
Member Author

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

@ChristophWurst
Copy link
Member Author

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
Copy link
Member

skjnldsv commented Mar 4, 2022

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:

  • Do we actually have any benefit from supporting newer versions ?
  • Considering this is just an indicator of "we're only guarantying it work with xxx version", couldn't the engine be seen as a min-version? Like, "feel free to use node 17 and npm 8 if you want, but the app is tested against 14/7".

Now, if I would have to give a definitive answer, I'd say: similarly to browserconfig or php,

  • we document the official supported node version (we have to pick one at some point)
    • current recommended php is 8.0 in our documentation for example
    • supported browsers are also hardcoded
  • we explain that you can use above/newer versions if you like, but our testing infrastructure is aiming towards this version only. I don't think this is a bad thing as this is not enforcing anything. This is exactly why the engines are not strict. Let's see that as an indicator. When we'll all ready, let's comply to newer versions globally. This is a discussion we can all have when we all tested it. (We can even have automated test manually triggered where we try newer npm and node versions for all supported apps and know if it's time to globally upgrade the engines)

@nursoda
Copy link

nursoda commented Mar 5, 2022

@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
Copy link

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
Copy link

nursoda commented Mar 6, 2022

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
Copy link
Member

skjnldsv commented Mar 8, 2022

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
Copy link
Member

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
Copy link
Member

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?

@skjnldsv skjnldsv added the discussion Being discussed label Jun 10, 2022
@ChristophWurst
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

nextcloud/text#2519 (comment)

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

#5

@nextcloud nextcloud locked as resolved and limited conversation to collaborators Jun 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion Being discussed
Projects
None yet
Development

No branches or pull requests

5 participants