-
Notifications
You must be signed in to change notification settings - Fork 220
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
Add tracker.getQueueName method #1217
base: master
Are you sure you want to change the base?
Conversation
BundleMonFiles added (6)
Total files change +95.43KB 0% Final result: ✅ View report in BundleMon website ➡️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a couple of thoughts I've had on the types.
I think I'm not properly following the types you've used here. I can see both:
'post' | 'get' | undefined
Exclude<EventMethod, 'beacon'>
This seems to boil down to the same type, a potentially optional 'post' | 'get'
. Could it be clearer to create a type alias for Exclude<EventMethod, 'beacon'>
, which then could be used with ?
as required?
/** | ||
* Returns the currently used queue name or if the `eventMethod` argument is provided, the queue name for the eventMethod. | ||
*/ | ||
getQueueName: (eventMethod?: 'post' | 'get' | undefined) => string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This can be eventMethod?: 'post' | 'get'
if you are using ?
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This slipped in the PR, should be Exclude<EventMethod, 'beacon'>
as in the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
What's the intended use case here vs just a "clearQueue" method or similar? Does this encourage people to mess with the queue themselves? Or is this for like a plugin API?
d27ef2d
to
f286ece
Compare
Add the
tracker.getQueueName
method.This method will return the currently used queue name or if provided arguments will return the one that would be used with the provided
eventMethod
.close #1215