Skip to content

feat: support custom global analytics key#928

Merged
oscb merged 12 commits intomasterfrom
oscb/window
Sep 12, 2023
Merged

feat: support custom global analytics key#928
oscb merged 12 commits intomasterfrom
oscb/window

Conversation

@oscb
Copy link
Copy Markdown
Contributor

@oscb oscb commented Aug 15, 2023

  • I've included a changeset (psst. run yarn changeset. Read about changesets here).

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Aug 15, 2023

🦋 Changeset detected

Latest commit: 31dd1d9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@segment/analytics-next Minor
@segment/analytics-core Minor
@segment/analytics-node Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@silesky
Copy link
Copy Markdown
Contributor

silesky commented Aug 16, 2023

Hey @oscb! Thanks for taking this.

I thought the goal was to allow users to use window.foo.track() rather than window.analytics.track()? So the config option would be like 'analyticsGlobalKey', and we save that key in memory -- using it whenever we want to get the global analytics, like how getCDNUrl() works. The fact that this would also be the same reference as the buffer is just an implementation detail.

Maybe I'm missing some context?

@oscb
Copy link
Copy Markdown
Contributor Author

oscb commented Aug 17, 2023

I thought the goal was to allow users to use window.foo.track() rather than window.analytics.track()? So the config option would be like 'analyticsGlobalKey', and we save that key in memory -- using it whenever we want to get the global analytics, like how getCDNUrl() works. The fact that this would also be the same reference as the buffer is just an implementation detail.

Yes, it's sort of the idea. I think the name of the config option can be changed to analyticsGlobalKey

Per my discussion with Chris we're not yet changing the script part of it as it is going to be a challenge given that the script itself uses window.analytics but devs can modify the script and use their custom key and send it through the config (needs docs).

@oscb oscb requested a review from silesky August 23, 2023 21:00
Comment thread .changeset/mean-apricots-hammer.md Outdated
'@internal/consent-tools-integration-tests': minor
---

Adds `bufferKey` option for setting custom global window buffers
Copy link
Copy Markdown
Contributor

@silesky silesky Aug 24, 2023

Choose a reason for hiding this comment

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

Suggested change
Adds `bufferKey` option for setting custom global window buffers
Adds `globalAnalyticsKey` option for setting custom global window buffers

})
})

describe('bufferKey', () => {
Copy link
Copy Markdown
Contributor

@silesky silesky Aug 24, 2023

Choose a reason for hiding this comment

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

Suggested change
describe('bufferKey', () => {
describe('globalAnalyticsKey', () => {

@@ -0,0 +1,26 @@
import { createTaskGroup } from '../task-group'
Copy link
Copy Markdown
Contributor

@silesky silesky Aug 24, 2023

Choose a reason for hiding this comment

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

Good idea!

@silesky silesky changed the title feat: support custom global buffer key feat: support custom global analytics key Aug 25, 2023
*/
highEntropyValuesClientHints?: HighEntropyHint[]
/**
* Key for the global window property storing the buffered calls
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Key for the global window property storing the buffered calls
* When using the snippet, this is the key that points to the global analytics instance (e.g. window.analytics).

Comment thread packages/browser/src/browser/index.ts Outdated
analytics: Analytics,
buffer: PreInitMethodCallBuffer
): Promise<void> {
const calls = getGlobalAnalytics()
Copy link
Copy Markdown
Contributor

@silesky silesky Aug 25, 2023

Choose a reason for hiding this comment

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

Can we get rid of this now? i.e restore the old function signature of "popSnippetWindowBuffer"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea I guess we don't ned this anymore since we store it globally

;(window as any).analytics = (await AnalyticsBrowser.standalone(
writeKey,
options
)) as AnalyticsSnippet
Copy link
Copy Markdown
Contributor

@silesky silesky Aug 25, 2023

Choose a reason for hiding this comment

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

Could we add a setGlobalAnalyticsMethod?
Then we can do:

setGlobalAnalytics(await AnalyticsBrowser.standalone(
    writeKey,
    options
  )))

noConflict(): Analytics {
console.warn(deprecationWarning)
window.analytics = _analytics ?? this
;(window as any).analytics = _analytics ?? this
Copy link
Copy Markdown
Contributor

@silesky silesky Aug 25, 2023

Choose a reason for hiding this comment

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

We probably want to add a setGlobalAnalytics method and do something like:

setGlobalAnalytics(_analytics ?? this)

Comment thread packages/browser/src/browser/utils.ts
@oscb oscb requested a review from silesky September 8, 2023 16:40
Copy link
Copy Markdown
Contributor

@silesky silesky left a comment

Choose a reason for hiding this comment

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

Small late breaking feedback just noticed!

Comment thread packages/browser/src/browser/utils.ts Outdated
@@ -0,0 +1,31 @@
import { AnalyticsSnippet } from './standalone-interface'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since file has import side effects -- I think we should name it 'global-analytics-helpers.ts ' or something like that, so nobody puts anything else in it. It could also go in /lib

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. It stopped being only an utils file


/**
* Gets the global analytics instance/buffer
* @param key name of the window property where the buffer is stored (default: analytics)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we change the comment to say "global analytics/buffer"

@oscb oscb requested a review from silesky September 8, 2023 22:27
Comment thread .changeset/mean-apricots-hammer.md Outdated
@@ -0,0 +1,6 @@
---
'@segment/analytics-next': minor
'@internal/consent-tools-integration-tests': minor
Copy link
Copy Markdown
Contributor

@silesky silesky Sep 9, 2023

Choose a reason for hiding this comment

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

Oops... this bump looks like a mistake

Copy link
Copy Markdown
Contributor

@silesky silesky left a comment

Choose a reason for hiding this comment

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

Some other stuff ;-)

@@ -44,7 +44,7 @@ const WebWorker: React.FC = () => {
}}
value="Track!"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's actually fine to do:

declare global {
  interface Window {
    analytics: AnalyticsSnippet
  }
}

... since this is not a published repo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll leave it as it is for now. But yeah, doesn't matter.

@oscb oscb requested a review from silesky September 12, 2023 18:19
@oscb oscb merged commit 7f4232c into master Sep 12, 2023
@oscb oscb deleted the oscb/window branch September 12, 2023 18:56
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.

2 participants