Skip to content

feat: recreate IndexedDB in storagestate - #34591

Merged
Simon Knott (Skn0tt) merged 30 commits into
microsoft:mainfrom
Skn0tt:storageState
Feb 5, 2025
Merged

feat: recreate IndexedDB in storagestate#34591
Simon Knott (Skn0tt) merged 30 commits into
microsoft:mainfrom
Skn0tt:storageState

Conversation

@Skn0tt

Copy link
Copy Markdown
Contributor

Closes#11164

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

docs/src/api/params.md:268

  • The documentation for the indexedDB field is incomplete. The 'TODO: document more' comment should be replaced with a proper description.
indexedDB ?<[Array]<[Object]>>

tests/library/browsercontext-storage-state.spec.ts:320

  • The test case should also cover scenarios where the 'indexedDB' array is empty or null.
it('should support IndexedDB', async ({ page, contextFactory }) => {

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Skn0tt
Simon Knott (Skn0tt) marked this pull request as ready for review February 5, 2025 08:43
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment threaddocs/src/auth.md Outdated
Comment threaddocs/src/api/class-browsercontext.md
Comment threadpackages/playwright-core/src/client/types.ts Outdated
Comment threadpackages/playwright-core/src/server/browserContext.ts Outdated
Comment threadpackages/protocol/src/protocol.yml Outdated

const idbResult = await Promise.all((await indexedDB.databases()).map(async dbInfo => {
if (!dbInfo.name)
throw new Error('Database name is empty');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's make sure any error clearly states that it was not able to serialize IndexedDB. Probably a big try/catch around IndexedDB code would be the best.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

i've added .catch to the two big promises.

Comment threadpackages/playwright-core/src/server/browserContext.ts Outdated
for (const store of db.stores) {
for (const record of store.records) {
if (record.key !== undefined)
record.key = JSON.stringify(utilitySerializers.serializeAsCallArgument(record.value, v => ({ fallThrough: v })));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

WDYT about including keys/values verbatim, as long as they are serializable as JSON? And if not, we can have additional keyEncoded/valueEncoded properties that will hold the serialized value in any format. This way, the format is usually readable, except for when encoding is actually required.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

makes sense to me 👍 doing this in a follow-up

Comment threadpackages/playwright-core/src/server/browserContext.ts Outdated
Comment threadpackages/playwright-core/src/server/browserContext.ts Outdated
@github-actions

This comment has been minimized.

properties:
origin: string
localStorage:
type: array

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess we can make this one optional as well.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

gonna do that in a follow-up.

Comment threadpackages/playwright-core/src/server/browserContext.ts Outdated
Comment threadpackages/playwright-core/src/server/browserContext.ts Outdated
Comment threadpackages/playwright-core/src/server/browserContext.ts Outdated
Comment threadpackages/playwright-core/src/server/browserContext.ts Outdated
Comment threadtests/library/browsercontext-storage-state.spec.ts
Comment threadpackages/playwright-core/src/server/injected/storageScript.ts Outdated
@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

10 flaky⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-trace.spec.ts:341:5 › should work behind reverse proxy @ubuntu-latest-node22-1
⚠️ [webkit-library] › tests/library/browsercontext-clearcookies.spec.ts:146:3 › should remove cookies by name and domain @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-proxy.spec.ts:27:3 › should work when passing the proxy only on the context level @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/debug-controller.spec.ts:142:1 › should reset for reuse @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/selector-generator.spec.ts:452:5 › selector generator › should not accept invalid role for candidate consideration @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/trace-viewer.spec.ts:247:1 › should render network bars @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:82:5 › click should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:107:5 › fill should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:161:5 › waitFor should not leak @webkit-ubuntu-22.04-node18

37781 passed, 655 skipped
✔️✔️✔️

Merge workflow run.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Support IndexedDB for shared auth use cases

3 participants

@Skn0tt@dgozman