Increases specificity of localStorage keys to differentiate events by writeKey #861
Conversation
🦋 Changeset detectedLatest commit: 53799f0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| @@ -86,16 +86,17 @@ function mutex(key: string, onUnlock: Function, attempt = 0): void { | |||
| } | |||
|
|
|||
| export class PersistedPriorityQueue extends PriorityQueue<Context> { | |||
| constructor(maxAttempts: number, key: string) { | |||
| constructor(maxAttempts: number, key: string, writeKey: string) { | |||
There was a problem hiding this comment.
Nit: While I think you only made this choice because EventQueue can take a configured priorityQueue, I can't help but feel there is something off about this class knowing what a writeKey is... "domain" concept seeping into the ostensibly generic "lib". It never uses the writeKey by itself. Perhaps a new "AnalyticsPersistedPriorityQueue" subclass would be better. 🤔
There was a problem hiding this comment.
Originally required it to handle the fallback case of reading keys that didn't include the writeKey, but I can remove that from this now. It did give me the benefit of having TypeScript yell at me if any usage didn't include the writekey though, but that work is done now.
|
|
||
| expect(persistedItems).toHaveLength(1) | ||
| expect(persistedItems[0].writeKey).toBe('key2') | ||
| expect(persistedItems[0].messageIds).toStrictEqual([messageId2]) |
| import { | ||
| PersistedQueueResult, | ||
| getPersistedItems, | ||
| } from './helpers/get-persisted-items' | ||
|
|
||
| test.describe('Standalone tests', () => { |
There was a problem hiding this comment.
Was this meant to be the suite name, or is this copypasta?
There was a problem hiding this comment.
Ooph, I think it was meant to signify these tests are for the standalone (CDN) version of a.js, not the NPM version. Can probably remove this describe block though.
This PR adds the
writeKeyused to configure analytics.js to thePersistedPriorityQueuelocalStorage keys.This prevents 2 analytics.js instances configured with different writeKeys but installed on the same domain (e.g.
https://foo.segment.com/app1with writeKeyfoo,https://foo.segment.com/app2with writeKeybar) from reading each others retried events from localStorage.Analytics.js saves events to localStorage when a user navigates away from a page that has events that haven't been sent to destinations/Segment.
Special Notes
writeKeyto a lot more places than I hoped.I backed those changes out because it's extra tech debt that should only be needed for about a week, and I was worried it would make this issue still possible if a page in the domain was using a version of analytics.js that persisted to the old key.
Testing
I added playwright tests that will execute the conditions that trigger events to be sent to localStorage.
[x] I've included a changeset (psst. run
yarn changeset. Read about changesets here).