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: add job name formatter #359

Merged
merged 1 commit into from
Dec 8, 2021
Merged

feat: add job name formatter #359

merged 1 commit into from
Dec 8, 2021

Conversation

felixmosh
Copy link
Owner

closes #349

@felixmosh felixmosh merged commit 8859c76 into master Dec 8, 2021
@bugs181
Copy link

bugs181 commented Dec 8, 2021

Definitely a lot cleaner than the solution I was working on!

@bugs181
Copy link

bugs181 commented Dec 8, 2021

Whoopsie. It appears that when no formatter is available, the whole jobProps object is passed into the name property. Unless I'm missing something.

Fix for this:

const nameProp = queue.format('name', jobProps)

return {
    name: (typeof nameProp === 'string') ? nameProp : jobProps.name,
}

Not the cleanest solution, but keeps the UI from crashing on my end.

@felixmosh can you confirm?

@felixmosh
Copy link
Owner Author

Ha, you are correct :|

@bugs181
Copy link

bugs181 commented Dec 8, 2021

The above fix would also take into account of the user accidentally passing the wrong info back. The name prop will always be guaranteed to be a string.

@bugs181 bugs181 mentioned this pull request Dec 8, 2021
@felixmosh
Copy link
Owner Author

felixmosh commented Dec 8, 2021

If a user passes a formatter function, he "takes" responsibility on himself, no? ;]

If we encounter many issues on it, I will add the check :]

@bugs181
Copy link

bugs181 commented Dec 8, 2021

That is true; but sanitization and type safety is never a bad thing. Why else do we use tools like TypeScript? ;)

The only alternative I can think would be to overload format with a fallbackData option, then we would pass that data along.

return {
  name: queue.format('name', jobProps, jobProps.name)
}
public format(field: FormatterField, data: any, fallbackData: any): any {
  return typeof this.formatters[field] === 'function' ? this.formatters[field](data) : fallbackData || data;
}

@felixmosh
Copy link
Owner Author

felixmosh commented Dec 8, 2021

Yeah, I've thought about the same thing 😅
Actually, it is an easy thing to make TS check this
image
image

@bugs181
Copy link

bugs181 commented Dec 8, 2021

Fair points. I still recommend doing something other than passing the whole jobProps into the name prop. Just for future proofing code.

@felixmosh
Copy link
Owner Author

I've used your suggestion, b306477

@bugs181
Copy link

bugs181 commented Dec 8, 2021

Very nice. Thanks for all your hard work. I hope to help contribute more in the future. Hopefully without being a thorn in your side haha.

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.

Feature Request: Extract data from job to use as job Title
2 participants