-
-
Notifications
You must be signed in to change notification settings - Fork 245
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
Refine JSDoc / TypeScript types for plugins #324
base: master
Are you sure you want to change the base?
Conversation
@@ -289,7 +289,7 @@ function analytics(config = {}) { | |||
* @property {On} on - Fire callback on analytics lifecycle events. | |||
* @property {Once} once - Fire callback on analytics lifecycle events once. | |||
* @property {GetState} getState - Get data about user, activity, or context. | |||
* @property {Storage} storage - storage methods | |||
* @property {AnalyticsStorage} storage - storage methods |
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 conflicted with the built-in type Storage
used for localStorage
& sessionStorage
so I took the liberty of renaming it while I was in here to fix the TypeScript error I got. Hopefully that's OK.
eea7d2f
to
6b023a9
Compare
* @param {AnalyticsInstance} arg.instance analytics instance | ||
* @param {Object} arg.payload event data | ||
* @param {string} arg.payload.event event name passed to track | ||
* @param {TrackEventProperties} [arg.payload.properties] event properties passed to track |
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.
Not sure if this is actually optional. Will the library always supply an object here?
|
||
/** | ||
* Track data for overrides | ||
* @typedef {Object.<string>} TrackEventProperties |
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.
Just a generic map here since there's no predefined fields for this
6b023a9
to
95a7fa5
Compare
95a7fa5
to
1c26a40
Compare
* @property {PluginPageFunction} [page] - Page visit tracking method | ||
* @property {PluginTrackFunction} [track] - Custom event tracking method | ||
* @property {PluginIdentifyFunction} [identify] - User identify method | ||
* @property {PluginLoadedFunction} [loaded] - Function to determine if analytics script loaded | ||
* @property {PluginReadyFunction} [ready] - Fire function when plugin ready |
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.
Should there be one of these for every event named in events.js
?
Working on this it does feel like maintaining separate |
This would be a very welcome addition as all the plugin methods are typed simply |
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.
Would it make sense to add meta
, anonymousId
and userId
as well? I haven't entirely figured which are present in what methods always.
https://getanalytics.io/resources/faq/#do-i-need-to-use-a-plugin
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.
Well, maybe? Doesn't seem like this PR is getting any attention from the library maintainer so I'm not sure making any changes to it makes practical sense.
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.
Yeah, it seems like issues and PRs are increasing. I'm sure @DavidWells is just busy at the moment. Maybe it's time for a call for maintainers? 🤔
Starting working on improving the JSDoc / TypeScript a bit so that I could better understand the types here.
I'm not totally sure these are quite correct.
Also - can plugins define all the events defined in
events.js
? It seems like the JSDoc currently only lists a few events.