Skip to content

Conversation

Syjalo
Copy link
Contributor

@Syjalo Syjalo commented Feb 23, 2023

Please describe the changes this PR makes and why it should be merged:
This PR adds new iterators and getters that allow you use collectors without deep nesting.

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@vercel
Copy link

vercel bot commented Feb 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
discord-js ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 24, 2023 at 7:01AM (UTC)
discord-js-guide ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 24, 2023 at 7:01AM (UTC)

Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

The next_ and the new async iterators look quite similar, these could probably be abstracted, if it doesn't hurt readability too much 👀

@Jiralite Jiralite added this to the discord.js 15.0.0 milestone Jul 31, 2024
@Jiralite Jiralite removed the blocked label Oct 1, 2024
@almeidx
Copy link
Member

almeidx commented Dec 5, 2024

This needs a rebase

* @type {Promise}
* @readonly
*/
get nextCollecting() {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we shouldn't have async getters. What about waitForCollect()? Same for the other methods.

I also think that it'd be useful to accept an AbortSignal so we can cancel and unregister the listeners earlier.

Comment on lines 387 to 389
this.removeListener('collect', tick);
this.removeListener('dispose', tick);
this.removeListener('end', tick);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't removing the ignore listener that's registering in line 394.

@kyranet
Copy link
Member

kyranet commented Dec 9, 2024

Also, regarding this description:

This PR adds new iterators and getters that allow you use collectors without deep nesting.

Technically you can achieve the helper iterators by filtering the main iterator:

for await (const [type, value] of collector) {
	if (type !== ...) continue;
}

However, they would use more resources than the dedicated ones, as it registers 2 more events. And the next one can achieved with similar code.

Praying to get async iterator helpers soon so we can do this 🙏🏼:

const next = await collector.filter(([type]) => type === ...).next();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Review in Progress

Development

Successfully merging this pull request may close these issues.

5 participants