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

Feature/update priority #209

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

RaishavHanspal
Copy link

@RaishavHanspal RaishavHanspal commented Jul 11, 2024

Fixes #208 - add a uid to track the promise functions and update the priority on any promise.
also add a new event started to notify when a promise function is executed.

source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
source/priority-queue.ts Outdated Show resolved Hide resolved
source/priority-queue.ts Outdated Show resolved Hide resolved
source/priority-queue.ts Outdated Show resolved Hide resolved
source/priority-queue.ts Outdated Show resolved Hide resolved
@Richienb Richienb mentioned this pull request Jul 17, 2024
source/priority-queue.ts Outdated Show resolved Hide resolved
source/priority-queue.ts Outdated Show resolved Hide resolved
@RaishavHanspal
Copy link
Author

requested changes have been pushed! @Richienb

@Richienb
Copy link
Collaborator

Richienb commented Jul 27, 2024

I think we should allow searching by the object returned by enqueue as well. What do you think? Does there remain a use case where a string id remains more useful?

@RaishavHanspal
Copy link
Author

@Richienb that would be a very nice idea, and will not require id property. The problem that I faced was enqueue accepts RunFunction - which in my understanding is a callback not accessible to the end-user.
The Idea of priority update - is for the user to be able to update the priority on the fly, currently this is achieved by a unique id assigned by user, or this can be passed using the invoke event as added in #210 incase assigned by #idAssigner.

@RaishavHanspal
Copy link
Author

RaishavHanspal commented Aug 9, 2024

@Richienb, @sindresorhus , Please suggest further.

t.deepEqual(result, ['🐌', '🐢', '🦆']);
});

test.failing('.setPriority() - with invalid "id"', async t => {
Copy link
Owner

Choose a reason for hiding this comment

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

test.failing is not for tests that are intentionally failing.

const [item] = this.#queue.splice(existingIndex, 1);
if (item === undefined) {
throw new Error('Undefined Item - No promise function of specified id available in the queue.');
}
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think these should throw. It should be allowed to set a priority without knowing beforehand whether the has already finished.

@@ -69,6 +69,7 @@ export type QueueAddOptions = {
@default 0
*/
readonly priority?: number;
id?: string;
Copy link
Owner

Choose a reason for hiding this comment

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

Doc comment.

/**
Adds a sync or async task to the queue. Always returns a promise.
*/
async add<TaskResultType>(function_: Task<TaskResultType>, options: {throwOnTimeout: true} & Exclude<EnqueueOptionsType, 'throwOnTimeout'>): Promise<TaskResultType>;
async add<TaskResultType>(function_: Task<TaskResultType>, options?: Partial<EnqueueOptionsType>): Promise<TaskResultType | void>;
async add<TaskResultType>(function_: Task<TaskResultType>, options: Partial<EnqueueOptionsType> = {}): Promise<TaskResultType | void> {
// Incase id is not defined
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// Incase id is not defined
// In case `id` is not defined.

@@ -228,12 +231,21 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT
});
}

setPriority(id: string, priority: number) {
Copy link
Owner

Choose a reason for hiding this comment

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

Doc comment

@@ -43,6 +43,9 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT

readonly #throwOnTimeout: boolean;

/** Use to assign a unique identifier to a promise function, if not explicitly specified */
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/** Use to assign a unique identifier to a promise function, if not explicitly specified */
// Used to assign a unique identifier to a promise function, if not explicitly specified.

@@ -43,6 +43,9 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT

readonly #throwOnTimeout: boolean;

/** Use to assign a unique identifier to a promise function, if not explicitly specified */
#idAssigner = 1;
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be a BigInt, to ensure it will never overflow.

Comment on lines +245 to +247
if (options.id === undefined) {
options.id = (this.#idAssigner++).toString();
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (options.id === undefined) {
options.id = (this.#idAssigner++).toString();
}
options.id ??= (this.#idAssigner++).toString();

@@ -69,6 +69,7 @@ export type QueueAddOptions = {
@default 0
*/
readonly priority?: number;
id?: string;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
id?: string;
readonly id?: string;

@sindresorhus
Copy link
Owner

Needs to be added to the readme.

I would like to see some more tests.

And the pull request needs a proper title.

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.

Is it feasible to change priority of a Queued item on the fly
3 participants