Repository navigation
Enforce npm 11.10.0 minimum via devEngines and engine-strict - #830
Merged
Merged
Conversation
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0e46ccad0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The perf workflow installs Node from the base branch's .nvmrc, then switches to the PR head without re-running setup-node. When a PR bumps the Node/npm requirements (as this one does via devEngines), the reinstall step fails with EBADDEVENGINES because npm is still the base branch's version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
39 tasks done
inimaga
approved these changes
Aug 25, 2026
Contributor
pull Bot
pushed a commit
to erickirt/expensify
that referenced
this pull request
Sep 1, 2026
Validation PR for Expensify/react-native-onyx#830 — runs the full E/App suite against the Onyx PR head. Will be swapped to the published version once the Onyx PR merges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
This PR enforces minimum Node/npm toolchain versions as a prerequisite for adopting npm's
min-release-ageconfig (the upcoming supply-chain cooldown for newly published package versions). That config shipped in npm 11.10.0 — older npm versions silently ignore it, which would give a false sense of protection. The Node version pinned in.nvmrcuntil now (24.11.0) bundles npm 11.6.x, which is below that floor.Changes:
.nvmrc: 24.11.0 → 24.19.0 (same Node 24.x LTS line; bundles npm 11.17.0).package.json: adddevEngines.packageManagerrequiring npm>=11.10.0(wheremin-release-ageshipped) withonFail: "error".devEnginesis only evaluated for the root project, never when this package is installed as a dependency, so the tooling-only npm floor cannot break consumers. The publishedenginesfield is unchanged (node >=20.19.5,npm >=10.8.2)..npmrcwithengine-strict=trueso installs under a non-compliant toolchain fail fast instead of warning..gitignore: stop ignoring.npmrcso it can be tracked (publishing now uses OIDC trusted publishing via the sharednpmPublish.ymlworkflow, so CI no longer writes a token.npmrcinto the repo).package-lock.json: regenerated under Node 24.19.0 / npm 11.17.0 (no dependency changes).All GitHub workflows already read
node-version-file: .nvmrc, so they pick up 24.19.0 automatically.Related Issues
For https://github.com/Expensify/Expensify/issues/675045
Linked E/App PR
Expensify/App#99436
Automated Tests
No tests added — this PR only changes toolchain configuration (
.nvmrc,engines,.npmrc); no runtime code is touched. Enforcement itself was verified manually (see below).Manual Tests
npm ci— completes successfully.devEnginesnpm floor temporarily set to an unsatisfiable range,npm install --dry-rununder npm 11.17.0 fails fast with exit code 1, confirming enforcement fires:npm packand installed the resulting tarball into a scratch project withengine-strict=trueunder both npm 10.8.2 (Node 20.20.0) and npm 11.17.0 — both succeed with noEBADENGINE/EBADDEVENGINES, confirming the npm floor does not propagate to consumers. Also confirmed the packed tarball contains no.npmrc.Known gap: npm <10.9 predates
devEnginesand silently ignores it, but.nvmrcand all CI workflows pin Node 24.19.0 (npm 11.17.0), so contributors on the standard setup get the enforced floor.Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.