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

Drop fs-extra as dependency #639

Closed
wants to merge 3 commits into from
Closed

Drop fs-extra as dependency #639

wants to merge 3 commits into from

Conversation

ad-m
Copy link
Contributor

@ad-m ad-m commented Jun 11, 2020

Current NodeJS versions have support for fs.promises. In this case, fs-extra is not necessary. See also #638 for drop support for NodeJS 8.

@ad-m
Copy link
Contributor Author

ad-m commented Jun 12, 2020

Anyone have an idea why they don't pass the Node 10 tests?

@leontastic
Copy link
Collaborator

I agree with this change in principle, but I can't merge this if the tests don't pass. Looking at the failed cases it looks like there might be an issue with directory cleanup. I'm currently unable to check out the branch to try locally because it looks like it was deleted. Maybe we can revisit this in the future as part of another PR. Thank you for your effort @ad-m

@leontastic leontastic closed this Feb 11, 2021
@ad-m
Copy link
Contributor Author

ad-m commented Feb 11, 2021

@leontastic you can always pull old pull request via:

git clone [email protected]:jamhall/s3rver.git
cd s3rver
git fetch origin "pull/639/head:pr-639"

@leontastic
Copy link
Collaborator

@ad-m Neat! I gave her a shot and the tests are still failing on Node 10. Not sure if we still want to support Node 10 but it's probably a decision for the next major release. This is a nice-to-have but it's not that urgent.

I would reopen this branch but I can't seem to since it was deleted. Maybe re-submit a PR to track progress on making the Node 10 tests pass? It's up to you if you'd like to take the initiative. Thanks again @ad-m

@ad-m
Copy link
Contributor Author

ad-m commented Feb 11, 2021

Currently, I do not have the resources to continue the work. I deleted my fork, so GitHub show branch as "unknown repository".

If you want - go ahead and take the code.

@kherock
Copy link
Collaborator

kherock commented Feb 12, 2021

Btw, the issue here is simply that the recursive flag wasn't added to fs.rmdir[Sync] until Node 12:

image

If we want to keep Node 10 support, we could bring in a more specialized library like rimraf. My vote is currently just to drop v10 for the next major.

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.

3 participants