-
Notifications
You must be signed in to change notification settings - Fork 136
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
Conversation
🦋 Changeset detectedLatest commit: 807d896 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 |
c9b2dcb
to
cdc8606
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cdc8606
to
f33fc25
Compare
cookieEnabled = document.cookie.includes('ajs:cookies') | ||
jar.remove('ajs:cookies') | ||
try { | ||
jar.set('ajs_cookies', 'test') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just minor comment on versioning.
Analytics would fail fatally if cookies are made inaccessible by overriding the cookie getter/setter by a piece of code. This patch adds resilience against such a case.