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

Jump to bottom

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

Merged
merged 1 commit into from Apr 15, 2022
Merged

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

merged 1 commit into from Apr 15, 2022

Conversation

hargasinski
Copy link
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 hargasinski mentioned this pull request Jan 24, 2022
Closed
@hargasinski
Copy link
Collaborator Author

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

aearly
aearly reviewed Jan 28, 2022
View reviewed changes
Copy link
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.

test/priorityQueue.js
@@ -31,26 +31,25 @@ describe('priorityQueue', () => {
expect(q.length()).to.equal(0);
call_order.push('callback ' + 3);
});
q.push(4, 2.9, (err, arg) => {
Copy link
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
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
Collaborator Author

@aearly is this good to go?

@hargasinski hargasinski merged commit 6927a81 into master Apr 15, 2022
@hargasinski
Copy link
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 hargasinski mentioned this pull request May 1, 2022
Closed
@hargasinski
Copy link
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
Reviewers

@aearly aearly aearly approved these changes

Assignees
No one assigned
Labels
queue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@hargasinski @aearly