feat!: bump engines requirement to Node 22
#222
Conversation
BREAKING CHANGE: bumps required Node.js version to >=22.0.0
erickzhao
changed the title
engines requirement to Node 22engines requirement to Node 22
src/helpers.ts
Outdated
| throw err; | ||
| } | ||
| d('work succeeded'); | ||
| await fs.remove(dir); | ||
| await fs.rm(dir, { recursive: true }); |
There was a problem hiding this comment.
Just checking my assumptions here, but we had to add the recursive option here due to a behavior change when we moved from fs-extra to Node's fs?
There was a problem hiding this comment.
Yup. It's not very explicitly documented but fs-extra's remove is recursive while Node's fs is not by default.
If we look at the latest fs-extra code for remove we can see it's just calling through to fs.rm(path, { recursive: true, force: true }) (it's technically using graceful-fs, but looks like that library just passes through fs.rm as-is - yay layers of abstraction).
But this is a good callout, because I think to ensure there's no regression in behavior here we need to add the force: true option. Adding suggestions to do so.
There was a problem hiding this comment.
Keeping force: true makes things easier for migration purposes, but might just be masking an erroneous call if some folder never exists?
For example, I was running into a case in @electron/get where we were always attempting to delete a folder path that didn't exist.
There was a problem hiding this comment.
Fair point. I think for migration purposes we should strive for no intentional behavior change, and then we can follow-up with that change after?
There was a problem hiding this comment.
Yep, I think removing force calls after the fact could be a good-first-issue since the acceptance criteria is pretty clear!
src/helpers.ts
Outdated
| throw err; | ||
| } | ||
| d('work succeeded'); | ||
| await fs.remove(dir); | ||
| await fs.rm(dir, { recursive: true }); |
There was a problem hiding this comment.
Yup. It's not very explicitly documented but fs-extra's remove is recursive while Node's fs is not by default.
If we look at the latest fs-extra code for remove we can see it's just calling through to fs.rm(path, { recursive: true, force: true }) (it's technically using graceful-fs, but looks like that library just passes through fs.rm as-is - yay layers of abstraction).
But this is a good callout, because I think to ensure there's no regression in behavior here we need to add the force: true option. Adding suggestions to do so.
.github/workflows/test.yml
Outdated
| - '18.17' | ||
| - '16.20' | ||
| - '14.16' | ||
| - '22.13' |
There was a problem hiding this comment.
Something I've thought about in the past (and I feel like @BlackHole1 has pointed this out before), is we should probably explicitly test our lower bound Node.js version to ensure we remain compatible with it. It's extra CI runs, but since we just reduced the matrix here we'll still have a net reduction if we add back '20.0.0' to the matrix.
There was a problem hiding this comment.
Do you mean the lower bound being 22.0.0 (minimum engines value)?
@erickzhao, could you also change the commit message just to be sure? Some of these repos aren't configured to default to the PR title and we've accidentally merged in the past where it grabs the first commit message instead of the title. |
.nvmrc
Outdated
| @@ -0,0 +1 @@ | |||
| 22.13.0 | |||
There was a problem hiding this comment.
nit:
| 22.13.0 | |
| 22.13.0 | |
From the nvm readme:
The contents of a
.nvmrcfile must contain precisely one<version>(as described bynvm --help) followed by a newline.
nvm install works fine for me without the newline, but we do have it in all other project files anyway 🤷♂️
Also a question: shouldn't this version match the one in the engines field? We're saying we support 22.0.0 as the minimum version, but nvm users working on this project will be using 22.13.0 so they could end up adding code that doesn't work on 22.0.0 and not noticing it.
I think this is a similar point to @dsanders11's on test.yml about testing the lowest supported version - we should pick one of 22.0.0 / 22.13.0 and use it everywhere.
There was a problem hiding this comment.
I like the idea of having it on the lower bound so that we ensure that development happens on the minimum engines version. :)
I guess the one thing is that leaving it running on the lower bound in theory would leave us open to running insecure Node.js versions (e.g. if 22.x.y contains a vulnerability patch, we should probably be running >=22.x.y in development).
Co-authored-by: David Sanders <[email protected]>
Co-authored-by: David Sanders <[email protected]>
Co-authored-by: Erik Moura <[email protected]>
BREAKING CHANGE: bumps required Node.js version to >=22.0.0
Require
Test (20.9)to be removed as a required CI check.