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.

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

[LIBWEB-1353] Fix cookie write error #891

Merged
merged 3 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 5 additions & 0 deletions .changeset/pink-dancers-exist.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@segment/analytics-next': minor
zikaari marked this conversation as resolved.
Show resolved Hide resolved
---

[LIBWEB-1353] Fix cookie write error
24 changes: 19 additions & 5 deletions packages/browser/src/core/user/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ function clear(): void {
localStorage.clear()
}

/**
* Filters out the calls made for probing cookie availability
*/
const ignoreProbeCookieWrites = (
fn: jest.SpyInstance<
string | undefined,
[
name: string,
value: string | object,
options?: jar.CookieAttributes | undefined
]
>
) => fn.mock.calls.filter((c) => c[0] !== 'ajs_cookies')

let store: LocalStorage
beforeEach(function () {
store = new LocalStorage()
Expand Down Expand Up @@ -58,7 +72,7 @@ describe('user', () => {
assert(user.anonymousId()?.length === 36)
expect(jar.get('ajs_anonymous_id')).toBeUndefined()
expect(localStorage.getItem('ajs_anonymous_id')).toBeNull()
expect(setCookieSpy.mock.calls.length).toBe(0)
expect(ignoreProbeCookieWrites(setCookieSpy).length).toBe(0)
})

it('should not overwrite anonymous id', () => {
Expand Down Expand Up @@ -218,7 +232,7 @@ describe('user', () => {
user.id('foo')

assert(user.anonymousId() === prev)
expect(setCookieSpy.mock.calls.length).toBe(0)
expect(ignoreProbeCookieWrites(setCookieSpy).length).toBe(0)
})

it('should reset anonymousId if the user id changed', () => {
Expand All @@ -227,7 +241,7 @@ describe('user', () => {
user.id('baz')
assert(user.anonymousId() !== prev)
assert(user.anonymousId()?.length === 36)
expect(setCookieSpy.mock.calls.length).toBe(0)
expect(ignoreProbeCookieWrites(setCookieSpy).length).toBe(0)
})

it('should not reset anonymousId if the user id changed to null', () => {
Expand All @@ -236,7 +250,7 @@ describe('user', () => {
user.id(null)
assert(user.anonymousId() === prev)
assert(user.anonymousId()?.length === 36)
expect(setCookieSpy.mock.calls.length).toBe(0)
expect(ignoreProbeCookieWrites(setCookieSpy).length).toBe(0)
})
})

Expand Down Expand Up @@ -795,7 +809,7 @@ describe('group', () => {
expect(group.id()).toBe('gid')
expect(jar.get('ajs_group_id')).toBeFalsy()
expect(store.get('ajs_group_id')).toBeFalsy()
expect(setCookieSpy.mock.calls.length).toBe(0)
expect(ignoreProbeCookieWrites(setCookieSpy).length).toBe(0)
})

it('behaves the same as user', () => {
Expand Down
15 changes: 15 additions & 0 deletions packages/browser/src/core/user/__tests__/storage.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { Cookie } from '..'

describe('Cookie storage', () => {
it('should report cookie storage available when cookies are accessible', () => {
expect(Cookie.available()).toBe(true)
})

it('should report cookie storage unavailable when cookies are not accessible', () => {
;(document as any).__defineGetter__('cookie', function () {
return ''
})

expect(Cookie.available()).toBe(false)
})
})
15 changes: 7 additions & 8 deletions packages/browser/src/core/user/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,14 @@ const ONE_YEAR = 365

export class Cookie extends Store {
static available(): boolean {
let cookieEnabled = window.navigator.cookieEnabled

if (!cookieEnabled) {
jar.set('ajs:cookies', 'test')
cookieEnabled = document.cookie.includes('ajs:cookies')
jar.remove('ajs:cookies')
try {
jar.set('ajs_cookies', 'test')
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for quick reuse putting ajs_cookies as a const AJS_COOKIE_AVAILABILITY_KEY or similar.

I would also suggest renaming it to ajs_cookies_check or something to highight that it is only an availability check cookie.

Copy link
Contributor

@silesky silesky Jul 14, 2023

Choose a reason for hiding this comment

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

good point! Also, all of these strings get repeated in the bundle -- using a constant saves bytes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

const cookieEnabled = document.cookie.includes('ajs_cookies')
jar.remove('ajs_cookies')
return cookieEnabled
} catch (error) {
return false
}

return cookieEnabled
}

static get defaults(): CookieOptions {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,20 @@ import { SegmentEvent } from '../../../core/events'
import { JSDOM } from 'jsdom'
import { version } from '../../../generated/version'

/**
* Filters out the calls made for probing cookie availability
*/
const ignoreProbeCookieWrites = (
fn: jest.SpyInstance<
string | undefined,
[
name: string,
value: string | object,
options?: cookie.CookieAttributes | undefined
]
>
) => fn.mock.calls.filter((c) => c[0] !== 'ajs_cookies')

describe('before loading', () => {
let jsdom: JSDOM

Expand Down Expand Up @@ -317,7 +331,7 @@ describe('before loading', () => {
expect(object.context.referrer.id).toEqual('medium')
assert(object.context.referrer.type === 'millennial-media')
expect(cookie.get('s:context.referrer')).toBeUndefined()
expect(setCookieSpy).not.toHaveBeenCalled()
expect(ignoreProbeCookieWrites(setCookieSpy).length).toBe(0)
})

it('should add .referrer.id and .referrer.type from cookie', () => {
Expand Down