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

feat: return task id at enqueue and dequeue #654

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

Conversation

manuman94
Copy link

@manuman94 manuman94 commented Jan 5, 2022

Hi! I needed the task ID from outside the queue to keep control of tasks sent to the queue.

This is my approach to get it.

Thanks!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 5, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 48654ec:

Sandbox Source
Vanilla Configuration
queue-promise-sandbox-manual Configuration
queue-promise-sandbox-automatic Configuration

@manuman94 manuman94 changed the title [ADDED] ~minor ✨ Return task id at enqueue and dequeue feat: return task id at enqueue and dequeue Jan 5, 2022
@manuman94 manuman94 marked this pull request as ready for review January 5, 2022 11:52
@manuman94 manuman94 marked this pull request as draft January 5, 2022 12:09
@manuman94 manuman94 marked this pull request as ready for review January 5, 2022 14:44
@Bartozzz
Copy link
Owner

Bartozzz commented Jan 8, 2022

Hey @manuman94, thanks for your contribution. Could you elaborate a little bit more on your use case and how those changes solve your problem?

@manuman94
Copy link
Author

manuman94 commented Jan 8, 2022

Hi!

Yes! We are currently developing a Jasmine reporter that sends information to a custom made API (spec information, http requests done, screenshots).

Before using queue-promise, this API calls were blocking the actual tests, now using queue-promise we are parallelizing those calls.

By getting the task ID from outside the queue we know which async tasks are already pending and which of them are completed. We show information about that while the process is being executed.

If one task fail, we know which task is it, and show information about what are we sending and the error returned by the server.

That's our use case, but we find useful to keep control of the status of the tasks queued in the promise queue.

Thank you, greetings.

Side note: sorry about closing pull request, Github doesn't ask for confirmation in any of their actions. I'm very sorry about the confusion. (I'm writing this in my mobile phone, where is easy to miss click buttons)

@manuman94 manuman94 closed this Jan 8, 2022
@manuman94 manuman94 reopened this Jan 8, 2022
@manuman94
Copy link
Author

Hi, is there any problem with the PR? I can solve whatever thing you need.

Greetings.

@Bartozzz
Copy link
Owner

Bartozzz commented Jan 18, 2022

Hey @manuman94, first of all thanks for your contribution! I am not sure if this should be a part of this repository. I would recommend you create your own ID/progress system. Maybe something similar to the code below would work for your case:

// Simulate a job
function job(shouldSuccess = true) {
  return new Promise((resolve, reject) => {
    if (shouldSuccess) {
      setTimeout(() => resolve(`Success`), 500)
    } else {
      setTimeout(() => reject(`Failure`), 500)
    }
  });
}

// Wrap the job with an ID
function executeJob(id, callback) {
  return async () => {
    try {
      const response = await callback();

      return { success: true, id, response }
    } catch (error) {
      // Maybe you can even re-throw here this object literal to keep the
      // ID and be able to handle errors in the reject event listener
      return { success: false, id, response: error }
    }
  }
}

const queue = new Queue();

queue.on("resolve", ({ success, id, response }) => {
  if (success) {
    console.debug(`Resolved task with id ${id}`);
  } else {
    console.error(`Rejected task with id ${id}`);
  }
});

queue.enqueue(executeJob('job-1', () => job(true)));
queue.enqueue(executeJob('job-2', () => job(true)));
queue.enqueue(executeJob('job-3', () => job(true)));
queue.enqueue(executeJob('job-4', () => job(false)));
Output
Resolved task with id job-1 
Resolved task with id job-2 
Resolved task with id job-3 
Rejected task with id job-4

My main concern is the inconsistent API design between the methods and the equivalent events. The enqueue method returns the enqueued tasks ID, but the information is lost when calling dequeue. One has to implement an event listener for the dequeue event and implement his own logic for matching the tasks and dequeued tasks IDs.

Another concern that I have is the fact that uniqueId is basically a counter, incrementing as you enqueue new tasks. There's no strong relation between the task and the task ID. Just changing the order of enqueue calls will change the task ID associated with the task. Because of that, as your tasks resolve, you still need to implement a mechanism to match the tasks and the tasks IDs somehow anyway.

Using the code snippet I posted above, the task ID is predictable and can be easily associated with the task itself. Let me know your thoughts on this.

@manuman94
Copy link
Author

Hi @Bartozzz , thanks for your response!

I appreciate your purpose, but my thought is that the queue library should be self sufficient and keep control of the status of the async tasks from outside is something inherent to the library functions.

Maybe my approach is not the best, as I agree with all your concerns. But I think the library should offer a way to keep tracking of the promises queued.

But that's my opinion! I find your comments correct, so I understand this library is going to another way.

Thank you so much for answering and for this awesome library. Greetings.

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.

2 participants