Uh oh!
There was an error while loading. Please reload this page.
chore: support typed arrays in indexeddb - #34949
Conversation
There was a problem hiding this comment.
PR Overview
This pull request adds support for typed arrays in IndexedDB storage by extending the serialization and deserialization logic, updating relevant tests, and removing outdated documentation.
- Introduces new types, functions, and handling for converting typed arrays to/from base64 strings.
- Updates tests to store and retrieve typed arrays instead of regular strings.
- Removes the documentation note that previously mentioned a lack of support for typed arrays.
Reviewed Changes
| File | Description |
|---|---|
| packages/playwright-core/src/server/isomorphic/utilityScriptSerializers.ts | Adds typed array type definitions, conversion utilities, and updates parser logic. |
| tests/library/browsercontext-storage-state.spec.ts | Modifies test cases to use typed arrays for IndexedDB operations. |
| docs/src/api/class-browsercontext.md | Removes outdated note about lack of typed array support. |
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
0133258 to
1837b1bCompare
This comment has been minimized.
This comment has been minimized.
| }; | ||
| function typedArrayToBase64(array: any) { | ||
| if (globalThis.Buffer) |
There was a problem hiding this comment.
Why have Node-specific code? It seems like btoa/atob should work both in the browser and in the Node.
There was a problem hiding this comment.
good question! I mostly copied from Pavel Feldman (@pavelfeldman)'s gist. Pavel, why?
There was a problem hiding this comment.
looks like it works just fine without: dfd44ee
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| function typedArrayToBase64(array: any) { | ||
| /** | ||
| * Firefox does not support iterating over typed arrays, so we use `.toBase64`. |
There was a problem hiding this comment.
Perhaps instead of Array.from() use a for loop? This should be supported everywhere.
There was a problem hiding this comment.
Tried that, but doesn't work. It looks like any kind of iteration is forbidden:
https://searchfox.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp#576
There was a problem hiding this comment.
What exactly does this mean? You should be able to iterate arrays and use Array.from on them?
There was a problem hiding this comment.
But not inside the utility world, apparently 🤷
| */ | ||
| if ('toBase64' in array) | ||
| return array.toBase64(); | ||
| const binary = Array.from(new Uint8Array(array.buffer)).map(b => String.fromCharCode(b)).join(''); |
There was a problem hiding this comment.
It is incorrect to serialize array.buffer without looking at array.byteOffset and array.byteLength, because underlying buffer may be longer.
This comment has been minimized.
This comment has been minimized.
90ac23a to
37f1f9fCompare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| return { u: value.toJSON() }; | ||
| if (isRegExp(value)) | ||
| return { r: { p: value.source, f: value.flags } }; | ||
| for (const [k, ctor] of Object.entries(typedArrayConstructors) as [TypedArrayKind, Function][]) { |
There was a problem hiding this comment.
I'd start with looking up constructor.name in a set instead of iterating
There was a problem hiding this comment.
that won't work on Firefox, see impl of isTypedArray
There was a problem hiding this comment.
It does work for me, are you saying this is also due to utility worlds? Something is fishy here. Do we have a bug in the way we initialize utility context? It needs to be proper JS context.
There was a problem hiding this comment.
as discussed in the meeting, firefox is just off here. let's not fix in this PR.
| function typedArrayToBase64(array: any) { | ||
| /** | ||
| * Firefox does not support iterating over typed arrays, so we use `.toBase64`. |
There was a problem hiding this comment.
What exactly does this mean? You should be able to iterate arrays and use Array.from on them?
| { ref: number } | | ||
| { h: number }; | ||
| { h: number } | | ||
| { ta: { b: string, k: TypedArrayKind } }; |
There was a problem hiding this comment.
Sounds like we need a bunch of page.evaluate tests for this.
There was a problem hiding this comment.
The tests in page-evaluate.spec.ts all go through the protocol serializer, which also doesn't yet have support. Do we test the server-side of page.evaluate in isolation?
I was planning to add support to the protocol serializer in a follow-up, happy to add page.evaluate tests in there.
There was a problem hiding this comment.
Landing this w/o coverage does not feel right, probably supporting the protocol serializer and testing e2e is the right approach.
There was a problem hiding this comment.
I've added support to the protocol serializer and a bunch of page.evaluate tests.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Test results for "tests 1"5 flaky38880 passed, 805 skipped Merge workflow run. |
Based on https://gist.github.com/pavelfeldman/4c7826893182ac9f85cc60ddbd7b2d11