♻️ detect cookie API availability when selecting session store strategy#4624
♻️ detect cookie API availability when selecting session store strategy#4624thomas-lebeau wants to merge 2 commits into
Conversation
thomas-lebeau
changed the base branch from
main
to
thomas.lebeau/e2e-latest-browsers-in-ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dac2e44ca3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| deleteCookie, | ||
| resetInitCookies, | ||
| } from './browser/cookie' | ||
| export { getCookie, getInitCookie, setCookie, deleteCookie, resetInitCookies } from './browser/cookie' |
There was a problem hiding this comment.
When downstream code imports areCookiesAuthorized from the published @datadog/browser-core package, this changed root export no longer exposes the symbol that existed before this commit, so upgrades fail with a missing export even though the helper still exists internally. If the API needs to change, keep a compatibility export/wrapper or otherwise preserve the public root surface for existing consumers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is fine, people are not supposed to use @datadog/browser-core and I did not find any other package in the dataDog github org that uses it.
thomas-lebeau
force-pushed
the
thomas.lebeau/e2e-latest-browsers-in-ci
branch
2 times, most recently
from
573ba97 to
0c3132a
Compare
- Move areCookiesAuthorized from cookie.ts to cookieAccess.ts and make it async, taking a CookieAccess factory so it can verify each API end-to-end - Split createCookieAccess into createCookieStoreAccess and createDocumentCookieAccess and export both - selectCookieStrategy now probes the CookieStore API first and falls back to document.cookie, recording the chosen cookieApi on the strategy type - Thread cookieApi through CookieSessionStoreStrategyType and initCookieStrategy so the runtime strategy uses the same API that was probed - selectSessionStoreStrategyType is async and takes Configuration (with cookieOptions/sessionPersistence) instead of InitConfiguration; sessionManager awaits it during startup - Drop areCookiesAuthorized from the public core index export
thomas-lebeau
force-pushed
the
thomas.lebeau/cookie-api-detection
branch
from
dac2e44 to
84b60e7
Compare
|
/trigger-ci |
|
View all feedbacks in Devflow UI.
Started pipeline #113522402 |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
| trackingConsentState | ||
| ) | ||
|
|
||
| // Allow startSessionManager to await selectSessionStoreStrategyType and call setSessionState |
There was a problem hiding this comment.
minor
maybe it's time to add smth like flushPromises helper to test utils? I didn't find anything similar
There was a problem hiding this comment.
you're right, looks like we have waitNextMicrotask() already, so using that.
- Replace `await Promise.resolve()` with `waitNextMicrotask()` for clarity
2 participants
Motivation
In WebKit (Safari) on
localhost-based subdomains,window.cookieStoreis exposed andcookieStore.set(...)resolves without throwing — but the cookie is silently dropped becauselocalhostis not a registrable domain per RFC 6761 (Safari rejects explicitdomain=attributes for non-registrable hosts). This breaks session persistence end-to-end on the latest WebKit in our e2e setup (which usesfoo.bar.localhost), even thoughdocument.cookiewrites still succeed.Previously, the SDK preferred the CookieStore API whenever
window.cookieStorewas present, with no end-to-end verification that a cookie actually round-trips. This change makes the session store strategy probe each API by writing and reading back a test cookie, and records the chosen API on the strategy so the runtime uses the same one that was verified during initialization.Changes
areCookiesAuthorizedintocookieAccess.tsand make it async, taking aCookieAccessfactory so it can verify each API end-to-end.createCookieAccessintocreateCookieStoreAccessandcreateDocumentCookieAccess.selectCookieStrategyprobes the CookieStore API first and falls back todocument.cookie, recording the chosencookieApion the strategy type.cookieApithroughCookieSessionStoreStrategyTypeandinitCookieStrategyso the runtime strategy uses the probed API.selectSessionStoreStrategyTypeis now async and takesConfiguration(withcookieOptions/sessionPersistence);sessionManagerawaits it during startup.areCookiesAuthorizedfrom the public core index export.yarn test:e2eto Chromium for faster feedback.Test instructions
yarn test:unit --spec packages/core/src/browser/cookieAccess.spec.tsandyarn test:unit --spec packages/core/src/domain/session/sessionStore.spec.ts.cookieStoreavailable (e.g. Chromium) that the SDK selects the cookieStore API; in WebKit on localhost, verify it falls back todocument.cookie.Checklist