feat!: bump engines requirement to Node 22
#222
Conversation
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.
@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. |
erickzhao
force-pushed
the
node-2+2
branch
2 times, most recently
from
7f0fc11 to
1e024cf
Compare
BREAKING CHANGE: bumps required Node.js version to >=22.0.0
Co-authored-by: David Sanders <[email protected]>
Co-authored-by: David Sanders <[email protected]>
Co-authored-by: Erik Moura <[email protected]>
BREAKING CHANGE: ESM-only. Bumps required Node.js version to >=22.12.0
Require
Test (20.9)to be removed as a required CI check.