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

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

Merged
merged 1 commit into from Apr 15, 2022

Conversation

hargasinski
Copy link
Collaborator

@hargasinski hargasinski commented Jan 24, 2022

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
Collaborator Author

hargasinski commented Jan 24, 2022

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

Copy link
Collaborator

@aearly aearly left a comment

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

@@ -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

@aearly aearly Jan 28, 2022

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

@hargasinski hargasinski Feb 2, 2022

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

hargasinski commented Apr 13, 2022

@aearly is this good to go?

aearly
aearly approved these changes Apr 15, 2022
@hargasinski hargasinski merged commit 6927a81 into master Apr 15, 2022
30 of 32 checks passed
@hargasinski
Copy link
Collaborator Author

hargasinski commented Apr 15, 2022

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
@hargasinski
Copy link
Collaborator Author

hargasinski commented Jun 7, 2022

@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.

None yet

2 participants