search

pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 13 forks source link

Top-down iterative migration to async #93

Closed 2colours closed 11 months ago

2colours commented 11 months ago

If I know it right, it's possible for maintainers to edit this PR directly.

My approach is the following:

  1. go top to bottom
  2. change the callback-driven calls from the caller side
  3. trace the corresponding definition and wrap it into a Promise using the Promise constructor
  4. change the calls to the callback to resolve/reject calls as appropriate
  5. test and repeat with the next layer
2colours commented 11 months ago

No surprise it failed when the tests also need to be adapted to the changing call signature...

confused-Techie commented 11 months ago

I can see this is something we both want to happen lol, as I've started something similar over on #88.

Otherwise, as you can see the tests fail without any helpful logging at all, the tests being fixed to provide much more useful logging has been added over on #87 so I may recommend grabbing those updates once it's merged, which may make this process easier.

2colours commented 11 months ago

@confused-Techie yes, actually I think the testing is a blocker. I'm not sure if this is even doable with this obsolete "fork of fork of fork" version of Jasmine. For now, I'm going to rebase onto #87 to see if it changes this fundamentally; if not, it might be completely pointless to try.

confused-Techie commented 11 months ago

@2colours I actually totally agree that the testing is a bit of a blocker. So by all means hopefully having better tests here can help you to get your PR working. The testing is the main thing that stopped me from continuing my work of async.

And while I'm pretty sure everyone agrees the fork of a fork of jasmine is problematic, it's the same testing code used on Pulsar, and in turn what's used within every single community package. So there's lots of momentum propping it up, so we will probably have to work with it for quite some time more.

2colours commented 11 months ago

@confused-Techie Actually I'm not sure there is much sense in using this version of Jasmine only because other packages are using it. Besides, do you think it will ever become easier to move away from it? I think it's plain to see that at some point, this move must happen. Do you think there will be a point where it will suddenly get easier or more reasonable to do it than "as soon as possible"?

confused-Techie commented 11 months ago

@2colours Maybe I should have been slightly more clear.

What I really meant to say, is we will likely have to always support this testing framework in Pulsar. As Pulsar is the test framework for every single community package, and if we change it in Pulsar we break every single community package's testing. And to move away here, may only makes things more complex, as we are then using multiple different testing suites between two very closely related codebases. So in all reality, probably improving on the existing testing framework may make more sense, if we have to keep using it in the biggest codebase anyways.

But to answer your question, no I don't think it will ever been any easier. The only way it'd become any easier is if we ever manage to fully integrate PPM into Pulsar, at which time this entire codebase would likely be essentially rewritten, which would allow for new tests to be created. But beyond that, it will never be easier than it is now, albeit, likely won't become that much harder.

But trust me, I've brought up a few times the possibility of moving testing suites a couple times. And so have other team members. I'd love to do a proper investigation into this, and scope out what our possibilities may be. But I don't see it happening today. If we do want to move away from it, probably would be best to continue this discussion in Discord, and get more opinions on it, and see where other people are at.

2colours commented 11 months ago

As it can be seen, testing is really problematic with the change of the "topmost interface".

I'm going to try the same thing but omitting this layer of asincification. If that can be tested, I'm going to open a draft PR for that and close this one for history.

2colours commented 11 months ago

Closing this PR as the migration of the main API with this test framework seems like a tedious, if not downright impossible task.