Create primaryStub and deprecate 'primary' field.#6605
Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds primaryStub as an alias for the existing primary property on DurableObjectStorage, both backed by getPrimary(), and marks primary as deprecated via a C++ comment.
Issues
-
Missing
@deprecatedJSDoc onprimaryin TypeScript types (medium) — The deprecation is only expressed as a C++ comment. TypeScript users will see bothprimaryStubandprimaryin their IDE with no indication thatprimaryis deprecated. Consider adding a@deprecatedJSDoc annotation so IDEs surface the deprecation with a strikethrough. This could be done viaJSG_TS_OVERRIDEin theDurableObjectStorageblock, or by adding a comment annotation that the type generator picks up. -
No test for the new property (low) — Since
primaryStubmaps to the same getter asprimary, the risk is very low. Still, a quick assertion in an existing test thatctx.storage.primaryStubis accessible would provide coverage.
|
I've posted my review on PR #6605. Here's a summary: PR adds Two issues flagged:
|
ab0c23c to
6eef891
Compare
justin-mp
left a comment
There was a problem hiding this comment.
As matter of onboarding you our usual approach to commits, let's do this one by the book.
- The doc updates for ensureReplicas and disableReplicas should be separate commits. Why? Because commits should have exactly one idea in them. (They should really be a separate PR, but we do just-passing-through cleanups like this sometimes because stacking PRs is such a pain.)
- Rebase on top of master rather than merge. Why? We prefer to edit history in PRs.
Will split!
This PR is in auto-merge rebase mode, which means Github will cherry pick the lone commit in this branch onto main. The merge commit that exists was generated from Github's UI when I clicked "update branch", but I don't believe it will show up in the history if Github applies this commit in rebase mode. But okay, I'll avoid the button going forward even so! |
0b84773 to
e5fd145
Compare
Merging this PR will degrade performance by 12.94%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | Encode_ASCII_32[TextEncoder][0/0/32] |
2.7 ms | 3.1 ms | -12.94% |
Comparing jolio/add-primaryStub (2289287) with main (4028835)2
Footnotes
-
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. ↩
-
No successful run was found on
jolio/move-config-read-replication(8af12c7) during the generation of this report, somain(4028835) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
1d15aff to
da3c505
Compare
|
(drive-by comment, feel free to ignore) Arguably this also belongs on |
That's a good point. We put it on |
X-BranchTarget: jolio/add-primaryStub
da3c505 to
2289287
Compare
b90a31e
into
jolio/move-config-read-replication
|
ugh! unmerge! |
|
lol, this is what I get for setting auto merge mode on and then changing the target branch to one with less protections. okay |
|
Okay, sorry, this PR is reopened at #6652 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## jolio/move-config-read-replication #6605 +/- ##
=====================================================================
Coverage ? 66.42%
=====================================================================
Files ? 405
Lines ? 116112
Branches ? 19434
=====================================================================
Hits ? 77131
Misses ? 27401
Partials ? 11580 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.