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.

Sign up for GitHub

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: initialize server side google analytics with multiple options #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: initialize server side google analytics with multiple options #64

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions packages/analytics-plugin-google-analytics/src/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,24 @@ if (!process.browser) {
/**
* Serverside Google Analytics plugin
* @param {object} pluginConfig - Plugin settings
* @param {string} pluginConfig.trackingId - Google Analytics site tracking Id
* @param {string} pluginConfig.trackingId - Google Analytics site tracking Id. Same as tid (se below), only one needed
* @param {string} pluginConfig.tid - Google Analytics site tracking Id. Same as trackingId (se above), only one needed
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tid property looks like a repeat of trackingId do we need that or can we just use the trackingId field?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is indeed.
I added this 4 new docummented attributes of pluginConfig but there are more that can be used in universal-analytics initialization.
I keeped the trackingId attribute in order to keep compatibility with the actual implementation and also allow to define all the attributes accepted in universal-analytics initialization.
Maybe it would be better to keep the actual jsDoc and add only a reference to all the possible attributes that can be used in pluginConfig and explaining that the trackingId attribute can be declared also as tid.

* @param {string} [pluginConfig.cid] - Google Analytics client Id. It generates a random UUID if none is set
* @param {boolean} [pluginConfig.strictCidFormat] - Set it to false to set a custom client Id (not UUID). Only used if cid is set
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like strictCidFormat is used. Do we need that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you need to set strictCidFormat to false if you want to use a non UUID clientId. It's explained in the readme file and also you can see here how it's used.

* @param {string} [pluginConfig.uid] - Google Analytics user Id
* @return {*}
* @example
*
* googleAnalytics({
* trackingId: '123-xyz'
* trackingId: '123-xyz',
* cid: '123456789.987654321',
* strictCidFormat: false
* })
*
* googleAnalytics({
* tid: '123-xyz',
* cid: '123456789.987654321',
* strictCidFormat: false
* })
*/
function googleAnalytics(pluginConfig = {}) {
Expand Down Expand Up @@ -48,8 +60,9 @@ function googleAnalytics(pluginConfig = {}) {
export default googleAnalytics

export function initialize(config) {
if (!config.trackingId) throw new Error('No google analytics trackingId defined')
return universalAnalytics(config.trackingId)
if (!config.trackingId && !config.tid) throw new Error('No google analytics trackingId defined')
if (!config.tid) config.tid = config.trackingId
return universalAnalytics(config)
}

export function pageView({ path, href, title }, client) {
Expand Down