Page MenuHomePhabricator

Add "engines" to package.json
Closed, ResolvedPublic

Description

INFO:quibble.commands:node --version: v18.20.4
INFO:quibble.commands:npm --version: 10.7.0

Event Timeline

zeljkofilipin changed the task status from Open to In Progress.
zeljkofilipin claimed this task.
zeljkofilipin triaged this task as Low priority.
zeljkofilipin moved this task from Backlog 🪒 to Deep work 🌊 on the User-zeljkofilipin board.
zeljkofilipin updated the task description. (Show Details)

Change #1069155 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/core@master] selenium: Update node and npm versions in wdio-mediawiki

https://gerrit.wikimedia.org/r/1069155

Change #1069192 had a related patch set uploaded (by Zfilipin; author: Zfilipin):

[mediawiki/core@master] build: Add engines to package.json

https://gerrit.wikimedia.org/r/1069192

We should not enforce this as required verison. If anything, maybe as minium versions (which is different from what the above patch does).

But why would we want to hardcode such a precise version in each of hundreds of source repos? What does this solve?

If one of our dev dependencies has a minimum engine version, npm already enforces that.
What's in our code that warrants a separate or higher minimum Node version than the dev dependencies we use?

I read through T258562 but this only mentions:

NPM dependencies fail to install for some repositories (TODO: examples?) because they expect a specific Node version.

This states that some package (which?) in some repo (which?) fail to install on Node < 10 because a dependency requires Node 10+, and npm-install told you immediately upon trying. That's pretty good. What about this do we hope to improve, and what (else) would someone need to do to gain that benefit?

Node 10 is more than 6 years old now. Has this intervention only has proven valuable for that kind of very old system, or does it also change current code issues?

Change #1069192 abandoned by Zfilipin:

[mediawiki/core@master] build: Add engines to package.json

https://gerrit.wikimedia.org/r/1069192

We should not enforce this as required verison. If anything, maybe as minium versions (which is different from what the above patch does).

I'm fine with setting a minimum version. My thinking was that this precise version is used in our CI, so it's only really tested with this version.

But why would we want to hardcode such a precise version in each of hundreds of source repos?

The plan was to only put engines in core, not all repositories.

What does this solve?

The general problem is that our CI configuration is not stored in core but in a separate repository. Hence, it's far from trivial to know which node/npm version was used to run CI for which commit in core.

The specific problem is that I sometimes (admittedly rarely) need to debug problems introduced years ago. I was debugging such problem recently. The only idea I had on how to figure out which commit introduced the problem was to run git bisect on a very long range of commits in core. (Very long in this case was years.) git bisect was running a Selenium test to see if the checked our version was good or bad. Since the last good commit in core was several years old, I was not sure if the current version of node (v18) and/or npm (v10) would run the Selenium test several years old correctly.

It all worked just fine, but I could see it not working in a similar case.

If one of our dev dependencies has a minimum engine version, npm already enforces that.
What's in our code that warrants a separate or higher minimum Node version than the dev dependencies we use?

I thought it was a good idea to document in a machine-readable way which version of node and npm our CI uses at the time. I didn't think about dev dependencies and their requirements.

I read through T258562 but this only mentions:

NPM dependencies fail to install for some repositories (TODO: examples?) because they expect a specific Node version.

This states that some package (which?) in some repo (which?) fail to install on Node < 10 because a dependency requires Node 10+, and npm-install told you immediately upon trying. That's pretty good.

We created that task years ago because we were not able to install dependencies locally. I remember creating the task and planning to update it with more data but I never did. Now I don't remember the specific problem.

What about this do we hope to improve, and what (else) would someone need to do to gain that benefit?

The improvement is a machine-readable statement which version of node/npm that commit was tested with. Would that benefit anybody (besides my very specific need) it's hard to tell.

Node 10 is more than 6 years old now. Has this intervention only has proven valuable for that kind of very old system, or does it also change current code issues?

This is not node v10 specific. The original task was about a specific problem in node v10. I have closed that task and opened a new one because the old task was specific to node v10 problem (that we no longer have) and this one is a more general one.

@Jdforrester-WMF had a good point in 1069192:

Can you explain why? None of our toolchain pays any attention to this field, it just drifts and causes confusion until we remove it again.

The problem that I don't have the solution to is that our toolchain doesn't pay attention to the node and npm values in engines, causing the drift.

The only idea I had was to add engine-strict but that's a pretty nuclear option. My idea was to first test if there's any opposition to adding engines before considering how to solve the problem of it being up to date.

My idea was to first test if there's any opposition to adding engines before considering how to solve the problem of it being up to date.

I didn't think there would be much opposition to adding engines since it's present in various wikimedia repositories. How useful and correct node/npm version in engines are, I don't know.

Thanks @Krinkle and @Jdforrester-WMF for the feedback. Please let me know if you have further questions. I don't plan to continue working on this, so I'm resolving the task.

Change #1069155 merged by jenkins-bot:

[mediawiki/core@master] selenium: Remove `engines` from wdio-mediawiki

https://gerrit.wikimedia.org/r/1069155