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.

Sign up for GitHub

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

Refactor #85

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −42
Open

Refactor #85

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 14 additions & 42 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

const next = () => {
if (activeCount < concurrency && queue.size > 0) {
queue.dequeue()();
// Since `pendingCount` has been decreased by one, increase `activeCount` by one.
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.

queue.enqueue(resolve);
});

resumeNext();
};

const run = async (function_, resolve, arguments_) => {
const result = (async () => function_(...arguments_))();
queueMicrotask(next);

resolve(result);
await dequeuePromise;

try {
await result;
} catch {}

next();
};

const enqueue = (function_, resolve, arguments_) => {
// Queue `internalResolve` instead of the `run` function
// to preserve asynchronous context.
new Promise(internalResolve => {
queue.enqueue(internalResolve);
}).then(
run.bind(undefined, function_, resolve, arguments_),
);

(async () => {
// This function needs to wait until the next microtask before comparing
// `activeCount` to `concurrency`, because `activeCount` is updated asynchronously
// after the `internalResolve` function is dequeued and called. The comparison in the if-statement
// needs to happen asynchronously as well to get an up-to-date value for `activeCount`.
await Promise.resolve();

if (activeCount < concurrency) {
resumeNext();
}
})();
return await function_(...arguments_);
} finally {
activeCount--;
next();
}
};

const generator = (function_, ...arguments_) => new Promise(resolve => {
enqueue(function_, resolve, arguments_);
});

Object.defineProperties(generator, {
activeCount: {
get: () => activeCount,
Expand All @@ -80,7 +52,7 @@ export default function pLimit(concurrency) {
queueMicrotask(() => {
// eslint-disable-next-line no-unmodified-loop-condition
while (activeCount < concurrency && queue.size > 0) {
resumeNext();
next();
}
});
},
Expand Down