Skip to content

fix(package): require the oldest supported LTS (node >= 22) - #280

Merged
allanbowe merged 3 commits into
mainfrom
fix/node-engines-lts
Oct 5, 2026
Merged

allanbowe merged 3 commits into
mainfrom
fix/node-engines-lts

Conversation

@sasjs-dev

@sasjs-dev sasjs-dev Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

What

The declared node floor permits end-of-life releases. From the nodejs/Release schedule:

14  EOL 2023-04-30
16  EOL 2023-09-11
18  EOL 2025-04-30
20  EOL 2026-04-30
22  supported until 2027-04-30  <- oldest supported LTS

engines.node is now >=22, matching the oldest line still receiving security fixes and the version CI runs on.

Verification

  • the only package.json change is the engines.node value
  • CI runs on node 22

The declared node floor permits end-of-life releases - node 14 and 16 reached
EOL in 2023, 18 on 2025-04-30, and 20 on 2026-04-30. The oldest line still
receiving security fixes is 22, which is also the version CI runs.

Raise engines.node to >=22 so the declaration matches the runtimes this
package is tested and supported on.

Verification: the only package.json change is the engines.node value, and CI
runs on node 22.

@sasjs-dev sasjs-dev Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewed the full base...main diff (package.json only: engines.node raised from ">=10" to ">=22") plus hardening checks at the head: lockfile present, secrets scan, npm audit against the head lockfile, and repo-level config. This repo lags the org hardening baseline in several places that the engines bump makes worse, reported below. CI (build (lts/fermium)) is green on the head - but that check name is itself finding 1. Findings:

  • package.json:10-11 - engines ">=22" contradicts both CI workflows: .github/workflows/build.yml:16 and npmpublish.yml:17 still run "node-version: [lts/fermium]" (node 14), and package-lock.json:53-55 still records engines ">=10" (not regenerated). The declared floor of 22 is tested nowhere: CI runs on 14, the lockfile says 10, the manifest says 22. In fact node 14 cannot even install this tree under the new engines (npm warns; engine-strict consumers are hard-blocked), and the release workflow would fail its own package's floor. Update the matrices to 22 (or 22 plus newest LTS) and regenerate the lockfile root.

  • package.json:67 - "node-sass": "7.0.3" does not support node 22: node-sass 7.0.3's own support table tops out at Node 17 (ABI 102; verified in the published package's extensions.js), and its install script tries to fetch a binding for the runtime ABI, falling back to a source build that fails on modern toolchains. node-sass 9.0.0 is the first release supporting newer ABIs (its table covers up to Node 20; engines >=16). Since react-scripts 5 + sass-loader 12 prefer the pure-JS "sass" package when resolvable (verified in sass-loader's getDefaultSassImplementation: sass first, then node-sass), and "sass": "^1.52.2" is already a devDependency, the cleanest fix consistent with the engines bump is to drop node-sass entirely (react-scripts will use sass), or pin node-sass to 9.x if the native path must stay. As-is, npm ci on node 22 will attempt a compile and most likely fail.

  • Repo hardening baseline (present in the org's other repos, missing here entirely): no .npmrc (no ignore-scripts=true / save-exact=true), no .pre-commit-config.yaml (the standing requirement is a gitleaks hook pinned to an exact release tag), no gitleaks or npm audit step in CI, no dependency pinning (all ranges, ^x.y.z throughout), and devDependency "node-sass" plus the ^-ranges on react/react-dom peer pairs (react ^17 dev vs peer ^16) are drift-prone. The engines bump touches none of these, so they are pre-existing - but this PR is the natural moment to note the repo has no secret-scanning or audit gate at all. Advisory, listed for the maintainer's prioritisation rather than as a demand attached to a one-line engines change.

  • Commit subject "fix(package): require the oldest supported LTS (node >= 22)" is conventional and release-driving; the repo releases via the semantic-release action in npmpublish.yml. No finding on the commit.

4 findings above for review.

sasjs-dev Bot added 2 commits October 5, 2026 11:56
Two follow-ups of the same shape as the other engine bumps:

- package-lock.json still recorded "engines": {"node": ">=10"} at the root
  while the manifest said >=22. npm ci does not rewrite it, so regenerate it.
- CI ran [lts/fermium], which is node 14, so nothing tested the declared floor.
  build.yml is now [22, lts/*]. npmpublish.yml is [22] rather than a two-entry
  matrix on purpose: that job releases, so a second entry would run
  semantic-release twice against the same commit.
node-sass 7.0.3 supports node up to 17 (ABI 102), so with the floor now >=22
its install script would try a source build on node 22 and most likely fail -
which the new [22, lts/*] CI matrix would hit on its first run.

Nothing here needs it. The components import .scss and microbundle-crl compiles
them; `sass` is already a devDependency for the react-scripts path, and
sass-loader prefers `sass` whenever it resolves.

Verified with node-sass removed:
- npm ci with scripts enabled: exit 0, 1822 packages (was 1958)
- npm run build: exit 0, identical bundle sizes
@sasjs-dev

sasjs-dev Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Findings 1 and 2 fixed. Finding 3 acknowledged and deliberately not folded in.

  • matrix + lockfile (92d21ee): the matrices ran lts/fermium (node 14) and the lockfile root said >=10, while the manifest said >=22 - so the floor was tested nowhere. build.yml is now [22, lts/*]; npmpublish.yml is [22] alone on purpose, because that job releases and a second matrix entry would run semantic-release twice against the same commit. Lockfile regenerated.
  • node-sass (9f4c4e3): correct, and it would have broken the new matrix on its first run. Verified it is unused: the components import .scss, microbundle-crl compiles them, and sass is already a devDependency for the react-scripts path. Dropped it - npm ci with scripts enabled now exits 0 (1822 packages, was 1958) and the build produces identical bundles. I did not take the 9.x pin, because dropping is the fix rather than a workaround.
  • hardening baseline: agreed on every point, and I have deliberately not folded it in. No .npmrc, no gitleaks hook, no audit step, and ^ ranges throughout is a repo-wide change, not a consequence of the engines bump. It deserves its own PR rather than riding on a one-line engines change. Say the word and I will open it.

@sasjs-dev sasjs-dev Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-review at the head 9f4c4e3; the delta since the prior review at 05a262e is exactly the two fixes the prior review asked for: the CI matrices now run [22, lts/] (build.yml:18, npmpublish.yml:19), the lockfile engines root is synced (package-lock.json ">=22"), and node-sass is dropped from devDependencies (with the lockfile regenerated - no node_modules/node-sass entry remains; only the optional peerDependenciesMeta reference inside sass-loader's entry at package-lock.json:18391-18402, which is inert). CI (build (22), build (lts/)) is green on the head, so node 22 actually installs and builds the tree - the node-sass concern is resolved by observation. The remaining items from the prior review are the repo-level hardening baseline, which this PR does not touch and which remains open for the maintainer's prioritisation:

  • .npmrc (missing) - no ignore-scripts=true / save-exact=true repo-wide, so local installs run dependency install scripts and manual edits reintroduce ranges.

  • .pre-commit-config.yaml (missing) - the standing requirement is a gitleaks hook pinned to an exact release tag; the repo has no secret-scanning hook at all.

  • CI audit gate (missing) - no npm audit step in build.yml or npmpublish.yml, and no dependency pinning (all ^ ranges throughout, e.g. "sass": "^1.52.2").

Advisory: these predate this PR, but it is the natural moment to adopt them - the org's sibling repos (adapter, cli, lint, utils) all carry the .npmrc, the gitleaks pre-commit config and a strict audit step.

3 findings above for review.

@sasjs-dev

sasjs-dev Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks - agreed on all three, and agreed that they predate this PR.

I have not folded them in here. Adopting .npmrc with save-exact=true also means pinning the ^ ranges across the manifest, which is a repo-wide dependency change; the gitleaks hook and a strict audit step are a change of their own. Bundling all three into an engines-bump PR would make both harder to review.

They are worth doing - this repo is the outlier in the org on all three - and I will put them up as a separate change rather than leave them as a note on this one.

@allanbowe
allanbowe merged commit 8ff67a6 into main Oct 5, 2026
4 checks passed
@allanbowe
allanbowe deleted the fix/node-engines-lts branch October 5, 2026 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant