sindresorhus / p-queue

Promise queue with concurrency control
MIT License
3.45k stars 185 forks source link

make PriorityQueue array protected rather than private #140

Open trajano opened 3 years ago

trajano commented 3 years ago

This will allow us to extend the existing priority queue to make methods that change to the underlying structure for example "remove" to remove entries from the promise queue for scenarios like an image component getting unmounted so there's no point in keeping the promise of downloading the image.

Right now I am copying the class and adding the new method

  removeWithPredicate(predicate: (entry: PriorityQueueOptions & { run: RunFunction }) => boolean): void {
    let i, j;

    for (i = 0, j = 0; i < this._queue.length; ++i) {
      if (predicate(this._queue[i])) {
        this._queue[j] = this._queue[i];
        ++j;
      }
    }

    while (j < this._queue.length) {
      this._queue.pop();
    }
  }

based off https://stackoverflow.com/a/54270177/242042

sindresorhus commented 3 years ago

That's very much intentional. Allowing subclassing makes pretty much any internal behavior public. It's not something I would want to commit to. However, I'm happy to see a "remove" like method added.

trajano commented 3 years ago

It would be easier than to get people to fork it off. But I've gotten most of it done now. It's just a matter of figuring out how to call the remove method I have written above. Without doing a full copy of the class. The thing that is confusing me is how to convert the item I want to remove to something that would be a RunFunction and also Task does not seem to be exported.

Looking through the code it's not feasible to use the run to check whether the item has to be removed from the queue as run is built as an anonymous function and we can't easily map that to the original function. So I modified the code I had above to take in a predicate to evaluate whether to delete the record

What I think it needs as well is to add the fn variable into the PriorityQueueOptions object. That way the predicate can check whether the entry should be removed. I noticed I used run directly so I am changing that to match the queue data

Going to continue the remove discussion on #76

Richienb commented 2 years ago

Maybe we could imitate the prototype functions of Set within the class. Would be breaking through.

Richienb commented 2 years ago

@sindresorhus What do you think? This would allow the queue to support features like #76.

sindresorhus commented 2 years ago

Maybe we could imitate the prototype functions of Set within the class. Would be breaking through.

Not sure I see the benefit of that?

I think just adding a .remove() method to solve this well-defined problem is better.