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

Refactor #85

Closed
wants to merge 1 commit into from
Closed

Refactor #85

wants to merge 1 commit into from

Conversation

tuhm1
Copy link
Contributor

@tuhm1 tuhm1 commented Jul 9, 2024

Nothing important, I just thought the code was getting a bit messy and might be hard to maintain in the future.
Please feel free to reject if you find these changes unnecessary.

@sindresorhus
Copy link
Owner

I definitely welcome simplifications. I just worry that this would introduce regressions that are not covered by our tests.

@tuhm1
Copy link
Contributor Author

tuhm1 commented Jul 12, 2024

I understand your concern. I don't think there would be any problem, but it may not be worth the risk. Take your time to consider.

@sindresorhus
Copy link
Owner

@sugoroku-y Would you be able to help review this?

Copy link
Contributor

@sugoroku-y sugoroku-y left a comment

Choose a reason for hiding this comment

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

@sugoroku-y Would you be able to help review this?

OK.

I commented where I was interested in this PullRequest.

@@ -6,58 +6,30 @@ export default function pLimit(concurrency) {
const queue = new Queue();
let activeCount = 0;

const resumeNext = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The next after the change is almost the same as the resumeNext before the change.
It seems to me that the change is easier to understand if the unchanged part is unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was named resumeNext because there had already been a next function. Now that it's gone, resumeNext might be a bit unnecessary lengthy and even inaccurate, in case we haven't even started yet.

activeCount++;
queue.dequeue()();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to change the dequeue timing and/or remove the comment?

If not, could you revert back to the original? It is easier to understand the purpose of this PullRequest if you keep it as it was.

If there is a reason, it should be noted in the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just thought it would be safer to increase activeCount before starting execution. If the function were to be executed synchronously right after being dequeued, then we would increase activeCount after it has already run. I understand that we are executing them asynchronously in this case, but I just want to be cautious.
As for removing the comment, I thought it was unnecessary as it seems quite obvious that we increase activeCount because we are starting an execution.
Does that sound okay?

const next = () => {
activeCount--;
const generator = async (function_, ...arguments_) => {
const dequeuePromise = new Promise(resolve => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for creating a Promise first?

For example, due to the queueMicrotask specification, I need to create the Promise first for it to work properly、.etc.

As far as I know, there is no problem with creating the Promise after the queueMicrotask, etc.

If you have a reason for this, please leave it in the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems more natural to enqueue first and then schedule a dequeue. The previous code does the same. Someone might wonder why, if they see queueMicrotask(next) first.

@sugoroku-y
Copy link
Contributor

I understood all the comments, but I still think that unnecessary changes make the decision to accept this pull request more difficult.

I don't see anything wrong in the code, so I will defer to Owner @sindresorhus for the final decision.

@tuhm1
Copy link
Contributor Author

tuhm1 commented Dec 20, 2024

I guess it's unnecessary. 👍

@tuhm1 tuhm1 closed this Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants