Remove `fs-extra` by pralkarz · Pull Request #1476 · changesets/changesets

pralkarz · 2024-10-03T18:52:58Z

Part of the v3 cleanup: #1473. fs-extra is effectively unnecessary in modern Node versions, and can be replaced with the native functionality.

One file left because it depends on other packages.

changeset-bot · 2024-10-03T18:53:01Z

🦋 Changeset detected

Latest commit: 4b54c84

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages

Name	Type
@changesets/apply-release-plan	Patch
@changesets/release-utils	Patch
@changesets/test-utils	Patch
@changesets/config	Patch
@changesets/write	Patch
@changesets/read	Patch
@changesets/cli	Patch
@changesets/git	Patch
@changesets/pre	Patch
@changesets/get-release-plan	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2024-10-03T18:55:02Z

Codecov Report

Attention: Patch coverage is 79.66102% with 12 lines in your changes missing coverage. Please review.

Please upload report for BASE (next@ac917ca). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
packages/apply-release-plan/src/index.ts	80.95%	4 Missing ⚠️
packages/git/src/index.ts	0.00%	4 Missing ⚠️
packages/cli/src/run.ts	40.00%	3 Missing ⚠️
packages/cli/src/commands/init/index.ts	87.50%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             next    #1476   +/-   ##
=======================================
  Coverage        ?   81.01%           
=======================================
  Files           ?       54           
  Lines           ?     2239           
  Branches        ?      657           
