Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task Queue Utility #1213

Closed
wants to merge 3 commits into from
Closed

Conversation

TheDudeFromCI
Copy link
Member

This is a helpful utility object that allows players to define a list of async tasks to perform without needing to working about making a nested pyramid of callbacks. This adds a procedural style, much closer to what new programmers are used to coding with, so it'll help make the transition to callbacks a bit easier. Plus, it looks much nicer.

@TheDudeFromCI
Copy link
Member Author

I also decided to make this PR because I figure it would help with the transition to prismarine-world, as a lot of the functions, such as blockAt, are sync functions.

@Karang
Copy link
Contributor

Karang commented Aug 5, 2020

I think this pr is a good idea. But I don't see what you mean about pworld : it has both a sync and async api. The sync pworld was added especially for mineflayer

@TheDudeFromCI
Copy link
Member Author

Ah, okay. I was browsing this thread: #334 and thought that sync was considering being depreciated once the switch to pworld was complete. Guess I misunderstood that part.

Well, the utility is pretty useful regardless, lol.

Copy link
Member

@wvffle wvffle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion this could be done by some external library and will be void at the time of merging #984. However, the code looks ok.

@TheDudeFromCI
Copy link
Member Author

Agreed, this is just a small utility until then.

Plus I don't even know how to use promises anyway.

@rom1504
Copy link
Member

rom1504 commented Aug 5, 2020

Hi,
I think adding such an example is useful to help people with async.
However I don't think it makes sense to add this utility.
I think it would be better in the example to use some existing lib such as https://caolan.github.io/async/v3/ or to use promises. A third way can be to put this queue inside the example.

It does not make a lot of sense to maintain this kind of task queue inside mineflayer as it doesn't contain a good subset of what people may want to do with async and I bet we would have to add many things to it, eventually leading to having some kind of clone of async.js

@TheDudeFromCI
Copy link
Member Author

Alright then, I'll close this.

@rom1504
Copy link
Member

rom1504 commented Aug 5, 2020

Putting the queue in the example might be a reasonable choice, to show that implementing such a queue is one way to handle async but not the only way (and it could be good to talk about the other ways in comments in the example)

@rom1504
Copy link
Member

rom1504 commented Aug 5, 2020

Why close it ? I do mean adding the example is useful

@TheDudeFromCI
Copy link
Member Author

How would I add it as an example if you said don't add the utility?

@TheDudeFromCI
Copy link
Member Author

I mean, I could post the entire utility class in the example file, but that seems a bit much.

@wvffle
Copy link
Member

wvffle commented Aug 5, 2020

Indeed, there are libraries that do even more than this util, I opt for adding an example how to use them both in examples/ and tutorial

@TheDudeFromCI
Copy link
Member Author

Adding one of those async libraries as an example would be useful as an alternative, but that would be a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants