Skip to content

Fix remaining object serialization callsites#6672

Open
Ankcorn wants to merge 1 commit intomainfrom
tankcorn/fix-remaining-obj-serialization
Open

Fix remaining object serialization callsites#6672
Ankcorn wants to merge 1 commit intomainfrom
tankcorn/fix-remaining-obj-serialization

Conversation

@Ankcorn
Copy link
Copy Markdown
Member

@Ankcorn Ankcorn commented Apr 26, 2026

Summary

Follow-up to #6662.

This fixes the remaining object-construction paths that were still using js.obj(keys, values) producing unserializable objects.

Tests

regression tests cover fetch and queue but not email because I couldn't find an existing e2e test harness for email

@Ankcorn Ankcorn requested review from a team as code owners April 26, 2026 21:17
@Ankcorn Ankcorn force-pushed the tankcorn/fix-remaining-obj-serialization branch from 409d390 to 0943024 Compare April 26, 2026 21:18
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 26, 2026

Good - JsObject (via JsBase) has operator JsValue() so returning a JsObject from a function that returns jsg::JsValue is fine.

Everything looks clean. This is a straightforward, correct follow-up fix. No actionable issues.

LGTM

github run

@Ankcorn Ankcorn force-pushed the tankcorn/fix-remaining-obj-serialization branch 2 times, most recently from 49ff18a to 5229fe4 Compare April 26, 2026 21:26
@fhanau
Copy link
Copy Markdown
Contributor

fhanau commented Apr 26, 2026

Thank you for doing this – I said I'd investigate the underlying issue, haven't gotten to that yet.
For the tests being added here, can we all put them in the same test file? These and the existing test at src/workerd/api/tests/stw-jsrpc-dataclone-test.wd-test are all about the same underlying issue, consolidating this instead of having separate files for each test will make maintenance easier.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 26, 2026

Merging this PR will improve performance by 40.28%

⚡ 1 improved benchmark
✅ 71 untouched benchmarks
⏩ 129 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
simpleStringBody[Response] 27.2 µs 19.4 µs +40.28%

Comparing tankcorn/fix-remaining-obj-serialization (fbc2d59) with main (d571ec1)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Ankcorn Ankcorn force-pushed the tankcorn/fix-remaining-obj-serialization branch from 5229fe4 to fbc2d59 Compare April 27, 2026 07:28
@Ankcorn
Copy link
Copy Markdown
Member Author

Ankcorn commented Apr 27, 2026

Thank you for doing this – I said I'd investigate the underlying issue, haven't gotten to that yet. For the tests being added here, can we all put them in the same test file? These and the existing test at src/workerd/api/tests/stw-jsrpc-dataclone-test.wd-test are all about the same underlying issue, consolidating this instead of having separate files for each test will make maintenance easier.

Thanks for the suggestion, I have consolidated the tests :)

@danlapid danlapid requested a review from fhanau April 27, 2026 10:58
Sign up for free to 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.

2 participants