feat(storageState): handle ArrayBuffer - #38923
Conversation
|
For the tests, I'm planning on adding data to playwright/tests/library/browsercontext-storage-state.spec.ts Lines 380 to 385 in 71c4240 Do i edit to-do-notifications/scripts/todo.js ? seems a bit heavy Or do I go with a |
|
That's exactly what I did in 311625b#diff-303a7e6f772039cc6ffb24ee18c55a69c7f42a9c1956ec8c53985f4d2faa9397 - go ahead and update the index.html file, yeah. When you use page.evaluate, you'll be testing the serialisation for page.evaluate on top of the storagestate one. |
I was thinking like, filling another indexeddb store within a page.evaluate() call but alright, i'll add something to the todo app, but tbf idk what to add that would warrant an ArrayBuffer '^^ |
|
it doesn't have to make sense, just take one of the strings and turn it into an array buffer before serialising |
|
do I add a second todo item? because the trivial case is not there anymore since theres an array buffer (the record becomes a valueEncoded instead of a value) |
|
you can add a "task description" column that's stored as an arraybuffer. I'm sure Copilot will write you some nice code for it :) |
046c833 to
67735a3
Compare
|
Simon Knott (@Skn0tt) sorry didnt see your comment, i ended up storing a binaryTitle column that's just the utf8 bytes of the title, but its pretty much the same thing :) although i guess we could add a proper separate description column and put in larger text, with some special chars to check for mojibake if the base64 encoding somehow breaks |
|
the test passes in all browsers on my machine! |
Simon Knott (Skn0tt)
left a comment
There was a problem hiding this comment.
Looks good! Left some minor feedback. I'll kick off CI.
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
please remove the unintended whitespace formatting changes
| { h: number } | | ||
| { ta: { b: string, k: TypedArrayKind } }; | ||
| { ta: { b: string, k: TypedArrayKind } } | | ||
| { ba: { b: string } }; |
There was a problem hiding this comment.
let's call it ab - it's not an BrrayAuffer!
| { k: 'month', v: 'January' }, | ||
| { k: 'year', v: '2025' }, | ||
| { k: 'notified', v: 'no' }, | ||
| { v: { ba: { b: 'UGV0IHRoZSBjYXQ=' } }, k: 'binaryTitle' } |
There was a problem hiding this comment.
nit: swap k and v for easy mental parsing when glancing over this code
There was a problem hiding this comment.
the test failure diff showed them in this order, but i guess key order is not checked, ill swap them around yeh
|
if you feel like contributing more, I think this comment can be resolved! Float16 is in baseline now. https://github.com/gwennlbh/playwright/blob/67735a3afba771684ea09444d4f2a91ed41ed77b/packages/playwright-core/src/utils/isomorphic/utilityScriptSerializers.ts#L98-L99 |
67735a3 to
2b22004
Compare
don't worry, we're here to test the serializer, not the impl of |
|
eslint is complaining, please take a look |
|
Simon Knott (@Skn0tt) yeah i saw, im waiting for the tests to finish before pushing again i also added the Float16Array, do i make a separate PR for it or just another commit is fine? |
|
Please make a separate PR, then we can review it separately ahead of the next release. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2b22004 to
76ea4c4
Compare
|
lint should be fixed, idk about the MCP tests either |
Test results for "MCP"12 failed 3094 passed, 300 skipped Merge workflow run. |
Test results for "tests 1"5 failed 4 flaky34664 passed, 695 skipped Merge workflow run. |
Closes #38915