Support loading analytics into a custom global variable when using snippet version 5.2.1 or later #1032

zikaari · 2024-01-26T21:44:12Z

Fixes #1014

Please refer to the original issue for more information

Docs for this feature - segmentio/segment-docs#5949

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

changeset-bot · 2024-01-26T21:44:16Z

🦋 Changeset detected

Latest commit: d6caafa

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

This PR includes changesets to release 1 package

Name	Type
@segment/analytics-next	Minor

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 · 2024-01-26T22:05:07Z

packages/browser-integration-tests/standalone-custom-key.html

@@ -0,0 +1,97 @@
+<!DOCTYPE html>


Following the pattern of the other integration tests -- should we move this file to standalone-playground?

This is as per how @chrisradek advised me to setup an integration test

silesky · 2024-01-26T22:10:14Z

packages/browser-integration-tests/standalone-custom-key.html

+              t.src =
+                'https://cdn.segment.com/analytics.js/v1/' +
+                key +
+                '/analytics.min.js'


Is this testing against the bundle that lives in the monorepo? This is loading the cdn bundle, which doesn't have the updated code AFAIK. Kind of wondering how the tests are passing.

(It's be even cleaner IMO to use the segment snippet npm package with new option).

See the other browser integration test's html for example FYI of how they are symlinked to this bundle.

Yes, after it's been built

@zikaari I am still a bit confused -- to rephrase:

If this is loading analytics from the CDN, wouldn't this be currently testing against the version of analytics that doesn't have the changes reflected in this PR (i.e if (globalAnalyticsKey))?

What am I missing? I would expect that the link to look more like https://github.com/segmentio/analytics-next/blob/master/playgrounds/standalone-playground/pages/index-local.html#L79

This does the redirection -

analytics-next/packages/browser-integration-tests/src/helpers/standalone-mock.ts

Lines 18 to 33 in d4f6975

await context.route(

'https://cdn.segment.com/analytics.js/v1/*/analytics.min.js',

(route, request) => {

if (request.method().toLowerCase() !== 'get') {

return route.continue()

}

return route.fulfill({

path: joinPath(ajsBasePath, 'standalone.js'),

status: 200,

headers: {

'Content-Type': 'text/javascript',

},

})

}

)

ohhhh... I missed that detail! That's excellent!

silesky · 2024-01-26T22:14:28Z

packages/browser-integration-tests/standalone-custom-key.html

+            analytics.SNIPPET_VERSION = '5.2.0'
+
+            // will be used for asserting in the corresponding test
+            analytics.track('track from snippet')


a bit confused by this part? Can we make this snippet vanilla via analytics.page, and use page.evaluate to run other code as usual?

Would be even better if this snippet was generated from the npm package, and we can even use route.fulfill to link to the current umd.

analytics here is a reference to window['segment_analytics']

Agree with what Seth is saying - this page should just be as vanilla a snippet as can be - we can make the actual track call in the test file.

Also, does the new snippet always set window.analytics even if you're using a custom global key? I think that would cause a problem for users reserving window.analytics for something else. Wouldn't tests using this continue to pass without your a.js changes because window.analytics still exists with this snippet?

this page should just be as vanilla a snippet as can be - we can make the actual track call in the test file.

During our meeting we talked about testing if buffered calls (initiated in the snippet, before the main bundle loads) are taken care of safely - making those calls in the test file won't assert what we're trying to assert

does the new snippet always set window.analytics even if you're using a custom global key

No, it does not. Where did you notice a read or write to window.analytics?

No, it does not. Where did you notice a read or write to window.analytics?

~~On the line that's highlighted - analytics.track('track from snippet')~~

Ah, I may have misread this - apologies and disregard this part then!

making those calls in the test file won't assert what we're trying to assert

Couldn't we make a track call, call custom_analytics.load('writekey', { globalAnalyticsKey: 'custom_analytics' }), and verify we see a network request for the track call we generated?

Yes you guys are right, its not that we are testing if calls in snippet get dealt with - its the calls before load, and your proposal will cover that. Let me change that

Done - fc0be16

silesky · 2024-01-26T22:18:01Z

@chrisradek should we get it out of the way and link to our segment doc update PR here, so it doesn't get lost in the fray?

silesky · 2024-01-26T22:19:20Z

packages/browser/src/browser/standalone.ts

+)?.dataset.globalSegmentAnalyticsKey
+
+if (globalAnalyticsKey) {
+  setGlobalAnalyticsKey(globalAnalyticsKey)


This was a good idea to live here, so it doesn't get included in the npm bundle.

chrisradek

I like using the browser integration tests since it means we can test that the actual track calls work on the new global - I had some questions I posted in the PR - is there a docs PR for this change as well? I think we'll have to be very clear on how the snippet has to be modified to work with the globalAnalyticsKey configuration - based on the changelog I wouldn't expect that I'd need a new snippet to make this work.

zikaari · 2024-01-29T22:22:24Z

My docs PR was 1:1 replica of Seth's PR. Where do I explain the usage of this feature?

chrisradek · 2024-01-29T22:30:56Z

packages/browser-integration-tests/src/custom-global-key.test.ts

+    // Load analytics.js
+    await page.goto('/standalone-custom-key.html')
+    await page.evaluate(() =>
+      (window as any).segment_analytics.load('fake-key')


Ah, so if a custom key is used, we don't actually have to set globalAnalyticsKey? That's nice! (I'm also kind of wondering now what use we get out of the globalAnalyticsKey...)

Yes, it serves no purpose for snippet users. Unless it serves a utility for NPM users, it is useless.

chrisradek · 2024-01-29T22:39:00Z

My docs PR was 1:1 replica of segmentio/segment-docs#5754. Where do I explain the usage of this feature?

Do we currently explain anywhere in the docs that you can change the variable used to install analytics? I originally thought you also had to use the globalAnalyticsKey with this feature - but looking at your test looks like that's probably not the case. So might make sense to add another section to Segment Docs on using something other than window.analytics for snippet users.

chrisradek · 2024-01-29T22:41:37Z

.changeset/kind-crabs-report.md

+'@internal/browser-integration-tests': patch
+---
+
+Fix globalAnalyticsKey feature


Suggested change

Fix globalAnalyticsKey feature

Support loading analytics into a custom global variable when using snippet version [X.X.X]+

Done - 631923d

zikaari · 2024-01-29T22:41:44Z

Will add a section to the javascript, what do you think should the section be titled? "Installing Analytics under custom namespace"?

chrisradek · 2024-01-29T22:51:09Z

"Installing Analytics under custom namespace"

I think something like that is fine. We can always update it as well, just want to make sure we are documenting our changes - we've not always been great about that historically.

zikaari · 2024-01-30T20:40:56Z

Docs are here - segmentio/segment-docs#5949

silesky · 2024-01-30T23:11:45Z

.changeset/kind-crabs-report.md

@@ -0,0 +1,6 @@
+---
+'@segment/analytics-next': patch
+'@internal/browser-integration-tests': patch


This should not really be bumped. It doesn't matter, but we should leave this off the changelog.

silesky · 2024-01-30T23:13:09Z

.changeset/kind-crabs-report.md

@@ -0,0 +1,6 @@
+---
+'@segment/analytics-next': patch


I would change this to minor, as this is a new feature IMO (even though it does kind of have a fix element)

Fix globalAnalyticsKey not working

a1012fb

silesky reviewed Jan 26, 2024

View reviewed changes

silesky closed this Jan 26, 2024

silesky reopened this Jan 26, 2024

silesky reviewed Jan 26, 2024

View reviewed changes

chrisradek suggested changes Jan 29, 2024

View reviewed changes

chrisradek reviewed Jan 29, 2024

View reviewed changes

update test

fc0be16

chrisradek reviewed Jan 29, 2024

View reviewed changes

Update changeset description

631923d

zikaari changed the title ~~Fix globalAnalyticsKey not working~~ Support loading analytics into a custom global variable when using snippet version 5.2.1 or later Jan 29, 2024

zikaari requested review from chrisradek and silesky January 29, 2024 22:44

silesky approved these changes Jan 30, 2024

View reviewed changes

silesky reviewed Jan 30, 2024

View reviewed changes

chrisradek approved these changes Jan 30, 2024

View reviewed changes

silesky reviewed Jan 30, 2024

View reviewed changes

changelog update

d6caafa

zikaari merged commit 5c1511f into master Feb 1, 2024
4 checks passed

zikaari deleted the global_key_fix branch February 1, 2024 16:51

github-actions bot mentioned this pull request Feb 1, 2024

Version Packages #1033

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support loading analytics into a custom global variable when using snippet version 5.2.1 or later #1032

Support loading analytics into a custom global variable when using snippet version 5.2.1 or later #1032

zikaari commented Jan 26, 2024 •

edited

Loading

changeset-bot bot commented Jan 26, 2024 •

edited

Loading

silesky Jan 26, 2024 •

edited

Loading

zikaari Jan 27, 2024

silesky Jan 26, 2024 •

edited

Loading

zikaari Jan 27, 2024

silesky Jan 30, 2024 •

edited

Loading

zikaari Jan 30, 2024

silesky Jan 30, 2024 •

edited

Loading

silesky Jan 26, 2024 •

edited

Loading

zikaari Jan 27, 2024

chrisradek Jan 29, 2024

zikaari Jan 29, 2024

chrisradek Jan 29, 2024 •

edited

Loading

zikaari Jan 29, 2024

zikaari Jan 29, 2024

silesky commented Jan 26, 2024 •

edited

Loading

silesky Jan 26, 2024

chrisradek left a comment

zikaari commented Jan 29, 2024

chrisradek Jan 29, 2024 •

edited

Loading

zikaari Jan 29, 2024

chrisradek commented Jan 29, 2024

chrisradek Jan 29, 2024

zikaari Jan 29, 2024

zikaari commented Jan 29, 2024

chrisradek commented Jan 29, 2024

zikaari commented Jan 30, 2024

silesky Jan 30, 2024 •

edited

Loading

silesky Jan 30, 2024 •

edited

Loading

	await context.route(
	'https://cdn.segment.com/analytics.js/v1/*/analytics.min.js',
	(route, request) => {
	if (request.method().toLowerCase() !== 'get') {
	return route.continue()
	}

	return route.fulfill({
	path: joinPath(ajsBasePath, 'standalone.js'),
	status: 200,
	headers: {
	'Content-Type': 'text/javascript',
	},
	})
	}
	)

	Fix globalAnalyticsKey feature
	Support loading analytics into a custom global variable when using snippet version [X.X.X]+

Support loading analytics into a custom global variable when using snippet version 5.2.1 or later #1032

Support loading analytics into a custom global variable when using snippet version 5.2.1 or later #1032

Conversation

zikaari commented Jan 26, 2024 • edited Loading

changeset-bot bot commented Jan 26, 2024 • edited Loading

🦋 Changeset detected

silesky Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

silesky Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisradek Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky commented Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

chrisradek left a comment

Choose a reason for hiding this comment

zikaari commented Jan 29, 2024

chrisradek Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisradek commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zikaari commented Jan 29, 2024

chrisradek commented Jan 29, 2024

zikaari commented Jan 30, 2024

silesky Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

silesky Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

zikaari commented Jan 26, 2024 •

edited

Loading

changeset-bot bot commented Jan 26, 2024 •

edited

Loading

silesky Jan 26, 2024 •

edited

Loading

silesky Jan 26, 2024 •

edited

Loading

silesky Jan 30, 2024 •

edited

Loading

silesky Jan 30, 2024 •

edited

Loading

silesky Jan 26, 2024 •

edited

Loading

chrisradek Jan 29, 2024 •

edited

Loading

silesky commented Jan 26, 2024 •

edited

Loading

chrisradek Jan 29, 2024 •

edited

Loading

silesky Jan 30, 2024 •

edited

Loading

silesky Jan 30, 2024 •

edited

Loading