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

cleanup(js): replace fs-extra with node:fs #27932

Merged
merged 7 commits into from
Sep 17, 2024

Conversation

ziebam
Copy link
Contributor

@ziebam ziebam commented Sep 16, 2024

This PR is a suggestion to replace fs-extra with the native node:fs to reduce the dependency count and the bundle size.

Current Behavior

@nx/js uses fs-extra which, at present, mostly wraps the native node:fs API.

The biggest change is fs-extra.copySync -> fs.cpSync. The latter has been added in Node 16.7.0, and has been marked as "experimental" since, dropping that status in Node 22. The API seems stable across the releases though, and the affected tests pass, so this change might be acceptable. Other possible solutions:

  1. Keep fs-extra just for this function, and eventually remove it in the distant future where Node version older than 22 are deemed obsolete.
  2. Mirror the cpSync behavior by writing a utility that uses stable methods fs.writeFile, fs.stat, etc.

Expected Behavior

Reduced dependency count and bundle size by replacing fs-extra with node:fs.

Related Issue(s)

N/A

@ziebam ziebam requested a review from a team as a code owner September 16, 2024 15:52
Copy link

vercel bot commented Sep 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview Sep 17, 2024 11:04am

Copy link
Collaborator

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

Thank you for this @ziebam.

It could possibly cause issues for any in flight branches/PRs. To help make things easier to both figure out and enforce, please add something like the following ESLint rule to the js package .eslintrc.json

"no-restricted-imports": ["error", {
    "name": "fs-extra",
    "message": "Please use equivalent utilties from `node:fs` instead."
}]

@JamesHenry JamesHenry self-assigned this Sep 17, 2024
@ziebam ziebam requested a review from a team as a code owner September 17, 2024 09:48
@ziebam
Copy link
Contributor Author

ziebam commented Sep 17, 2024

Thank you for this @ziebam.

It could possibly cause issues for any in flight branches/PRs. To help make things easier to both figure out and enforce, please add something like the following ESLint rule to the js package .eslintrc.json

"no-restricted-imports": ["error", {
    "name": "fs-extra",
    "message": "Please use equivalent utilties from `node:fs` instead."
}]

Done. I also merged with master, although that might have erroneously triggered GitHub to ask another person for a review, apologies.

@JamesHenry
Copy link
Collaborator

@ziebam So you tested this catches cases of importing from fs-extra, yeah?

@ziebam
Copy link
Contributor Author

ziebam commented Sep 17, 2024

@ziebam So you tested this catches cases of importing from fs-extra, yeah?

Yup, here's a screenshot for the record.
image

Not sure about the CI failure, investigating right now.

@JamesHenry JamesHenry merged commit d38bb78 into nrwl:master Sep 17, 2024
6 checks passed
@ziebam ziebam deleted the remove-fs-extra branch September 17, 2024 11:49
@JamesHenry
Copy link
Collaborator

Thanks @ziebam

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.

2 participants