search

pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 13 forks source link

Migrate from `rimraf` to NodeJS `fs` #108

Closed confused-Techie closed 8 months ago

confused-Techie commented 8 months ago

In this repo we have the module dependency rimraf which is used in a singular location to bring unix like file deletions into the copy and move commands.

With such little usage, it seemed easy enough to migrate this to the builtin NodeJS fs module's rm method.

After migrating that then means we are able to fully go ahead and remove the rimraf dependency from this repository completely.

As for any concerns about the change, the options provided to fs.rm, according to the docs, will mimic Unix rm -rf functionality. Which is quite literally the repo description of rimraf.

DeeDeeG commented 8 months ago

(Getting sidetracked, feel free to skip this: To be honest, this entire file is suspect for being replaced with core fs module stuff -- once we get on newer Electron and Node versions, we should consider core fs.cp instead of ncp as well. But that requires Node v16.7.0 or newer and is still experimental, so no dice on that quite yet... And that's a bit too much scope creep for this one obviously applicable dependency replacement today.)

(A long term "wishlist item" is to get rid of wrench, too.)

confused-Techie commented 8 months ago

Thanks for taking a look at this one as well.

And great catch on the import name. But yeah otherwise this one felt easy enough and could get rid of the whole dep. But you are totally right that I bet (maybe after a bump or two) we can remove ncp as well, which would be great. Even from there, if we were dedicated to use only normal NodeJS fs we could remove fs-plus especially considering that all fs usage comes from the main file edited in this PR.

But once tests are happy after all the changes we can go ahead and merge this one!

DeeDeeG commented 8 months ago

[ EDIT: Okay, I guess it's just a network error. Probably a fluke, but updating yarn.lock still wouldn't hurt, IMO. ]

Bizarre error on Windows Node 16 CI run, Yarn's not happy:

> error Couldn't find package "rimraf@^3.0.2" required by "node-gyp@9.4.0" on the "npm" registry.

(rimraf@3.0.2 definitely exists: https://www.npmjs.com/package/rimraf?activeTab=versions)

Maybe this is a fluke and never happens again. On the other hand, could be good to update the yarn.lock file and commit it in this PR? Maybe that helps?

DeeDeeG commented 8 months ago

I can push commit https://github.com/pulsar-edit/ppm/commit/53d3d5f6a4d01ef825dd63a6b4b3c757729cd88f to this branch if you want an updated yarn.lock lockfile in this PR. Otherwise I can do a follow-up PR to sync yarn.lock. Also if you get to it without reading this, then no worries, lol.

confused-Techie commented 8 months ago

@DeeDeeG if you'd like feel free to push it, otherwise I can get an update in within the next 30 minutes

DeeDeeG commented 8 months ago

@confused-Techie done, I pushed the updated lockfile to this PR's branch just now.

confused-Techie commented 8 months ago

Thanks @DeeDeeG merging this one now then!