Skip to content

chore(deps): drop fs-extras and mkdirp dependencies#3230

Merged
rpl merged 3 commits intomozilla:masterfrom
fregante:fs-extra
Aug 23, 2024
Merged

chore(deps): drop fs-extras and mkdirp dependencies#3230
rpl merged 3 commits intomozilla:masterfrom
fregante:fs-extra

Conversation

@fregante
Copy link
Contributor

@fregante fregante commented Aug 23, 2024

See review comments below

@fregante fregante mentioned this pull request Aug 23, 2024
16 tasks
@fregante fregante marked this pull request as ready for review August 23, 2024 13:36
"eslint": "8.57.0",
"eslint-plugin-async-await": "0.0.0",
"eslint-plugin-import": "2.29.1",
"fs-extra": "11.2.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still used in tests, I didn't want to rewrite all of them. It's fine as a dev dependency

Copy link
Member

Choose a reason for hiding this comment

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

that's fair, it seems to be used only in the test.chromium.js unit test, but it is not a big deal if we keep it as a dev dependency in the meantime. Let's keep that test as is 👍

if (userDataDir && profileDirName) {
// copy profile dir to this temp user data dir.
await fs.copy(
await fs.cp(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to Node 16.7 exactly 3 years ago

https://nodejs.org/api/fs.html#fspromisescpsrc-dest-options

log.debug(`Creating reload-manager-extension in ${extPath}`);

await asyncMkdirp(extPath);
await fs.mkdir(extPath, { recursive: true });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fregante fregante changed the title chore(deps): drop fs-extras and mkdirp dependencies chore(deps): drop fs-extras and mkdirp dependencies Aug 23, 2024
Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@fregante thank you, I'm signing off this version of this PR as is (and merging it right after).

I also took a look to the mkdirp README and based what they are stating in it mkdir was basically already using the native recursive fs.mkdir feature available in nodejs > 10, besides in a few cases that we shouldn't be hitting and for a couple of cases wher the errors raised by the native implementation wasn't clear on why the call was failing (but the issues referenced seems to have been also fixed in th meantime, merged in nodejs v22 if I'm not mistaken, and so I'm not too worried about those to be honest).

I also agree that keeping fs-extra as a dev dependency in this PR is fine (so that we can keep test.chromium.js tests unchanged).

"eslint": "8.57.0",
"eslint-plugin-async-await": "0.0.0",
"eslint-plugin-import": "2.29.1",
"fs-extra": "11.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

that's fair, it seems to be used only in the test.chromium.js unit test, but it is not a big deal if we keep it as a dev dependency in the meantime. Let's keep that test as is 👍

@rpl rpl merged commit 248d382 into mozilla:master Aug 23, 2024
@fregante fregante deleted the fs-extra branch August 23, 2024 17:56
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