=======================================
  Hits            ?     1814           
  Misses          ?      421           
  Partials        ?        4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pralkarz · 2024-10-03T18:54:42Z


 afterEach(() => {
  console.error = consoleError;
+  jest.restoreAllMocks();


Some tests were failing without this when run as a suite, but passed individually. Not sure why.

hm, interesting - i'll take a look into this before landing htis

pralkarz · 2024-10-03T18:57:15Z

+      let pathExists = await access(changesetPath)
+        .then(() => true)
+        .catch(() => false);


Same here, could be a common utility function somewhere. This implementation is also what fs-extra.pathExists is doing under the hood, so we might need to credit the author here? Not sure since even Node documentation suggests using access for asynchronous existence checks.

This is needed due to a conflict with Node types that was fixed in 2.3.4. Also dedupes the dependency, so overall a net positive. See: https://github.com/enquirer/enquirer/blob/master/CHANGELOG.md#234---2020-01-13

bluwy · 2024-10-04T07:01:47Z

This looks great to me too. I don't know what others think, but I usually prefer import fs from 'node:fs/promises rather than named imports because the API surface is large and names can likely collide or misinterpreted as custom functions. But I don't think it's a big deal if others are fine by the named imports too.

For the access/then/catch pattern, I wonder if it's also fine to simply use fsSync.existsSync. It's not exactly the same as access also checks for file permissions rather than just existing, but I don't know if it'd matter in most case. But again also not a big deal for me 😄

pralkarz · 2024-10-04T10:54:21Z

This looks great to me too. I don't know what others think, but I usually prefer import fs from 'node:fs/promises rather than named imports because the API surface is large and names can likely collide or misinterpreted as custom functions. But I don't think it's a big deal if others are fine by the named imports too.

For the access/then/catch pattern, I wonder if it's also fine to simply use fsSync.existsSync. It's not exactly the same as access also checks for file permissions rather than just existing, but I don't know if it'd matter in most case. But again also not a big deal for me 😄

I'm fine with changing to default imports as my preference for named imports is very shallow, as in I can't even quite put my finger on why I prefer them. I'm gonna hold off on making the change for now though, in case someone with a stronger opinion pops in! 😄

As for existsSync, that's what I've usually ended up doing in other projects where I've worked on removing fs-extra, but I usually try to keep the behavior as close to the original fs-extra usage as possible on the first pass, and then make the change if the maintainers are fine with it.

Andarist · 2024-10-04T14:59:20Z

i softly prefer dotted access to those but it's harder to set up autoimports for that than for named exports so 🤷

Andarist · 2024-10-04T16:16:48Z

@ziebam could you add a changeset to this PR?

pralkarz · 2024-10-04T17:51:12Z

@ziebam could you add a changeset to this PR?

I initially branched off off main (the next branch didn't exist yet), and later caught up with next manually, so now I get this error:

$ packages/cli/bin.js
🦋  error Error: Failed to find where HEAD diverged from next. Does next exist?
🦋  error     at getDivergedCommit (/home/pralky/dev/changesets/packages/git/src/index.ts:51:11)
🦋  error     at getChangedFilesSince (/home/pralky/dev/changesets/packages/git/src/index.ts:196:22)
🦋  error     at getChangedPackagesSinceRef (/home/pralky/dev/changesets/packages/git/src/index.ts:258:24)
🦋  error     at getVersionableChangedPackages (/home/pralky/dev/changesets/packages/cli/src/utils/versionablePackages.ts:15:27)
🦋  error     at add (/home/pralky/dev/changesets/packages/cli/src/commands/add/index.ts:47:7)
🦋  error     at run (/home/pralky/dev/changesets/packages/cli/src/run.ts:72:5)

Should I temporarily change the "baseBranch" in the config to main to solve this? Also, do I just do the regular patch bump in this case? Not sure what's the correct approach here since this is technically working towards a new major release.

Andarist · 2024-10-04T18:58:24Z

Hm, it might be worth investigating why this happens because it's not a good experience for the users. I'll try to repro it later using this branch. To recheck a thing - do you have a next branch locally? What is its tip, is it ac917ca?

Also, do I just do the regular patch bump in this case? Not sure what's the correct approach here since this is technically working towards a new major release.

I'd use patch since it's not a significant change and we want it to be listed in the patch section of changes in the final v3 release

pralkarz · 2024-10-04T19:05:36Z

Hm, it might be worth investigating why this happens because it's not a good experience for the users. I'll try to repro it later using this branch. To recheck a thing - do you have a next branch locally? What is its tip, is it ac917ca?

Also, do I just do the regular patch bump in this case? Not sure what's the correct approach here since this is technically working towards a new major release.

I'd use patch since it's not a significant change and we want it to be listed in the patch section of changes in the final v3 release

I thought I had (I pulled the refs earlier), but I had to actually checkout next and then back to my branch to get it to work. Seems like a mistake on my part then.

Andarist · 2024-10-04T19:16:53Z

I'm glad that you have resolved the issue. I think this could be improved in the tool though - likely we don't consult the remote at all in this kind of a situation, the local branch could also be outdated. It might be worth rechecking how Changesets behave in cases like this.

pralkarz · 2024-10-04T19:30:08Z

I'm glad that you have resolved the issue. I think this could be improved in the tool though - likely we don't consult the remote at all in this kind of a situation, the local branch could also be outdated. It might be worth rechecking how Changesets behave in cases like this.

Indeed. Should I file an issue for that or would you prefer to have it in the v3 discussion?

Related: pushed the changeset and changed the named imports to default ones per your and @bluwy's suggestions.

Andarist · 2024-10-04T21:12:08Z

Should I file an issue for that or would you prefer to have it in the v3 discussion?

Let's keep it in that v3 discussion for now.

bluwy

Looks great! I think the fs.access thing is probably worth revisiting, but we could do that later.

ziebam added 11 commits October 3, 2024 20:02

Remove fs-extra from packages/cli

fd49a42

One file left because it depends on other packages.

Remove fs-extra from packages/apply-release-plan

5b514b3

Remove fs-extra from packages/config

8c4e89d

Remove fs-extra from packages/git

913d1c8

Remove fs-extra from packages/pre

4d6b1c1

Remove fs-extra from packages/read

08cc11f

Remove fs-extra from packages/release-utils

89e7355

Remove fs-extra from packages/write

d4ad902

Remove fs-extra from scripts/test-utils

3911928

Finalize version.test.ts change

a1af212

Merge 'upstream/next' into 'remove-fs-extra'

8dd9b90

Remove @types/fs-extra

a99a4ef

pralkarz commented Oct 3, 2024

View reviewed changes

Move common helpers to test-utils

2b1d818

pralkarz force-pushed the remove-fs-extra branch from 2d760a3 to 2b1d818 Compare October 3, 2024 21:20

Update enquirer version

b80a8c5

This is needed due to a conflict with Node types that was fixed in 2.3.4. Also dedupes the dependency, so overall a net positive. See: https://github.com/enquirer/enquirer/blob/master/CHANGELOG.md#234---2020-01-13

remove fs mocks

bfb50c1

ziebam added 2 commits October 4, 2024 21:28

Change to default imports

d7e4710

Add a changeset

fd6a24c

Andarist added 3 commits October 4, 2024 23:45

use single fs module

09e8138

fix silly mistake

4916579

fmt

4b54c84

bluwy approved these changes Oct 5, 2024

View reviewed changes

Andarist merged commit e0e1748 into changesets:next Oct 5, 2024

github-actions Bot mentioned this pull request Oct 5, 2024

Version Packages (next) #1481

Merged

pralkarz deleted the remove-fs-extra branch October 5, 2024 18:50

marcalexiei mentioned this pull request Oct 9, 2024

Update and align @types/jest with jest runtime and remove ts-ignores #1467

Closed

pralkarz mentioned this pull request Jan 24, 2025

Remove fs-extra from changesets e18e/ecosystem-issues#111

Closed

Conversation

pralkarz commented Oct 3, 2024

Uh oh!

changeset-bot Bot commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

codecov Bot commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

pralkarz Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

Andarist Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

pralkarz Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

bluwy commented Oct 4, 2024

Uh oh!

pralkarz commented Oct 4, 2024

Uh oh!

Andarist commented Oct 4, 2024

Uh oh!

Andarist commented Oct 4, 2024

Uh oh!

pralkarz commented Oct 4, 2024

Uh oh!

Andarist commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pralkarz commented Oct 4, 2024

Uh oh!

Andarist commented Oct 4, 2024

Uh oh!

pralkarz commented Oct 4, 2024

Uh oh!

Andarist commented Oct 4, 2024

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot Bot commented Oct 3, 2024 •

edited

Loading

codecov Bot commented Oct 3, 2024 •

edited

Loading

Andarist commented Oct 4, 2024 •

edited

Loading