expose ctx.version to workers with the enable_version_api compat flag#6283
expose ctx.version to workers with the enable_version_api compat flag#6283
ctx.version to workers with the enable_version_api compat flag#6283Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR threads Worker::VersionInfo through the entrypoint/handler/ExecutionContext pipeline and exposes it as ctx.version behind the experimental enable_version_api compat flag.
Findings (highest severity first):
- Missing test coverage: No test exercises
ctx.versionbeing populated with real data. The existing server-test (loopback binding calls accept version property) passeskj::noneforversionInfoat the server level and tests the loopback binding, notctx.versioncontent. Consider adding a test that provides aVersionInfowith realid/cohort/keyvalues and asserts they appear correctly onctx.version. - Mutable version object:
toJs()returns a plain unfrozen JS object — user code can mutatectx.version.idetc. Consider freezing it. JSG_TS_OVERRIDEcombinatorial explosion: 4 branches with duplicated version type. The TODO acknowledges this but duplicating the 4-field version type is fragile.- Pre-existing:
exportsnot traced invisitForGc: Not introduced by this PR but you're modifying this class —exportsis aJsRefthat should be visited.
This review was generated by an AI assistant and may contain inaccuracies.
|
Review posted successfully on PR #6283. Here's a summary of the findings:
|
|
The generated output of |
Merging this PR will not alter performance
Comparing Footnotes
|
86fe0f7 to
8d63505
Compare
8d63505 to
fe04923
Compare
CustomEvents do not currently pass versionInfo to their entrypoints.
fe04923 to
6cf8ef5
Compare
When a worker with the experimental
enable_version_apicompat flag has aversionInfopassed while its entrypoint is being fetched,ctx.versionwill be populated.ctx.versionis currently optional even when theenable_version_apicompat flag is enabled because we have some cases where it might not be populated (dynamic workers, the ctx object created for thenonclass_entrypoint_reuses_ctx_across_invocationscompat flag, and the singleenable_version_apitest in server-test.c++).Similar to #6111, there's not much testing here. We haven't exposed a way to hit all these codepaths directly from workerd so we're relying on tests from the internal codebase right now. This can change once we expose
VersionInfotoWorkerdDebugPort.