Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Remove wrench, fs-plus in favor of fs-extra #836

Closed
wants to merge 36 commits into from
Closed

Conversation

50Wliu
Copy link
Contributor

@50Wliu 50Wliu commented Apr 23, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

wrench has been deprecated for a few years in favor of fs-extra. However, since both fs-plus and fs-extra happen to extend fs, it would be illogical to use both of them in the same codebase. Therefore, this PR removes both wrench and fs-plus and uses fs-extra instead.

Alternate Designs

None.

Benefits

  1. Using a better-maintained fs alternative
  2. No longer depending on a deprecated package that isn't being updated

Possible Drawbacks

There were two troublesome functions with this swap:

  1. fs.is(Directory|File)Sync

fs-extra doesn't have any way to easily determine if a path is a directory or file without the possibility of throwing exceptions. I've tried to emulate that as best as I could using trys everywhere, but it's not that good-looking and the behavior might be subtly different in a few places (we may now throw where we didn't before).

  1. fs.writeFileSync

fs-plus creates any necessary directories first if the path doesn't exist; fs-extra doesn't. I tried to add fs.mkdirp calls wherever necessary but I may have missed a few.

Verification Process

For now, just the specs. This will clearly need some extra real-world stress testing as I do not trust these specs.

Applicable Issues

N/A

@50Wliu 50Wliu marked this pull request as ready for review April 23, 2019 20:45
@50Wliu
Copy link
Contributor Author

50Wliu commented Apr 25, 2019

Checked over all the writeFileSyncs and can confirm that they'll always be writing in an existing folder 👍

Copy link
Contributor

@as-cii as-cii left a comment

Choose a reason for hiding this comment

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

Thanks for getting rid of some deprecated packages, @50Wliu! ✨

@nathansobo and I reviewed this, and we think it might make sense to keep the wrapper fs.coffee file, so that we can put try/catch blocks around functions that now throw an exception in a single place and reduce the risk of missing some of them.

Other than that, this seems like a great change and we should merge this once conflicts are resolved. ⚡️

@DeeDeeG
Copy link
Contributor

DeeDeeG commented May 20, 2020

Is this PR still active? I would be interested in taking a shot at updating this against master.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Aug 10, 2020

Here is an updated version of this PR (with master branch merged in): master...DeeDeeG:wl-fs-extra-merge-master

CI passes: https://travis-ci.com/github/DeeDeeG/apm/builds/179155533

Please consider reviewing. IMO it would be great to start updating apm's dependencies, and I'd be excited if this got merged! Thanks for considering.


For your reference, these were the significant commits to master since this PR was posted that caused manual intervention to be needed:

@50Wliu
Copy link
Contributor Author

50Wliu commented Aug 11, 2020

Is this PR still active?

Uhh...not really, sorry 😬. If you have a branch that is updated, I'd encourage you to open it as a PR!
I'll close this to reduce confusion.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Aug 11, 2020

@50Wliu thanks for the reply! I opened #894 with the updated branch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants