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

Job name is taked from inside job data #241

Closed
kikar opened this issue Sep 4, 2020 · 8 comments · Fixed by #242
Closed

Job name is taked from inside job data #241

kikar opened this issue Sep 4, 2020 · 8 comments · Fixed by #242
Assignees
Milestone

Comments

@kikar
Copy link
Contributor

kikar commented Sep 4, 2020

Hi, I have a queue where the data contains a name property, and that name is used in the job list instead of the job name.

Is there a reason for this?

You can see it happening here

@eeVoskos
Copy link
Contributor

This behaviour is intentional, and was added in #74.

Do you have a better suggestion that covers this use case?

@kikar
Copy link
Contributor Author

kikar commented Sep 21, 2020

The behaviour is not documented, so now I have all funky names in the list, rather than just using the Bull name. This prevents people to use the name key in the job's data.

Bull has a job name already, why do you need to fetch it from the job?

@eeVoskos
Copy link
Contributor

Well, I believe this dates back to the time that bull didn't have a job name property, and we had to add one in the job's data.

Perhaps, a more appropriate change would be to give priority to using job.name in the UI, and only use job.data.name if the former is undefined.

So, try to replace this:

{{#if this.data.name}}
  <span style="margin-left: 8px">{{ this.data.name }}</span>
{{else if this.name}}
  <span style="margin-left: 8px">{{ this.name }}</span>
{{/if}}

with this:

{{#if this.name}}
  <span style="margin-left: 8px">{{ this.name }}</span>
{{else if this.data.name}}
  <span style="margin-left: 8px">{{ this.data.name }}</span>
{{/if}}

Disclaimer: I haven't tested the above code change, it's just a suggestion that may make the required change more backwards compatible.

@kikar
Copy link
Contributor Author

kikar commented Sep 21, 2020

Thank you for the suggestion @eeVoskos. I've updated the PR: #242

@eeVoskos
Copy link
Contributor

eeVoskos commented Oct 5, 2020

So, I just checked this issue and PR. I have two comments:

  1. Yes, Bull has an optional name field in queue.add(name?: string, data: object, opts?: JobOpts). However, this actually refers to the job type (see docs here), meaning that the core purpose of the name arg is the use of named processors (see doc here).
    If your suggested change is accepted, all names in the job list UI will be replaced with the processor's name. This contradicts the idea behind the introduction of job.data.name in the first place, which was to bypass this Bull's known behaviour, and giving the devs the power to have different explicit names per job.

  2. Yes, the use of job.data.name is not documented and could make a mess for someone who doesn't know about this behaviour.

As Arena is supposed to also work without Bull, and Bull of course without Arena, my suggestion is:

  • Introduce an internal __name field in the job data for Arena users, that will replace the use of job.data.name field.
  • Use the fields in Arena's job list, with the following priority: job.data.__name > job.name > job.data.name (keeping this for compatibility).
  • Document this feature in Arena's README.
  • Note this change in the CHANGELOG to make sure devs are not affected in the future by this behaviour change.

What do you think @kikar?

@kikar
Copy link
Contributor Author

kikar commented Oct 5, 2020

That's a great solution!
I'm going to update my PR, but could you give me a hand updating the README and the CHANGELOG @eeVoskos ?

@eeVoskos
Copy link
Contributor

eeVoskos commented Oct 5, 2020

Well, both the README and the CHANGELOG are files inside this repo, so, you could just edit them in the same PR, or perhaps ask one of the project maintainers - I'm not one of them, just an opinionated dev :-). I'd add something like this:

README.md

Add a new section, after the Custom configuration file section.

#### Custom names in the job list

Bull users can bypass the default job name (`job.name`), originally used to define a named job processor, by using a `__name` property in the job's data. E.g.

let data = {
  "__name": "This name will show up in list",
  "foo": "bar"
}
bull.queue.add(jobType, data)`

If `__name` is not specified, the `jobType` will be used instead.

CHANGELOG.md

Add a new top section (above 3.2.2) for unreleased changes.

### Unreleased

### Features

- **Job name:** Bull users can use a `__name` field in `job.data`, that will be used to replace the job's default name in the job list UI.

@skeggse
Copy link
Member

skeggse commented Oct 28, 2020

I'd advise against updating the CHANGELOG file, as it's automatically generated/updated by our release tooling. Based on conventionalcommits, you can add information to commits that'll be included in the changelog.


Perhaps another solution here, because there don't seem to be any standards around the naming of jobs in either queue library (certainly not in bee-queue, anyway), would be to allow the library user to provide a function that determines what the job's name should be given the job object itself?

@skeggse skeggse linked a pull request Nov 21, 2020 that will close this issue
@skeggse skeggse closed this as completed Nov 21, 2020
@skeggse skeggse assigned skeggse and kikar and unassigned skeggse Nov 21, 2020
@skeggse skeggse added this to the 3.5.0 milestone Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants