-
Notifications
You must be signed in to change notification settings - Fork 298
Remove wrench, fs-plus in favor of fs-extra (Reviving #836) #894
base: master
Are you sure you want to change the base?
Conversation
Down to 27 failures from 67! That was a pretty big chunk of failures caused by a isDirectory/isFile typo...
I'd recommend doing this. I think I was a bit too optimistic when writing the PR that there wouldn't be any exceptions thrown. Just never got around to re-editing all the files 😔. |
As I understand (let me know if this isn't right) the idea is tracking down which methods throw errors vs failing silently, and ensuring those that failed silently before this PR, but now may throw errors, are wrapped in There is native recursive For completeness of discussion of native Node options: There is also native recursive As far as I can tell, there is no native |
Happy to hear from you! 🎉 @50Wliu
Not all of the functions throw an error. You should go through them and check if they throw.
fs-extra already exports the native function. Most of them are identical! You should check the documentation of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I prefer that we add the functions that fs-plus
misses (from fs-extra
and wrench
). We can use a bundler like Rollup to tree-shake and compact the library. We can also make the try-catched version of the functions in fs-plus
if needed.
I have a WIP version of fs-plus
under development here: https://github.com/atom/fs-plus/pull/48/files
Not really, unless
This is great and will make it very easy in the future to switch to the native versions if we so desired :).
👋 My life's at a point where I can't commit to working on Atom anymore, but I should still be able to pop in once in a while on the issues that I have direct subscriptions to. |
I'm not too sure what you mean here; are you saying that we should add the methods from fs-extra/wrench that fs-plus doesn't have to fs-plus? If so, I would recommend against that. I admit my knowledge of Atom is one year stale at this point, but we realized that fs-plus had served its purpose, and there were better-maintained fs libraries out there. |
Yes, that's what I meant. If there are missing functions, we can add those to The API of the some of the functions in fs-plus is different from fs-extra and switching to a new library means that we should change the whole Atom ecosystem. In contrast, by improving Also, as you mentioned, there are some features that are not available in However, this is my opinion about |
For context, it sounds like @aminya you would be willing to have us take on maintaining a fork of That's doable in the very short (immediate) term, but if we get tired of it, that's one more stale dependency. That's basically a similar situation to why If we can use another module that someone else is maintaining, perhaps wrapped in some I don't prefer us to start maintaining custom stuff "because we can". The archeologist/anthropologist in me agrees with your assessment that Atom's maintainers did this a lot, then some core folks who used to work heavily on Atom had other life stuff come up, then a lot of Atom's roll-its-own dependencies became unmaintained and/or stale. So I would lean toward |
I don't think this is exactly true. These are basic operations around moving, These are some basic universal file operations. When I delete something, I want to delete it. When I copy something, I want to copy it. The differences between modules, where they exist, are painstakingly documented. It's worth the initial pain of the switch in order to get a widely used module someone else is committed to maintaining. Or... give up on being current and just use something older. Maintaining new P.S. by substantially changing P.P.S. |
I am not against using better libraries, but there should be a benefit in doing so. For example, I am replacing It is not about popularity but about the features and performance we gain by doing this.
jprichardson/node-fs-extra#746
If there is a native module which offers promisified fs functionality (like Deno), then we can discuss writing a wrapper around it inside |
Sorry for the confrontational tone yesterday. I guess I was grumpy about unrelated life stuff. We don't have to get rid of (Leaving it in a "good" state not meaning the same as "excellent." Don't let the perfect be the enemy of the good. New tech has to be thoroughly proved out, which takes time I don't know that we have. We can and should move forward with something, even if it isn't ready for ES6. Which is stil experimental in Node, by the way, so by convention not suitable for core infrastructure stuff: https://nodejs.org/api/esm.html#esm_ecmascript_modules) Maintaining a modern Popularity isn't valuable in a vacuum, but it means the package gets a lot of attention, is likely to receive bugfixes, and in general is less likely to become suddenly unmaintained without a clear replacement. I'm averse to spreading ourselves thin on things like maintaining a working custom I wouldn't want to stop you from modernizing |
If you want to do this only for |
I like merging larger PRs first, but I am confident in my ability to rebase either way, since my other PRs are tiny. These are supposed to be drop-in replacements, so if we do this PR right, the trail of carnage left behind should be minimal. (And I'll understand these changes, being a partial author to them...) Anyhow... I have to do the boring or tedious part (and out of my expertise area) of researching which methods throw or don't. I'll be looking at the main source Tentative plan, but it's better than no plan! I may not get postable progress on this right away. @aminya you are more the expert among us two for javascript. If you wanted to work on this I'd be glad for it. But we have plenty of things to choose from in terms of what to work on and in the mean-time we can use I'd like to be able to document and/or describe how this is the case (that htese are suitabke drop-in replacements) in a way good enough for a reviewer to (hopefully easily) verify. Or understand it deep enough to answer questions and assure such a reviewer of the details. But that's getting ahead of myself. In summary: next step is researching |
Related to this. atom/fs-plus#50 |
Requirements
Description of the Change
From the original description at #836:
The benefit of this PR over that one, is that this has had
master
properly merged back into it. There is also a developer available to work on it at the moment (me, @DeeDeeG), and if you have any requests, comments or updates you'd like to see done to this, please do let me know.Alternate Designs
Possibly reintroducing the
fs.coffee
wrapper so thattry/catch
blocks can be consolidated into that file. See this review comment on the original PR: #836 (review)Benefits
From #836:
Possible Drawbacks
See: #836
As far as I can tell, all the concerns in the original PR were addressed, other than perhaps the review comment above.
Verification Process
CI passes. All command-line commands I have tried work (
./bin/apm [command] [args]
).Applicable Issues
None.