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

Add tracker.getQueueName method #1217

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

igneel64
Copy link
Contributor

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

@bundlemon
Copy link

bundlemon bot commented Jun 29, 2023

BundleMon

Files added (6)
Status Path Size Limits
libraries/browser-tracker-core/dist/index.mod
ule.js
+25.15KB 26KB / +10%
trackers/javascript-tracker/dist/sp.js
+24.09KB 25KB / +10%
trackers/javascript-tracker/dist/sp.lite.js
+14.76KB 15KB / +10%
trackers/browser-tracker/dist/index.umd.min.j
s
+14.63KB 15KB / +10%
libraries/tracker-core/dist/index.module.js
+13.35KB 15KB / +10%
trackers/browser-tracker/dist/index.module.js
+3.46KB 5KB / +10%

Total files change +95.43KB 0%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history

Copy link
Contributor

@greg-el greg-el left a 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;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@jethron jethron left a 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?

libraries/browser-tracker-core/src/tracker/out_queue.ts Outdated Show resolved Hide resolved
trackers/browser-tracker/test/tracker.test.ts Outdated Show resolved Hide resolved
@igneel64 igneel64 force-pushed the feature/1215-expose-queue-name branch from d27ef2d to f286ece Compare July 3, 2023 07:16
@igneel64 igneel64 requested a review from greg-el July 3, 2023 07:19
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.

Expose localstorage queue name
3 participants