search

pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 14 forks source link

Begin less reliance on `async` package: Await as we go #134

Closed confused-Techie closed 2 months ago

confused-Techie commented 3 months ago

Within this repo we use the async package nearly everywhere, which while it may have been a game changer when using CoffeeScript, I'd argue with current versions of JavaScript, this isn't really needed.

Although replacing it's usage isn't always straightforward.

So what I've done here is remove the straightforward sections, which mostly consists of async.waterfall() which simply would run an array of async functions one after the other.

Due to this simplicity I was able to easily convert any instances of async.waterfall() to a few await calls wrapped in a try...catch block.

But I've otherwise left async usage in locations with more complex setups, that wouldn't be a simple translation to do. But eventually I'd hope to remove async as a package entirely

confused-Techie commented 2 months ago

@DeeDeeG Thanks for approval on this one! I'd be happy to get it merged, and if I do think you could get pulsar-edit/pulsar#1075 updated with this PR as well?

DeeDeeG commented 2 months ago

[ . . . ] do think you could get https://github.com/pulsar-edit/pulsar/pull/1075 updated with this PR as well?

Yeah, sure. đź‘Ť

savetheclocktower commented 2 months ago

I was going to express anxiety about a last-minute merge of a change like this, but then I saw the diff… and, yes, using async.waterfall in those scenarios is rather silly. I am not anxious about this anymore; the rewrites are straightforward.

DeeDeeG commented 2 months ago

I can confirm ppm ci works as expected, ppm develop does what its help text says it will do (I probably never ran this command before today, not sure I knew it existed!)...

And I can report that ppm dedupe has been quietly broken for some time now, as it fails with the same error message as ppm dedupe failed with as of at least April (as of at least this arbitrary commit I decided to test if not earlier: bc06ac66615eb90e0977e46f45603acefd49c5b0).

% ~/ppm/bin/ppm dedupe                               
Deduping modules Cannot read property 'config' of undefined

So, what worked before kept working, what's broken appears to have been broken since before this PR.

By the way: deduping is mostly an artifact from a time gone by, when npm didn't automatically dedupe, and running the command gives extremely finicky results. I feel it's downright risky for anyone to run it (even in vanilla npm 6 which ppm is based on), as this command has always felt closer to "flawed" than "flawless" in my book. If I recall correctly, it can leave you with missing or mis-recorded dependencies in package-lock.json, and it often doesn't even remove that many packages when it does anything helpful at all. Would like to propose deprecating and/or simply dropping the ppm dedupe command, since I think it does more harm than it helps, personally. But it's no major loss, and no-one's even reported it broken for several months now, maybe longer. (Or we noticed it before, didn't care a whole lot then, and enough time has gone by since then that I forgot we ever noticed it was broken. Either way, it's not super urgent.)