Refactor #85
Refactor #85
Conversation
|
I definitely welcome simplifications. I just worry that this would introduce regressions that are not covered by our tests. |
|
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. |
|
@sugoroku-y Would you be able to help review this? |
There was a problem hiding this comment.
@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 = () => { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
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.