Skip to content

[code-infra] Make engine version requirement strict #552

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 25, 2025
Merged

[code-infra] Make engine version requirement strict #552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e811b6
[infra] Make engine version requirement strict
brijeshb42 Aug 18, 2025
70e97f3
Convert to esm
brijeshb42 Aug 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ parameters:
default-job: &default-job
working_directory: /tmp/mui
docker:
- image: cimg/node:24.3
- image: cimg/node:22.18
environment:
DANGER_DISABLE_TRANSPILATION: 'true'

Expand Down
12 changes: 10 additions & 2 deletions .github/workflows/ci-base.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ name: PR Releases
on:
workflow_call:
inputs:
# For backwards compatibility
node-version:
description: 'Node.js version to use'
required: true
required: false
type: string
default: ''
use-compact-url:
description: 'Use compact URLs for pkg.pr.new'
required: false
Expand All @@ -33,10 +35,16 @@ jobs:
fetch-depth: 0
- name: Set up pnpm
uses: pnpm/action-setup@a7487c7e89a18df4991f7f222e4898a00d66ddda # v4.1.0
- name: Use Node.js ${{ inputs.node-version }}
- name: Use specific Node.js version
uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
if: ${{ inputs.node-version != '' }}
with:
node-version: ${{ inputs.node-version }}
- name: Use Node.js version from repo
uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
if: ${{ inputs.node-version == '' }}
with:
node-version-file: 'package.json'
# https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#caching-packages-dependencies
cache: 'pnpm'
- run: pnpm install
Expand Down
2 changes: 0 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,3 @@ jobs:
continuous-releases:
name: CI Releases
uses: ./.github/workflows/ci-base.yml
with:
node-version: 22
2 changes: 1 addition & 1 deletion apps/code-infra-dashboard/netlify.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
targetPort = 3000

[build.environment]
NODE_VERSION = "20"
NODE_VERSION = "22.18"

[functions]
directory = "functions"
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,6 @@
"packageManager": "[email protected]",
"engines": {
"pnpm": "10.15.0",
"node": ">=18.0.0"
"node": ">=22.18"
}
}
1 change: 1 addition & 0 deletions pnpm-workspace.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ overrides:
# and to eslint for another instance, causing a conflict.
# This should not be an issue for end users, but it is a problem for the monorepo.
'@types/eslint': 'npm:eslint@^9.29.0'
engineStrict: true
Copy link
Member

@oliviertassinari oliviertassinari Aug 18, 2025

Choose a reason for hiding this comment

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

Do we want this? Per https://pnpm.io/settings#enginestrict, it feels like the more we can get away with ignoring engine version, the better. Either because we don't want to upgrade Node.js to stay as low as possible, or because we want to upgrade Node.js but the dependency says it's not compatible with more recent versions.

Suggested change
engineStrict: true

Copy link
Member

@Janpot Janpot Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe we want this, yes. For our dev env we don't want to stay as low as possible. We want what allows us to write modern, secure, performant code and great contributor DX. We don't want to waste our and our contributor's time on problems caused by too low node.js version, nor do we want to waste our time on building polyfills and compatibility modes for outdated node.js. Best way to prevent us from having to chase these errors this is to just block altogether during the installation process.

I feel like we keep circling back to this. Are you sure you're not conflating end-user node.js version with contributor node.js version? Again, 100% agree that for our React libs we can keep it as low as possible. Personally, I wouldn't even explicitly set it. But these node versions are untouched here.

it feels like the more we can get away with ignoring engine version, the better.

But you can't just ignore the engines field, that would mean we would need to write our tooling with maximum backwards compatibility. On top of the fact that none of our dependencies are following this philosophy, it would be a total waste of time, there is simply no demand for this. If a contributor wants to be able to ignore engine version, they should just use a node version manager.

Copy link
Member

@oliviertassinari oliviertassinari Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We want what allows us to write modern, secure, performant code and great contributor DX

I got the impression that engineStrict goes against this objective for the infra packages. It doesn't make it possible to upgrade Node.js to v22 if a package says it only support Node.js up to v20.

Copy link
Member

@Janpot Janpot Aug 19, 2025

Choose a reason for hiding this comment

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

It doesn't make it possible to upgrade Node.js to v22 if a package says it only support Node.js up to v20.

isn't that a good thing? Why would we upgrade to node 22 if not all of our dependencies can run under it? If a dependency blocks us from upgrading to node.js maintenance LTS, it's a clear sign this package is going to be a liability for us.

Copy link
Member

@oliviertassinari oliviertassinari Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If a dependency blocks us from upgrading to node.js maintenance LTS, it's a clear sign this package is going to be a liability for us.

Yeah maybe, I thought that if the test in the CI pass, then we can ignore what the engine field says. I have grown to not trust the compatibility version library say they have. But either way seems ok.

Copy link
Member

@Janpot Janpot Aug 19, 2025

Choose a reason for hiding this comment

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

Yep, in practice many libraries just work outside of their officially supported node.js version range. For me, when we're blocked from upgrading it tends to mean one of two things:

  1. our dependency is hopelessly outdated
  2. the package is unmaintained

which ideally is solved by upgrading or replacing it. Ofcourse we should always be pragmatic, it's not always immediately possible, I get that, but we can relax the engineStrict again any time. As long as we're not unreasonably blocked by it, why not just keep it enabled?

2 changes: 1 addition & 1 deletion update-netlify-ignore.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const packageName = process.argv[2];

if (!packageName) {
console.error(
'Error: Package name is required. Usage: node update-netlify-ignore.js <package-name>',
'Error: Package name is required. Usage: node update-netlify-ignore.mjs <package-name>',
);
process.exit(1);
}
Expand Down
Loading