Repository navigation
chore: use native node:fs for removal scripts - #26219
Conversation
|
|
||
| for (const path of globSync(['suites/**/dist', 'loader-suites/**/dist', 'tmp'])) { | ||
| rmSync(path, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
Clean scripts fail on Node 20
Medium Severity
The new clean scripts import globSync from node:fs, but that API ships only in Node 22+. This repo still supports Node 20.19 (engines and CI), so pnpm clean and the browser-integration-tests pretest crash there with a missing-export / not-a-function error instead of removing build artifacts.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ee036d8. Configure here.
There was a problem hiding this comment.
- browser-integration-tests are not run against 20.x in CI, they run against 24.22.1 (per whats in package.json)
- no other CI job currently runs clean
Part of tidying up dependencies, supply chain security, etc. This replaces `rimraf` (7MB, 9 packages) with an in-house script of a few lines of `node:fs`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9b03122. Configure here.
| const { globSync, rmSync } = require('fs'); | ||
|
|
||
| for (const path of globSync(['suites/**/node_modules', 'suites/**/tmp_*'])) { | ||
| rmSync(path, { recursive: true, force: true }); |
There was a problem hiding this comment.
Clean scripts crash on Node 20
Medium Severity
These clean scripts import globSync from node:fs, which was added in Node 22 and is not present on Node 20. node-integration-tests and browser-integration-tests still declare >=20.19.0 in engines, and CI runs the Node integration suite on 20.19. On that version the import fails, so cleanup never runs — including the docker teardown that follows in clean.js.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 9b03122. Configure here.
There was a problem hiding this comment.
- browser-integration-tests are not run against 20.x in CI, they run against 24.22.1 (per whats in package.json)
- no other CI job currently runs clean
andreiborza
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for the contribution!
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #26219 Co-authored-by: andreiborza <168741329+andreiborza@users.noreply.github.com> Co-authored-by: Martin Sonnberger <martin.sonnberger@sentry.io>
**Why:** Removing rimraf in #26219 left two unused lockfile entries that pnpm prunes during installation, causing develop's lockfile stability check to fail. Commit the generated cleanup to restore CI. Co-authored-by: OpenAI Codex <codex@openai.com>
**Why:** Removing rimraf in #26219 left two unused lockfile entries that pnpm prunes during installation, causing develop's lockfile stability check to fail. Commit the generated cleanup to restore CI. Co-authored-by: OpenAI Codex <codex@openai.com>
**Why:** Removing rimraf in #26219 left two unused lockfile entries that pnpm prunes during installation, causing develop's lockfile stability check to fail. Commit the generated cleanup to restore CI. Co-authored-by: OpenAI Codex <codex@openai.com>
Why: Part of #26187. Barely any of rimraf's functionality is used in this repo, so its just acting as a convenience wrapper to let us call it via CLI. We can achieve the same through a small script.
How to review: Just ensure the scripts are consistent with each other and CI passes.
Unsure about: N/A
@andreiborza it actually turns out the bundler-plugin-integration-tests already use
premoveindevelop👀