Skip to content
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.

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

refactor: replace fs-extra with newer fs built-ins #5284

Merged
merged 2 commits into from
Jan 2, 2025

Conversation

ericcornelissen
Copy link
Contributor

PR Checklist

Overview

This updates the codebase to remove two uses of fs-extra by newer fs built-in functions. The code changes are based on the implementation of [email protected] (which is the version of fs-extra recorded in the project's lockfile).

This updates the codebase to remove two uses of fs-extra by newer fs
built-in functions. The code changes are based on the implementation of
[email protected], which is the version of fs-extra recorded in the
project's lockfile.

In particular, for `watch.spec.js`, the changes are based on:
https://github.com/jprichardson/node-fs-extra/blob/0220eac966d7d6b9a595d69b1242ab8a397fba7f/lib/remove/index.js#L15

and, also for `helper.js`, the changes are based on:
https://github.com/jprichardson/node-fs-extra/blob/0220eac966d7d6b9a595d69b1242ab8a397fba7f/lib/mkdirs/make-dir.js#L23
Copy link

linux-foundation-easycla bot commented Jan 2, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ericcornelissen / name: Eric Cornelissen (9f4c344)
  • ✅ login: JoshuaKGoldberg / name: Josh Goldberg ✨ (6fd91c4)

@ericcornelissen ericcornelissen changed the title Replace fs-extra with newer fs built-ins refactor: replace fs-extra with newer fs built-ins Jan 2, 2025
fs.ensureDirSync(path.dirname(dest));
fs.copySync(fixtureSource, dest);
fs.mkdirSync(path.dirname(dest), { recursive: true });
fs.cpSync(fixtureSource, dest);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the CI logs, it seems this API is (the only one) missing from Node.js v14. It looks like that version of Node.js only has functions to copy individual files.

@JoshuaKGoldberg I have three suggestions for moving forward:

  1. Write a custom implementation of fs.cpSync here.
  2. Postpone this change until Node.js v14 is no longer run in CI.
  3. Drop the Node.js v14 job since it's no longer supported.

Copy link
Member

Choose a reason for hiding this comment

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

Fun timing: #5279 just dropped v14 in CI. If you update this branch from main it should be good to go! 🚀

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it is updated from main. Investigating...

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I tried it out locally and all seems well. 🚀

Since this is just an internal refactor, I feel comfortable merging once CI is green.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 75dcf8c into mochajs:main Jan 2, 2025
75 checks passed
@ericcornelissen ericcornelissen deleted the 5266-fs-extra branch January 2, 2025 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🛠 Repo: Replace fs-extra with newer fs built-ins
2 participants