Skip to content

update priorityQueue functionality to match queue [fixes #1725]#1790

Merged
hargasinski merged 1 commit intomasterfrom
promisify_priority_queue
Apr 15, 2022
Merged

update priorityQueue functionality to match queue [fixes #1725]#1790
hargasinski merged 1 commit intomasterfrom
promisify_priority_queue

Conversation

@hargasinski
Copy link
Copy Markdown
Collaborator

This PR updates the priorityQueue behaviour to match the queue implementation:

  • better supports promises in priorityQueue (see PriorityQueue push doesn't wait for result #1725).
  • supports pushAsync and removes unshiftAsync.
  • fixes a bug where drain wasn't being called when an empty task was pushed.
  • eliminates duplicate code by keeping all of the queue logic in queue. The one issue with this is it now iterates over the tasks twice, once in priorityQueue and once in queue. Let me know if there's a better way you can think of.

I also added a few tests from queue to priorityQueue to make sure the functionality matches.

@hargasinski
Copy link
Copy Markdown
Collaborator Author

The failed tests don't seem to be related to this PR, tests for other functions are timing out.

Copy link
Copy Markdown
Collaborator

@aearly aearly left a comment

Choose a reason for hiding this comment

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

Thanks for this, this has been a missing feature for a while. Just a small question.

Comment thread test/priorityQueue.js
expect(q.length()).to.equal(0);
call_order.push('callback ' + 3);
});
q.push(4, 2.9, (err, arg) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The expected lengths change, due to pushing this array, correct?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes sir, we don't test pushing an array in any of the other priorityQueue tests, so I thought it would be useful to add.

@hargasinski
Copy link
Copy Markdown
Collaborator Author

@aearly is this good to go?

@hargasinski hargasinski merged commit 6927a81 into master Apr 15, 2022
@hargasinski
Copy link
Copy Markdown
Collaborator Author

I'm running into an issue with the build, looks like somewhere along the way one of babel-minify's dependencies got updated and it's now throwing an error during minification. I'll look more into it this weekend and publish v3.2.4 with the fix from this PR.

@hargasinski hargasinski deleted the promisify_priority_queue branch May 1, 2022 23:17
@hargasinski
Copy link
Copy Markdown
Collaborator Author

@aearly published v3.2.4 with this fix! Sorry for the delay, things have been busy on my end. I was hoping babel-minify v0.5.2 would fix the minification issue, but it didn't. I was able to work around it to get the minified file built and the patch published though.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants