espruino / EspruinoTools

JavaScript library of tools for Espruino - used for the Web IDE, CLI, etc.
Apache License 2.0
150 stars 89 forks source link

Async processor #170

Closed mariusGundersen closed 9 months ago

mariusGundersen commented 9 months ago

I've added two convenience methods, addAsyncProcessor and awaitProcessor, that work better with promises. I've refactored the modules.js file to use async/await and use these two methods, since it relied heavily on promises already.

I have gone ahead and used some more modern js features, like async/await, arrow functions and object shorthands. I'm assuming this code will only run in fairly modern js engines, like Chrome and nodejs browsers.

gfwilliams commented 9 months ago

Sorry, but I'm really not a fan of this - async isn't used anywhere in EspruinoTools/IDE and I don't see a reason to add it for this one case, especially as the new functionality is undocumented and used only in modules.js.

There seems no real benefit to using async other than preference over the look of code, and potentially it'll make it not run on some old systems as you note (like I still have Pis used for test harnesses with the CLI that are using older Node.js versions). We also bundle this up for use in BangleApps where we run it on all kinds of stuff (like old ios and android that could use very old webviews) and there is a real possibility it'll cause issues there.

If this were providing a real benefit, fine. I'm all for switching towards Promises (and I can understand addAsyncProcessor although it could really do with handling rejections so the chain continues).

... also I'm really against refactoring big chunks of code right before making a change like the module path you mentioned. When there's an issue it makes it much harder to track stuff down.

I'm also not sure why you're changing things like function() { to function () {, or a!==b to a !== b. Pretty much every other file uses the first form, so all you're doing is making the files you've edited not match everything else, and screwing up the history when we need to use git blame.

mariusGundersen commented 9 months ago

Thanks for the honest feedback, that's why I made this a separate PR from the other module changes.

I think there are some clear advantages to async await, or at least promises. For example it's not clear from the code alone of a callback will be called once, zero or multiple times. There is also no way with the callbacks to handle failures, which is why I haven't made an effort to handle rejections; it should behave as it does today.

Good to know that this code needs to run on old platforms, I'll keep that in mind in other PRs (and restrain my love for modern JS).

Sorry about all the formatting changes, it's vs code doing it automatically. Do you have a preferred formatter that can help out?

gfwilliams commented 9 months ago

Thanks! I'll close this for now then...

On the advantages - yes, Promises are a big improvement, and I'm all for moving over to using them (so keeping addAsyncProcessor may still be handy, although it's not hard to use addProcessor with Promises). My concern really is with async - I feel like it doesn't add much functionality over promises (it's mainly just hiding the complexity), and as mentioned could likely cause issues with older JS engines.

That's interesting about the VS code formatting - I guess it's off on mine. I don't have a formatter or specific set of rules I use, but the general code style used over Espruino is like this: https://www.espruino.com/Code+Style