Create primaryStub and deprecate 'primary' field.#6652
Create primaryStub and deprecate 'primary' field.#6652jtolio merged 0 commit intojolio/move-config-read-replicationfrom
Conversation
|
This PR was unfortunately moved from #6605 |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds ctx.primaryStub as a clearer alternative to ctx.storage.primary on DurableObjectState, and adds a @deprecated tag to the old property.
One minor issue:
- (Medium) The deprecation message on
DurableObjectStorage.primarysays "UseprimaryStubinstead" butprimaryStublives on a different object (DurableObjectState, i.e.ctx.primaryStub). A user readingctx.storage.primaryand seeing this deprecation might look forctx.storage.primaryStub, which doesn't exist. The message should clarify the new location.
|
I've posted a review on PR #6652 with one medium-severity finding: The deprecation message on The rest of the implementation looks correct — the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## jolio/move-config-read-replication #6652 +/- ##
======================================================================
- Coverage 66.44% 66.42% -0.02%
======================================================================
Files 405 405
Lines 116106 116112 +6
Branches 19433 19434 +1
======================================================================
- Hits 77148 77131 -17
- Misses 27377 27401 +24
+ Partials 11581 11580 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
Comparing Footnotes
|
56a2896 to
0864429
Compare
justin-mp
left a comment
There was a problem hiding this comment.
I think this PR is fine, but you should invert the PR ordering so that this one is a parent of #6649 since being able to use getPrimary would help you significantly clean up #6649 .
It's probably worth dropping a TODO(cleanup) in the DurableObjectState::getPrimary implementation noting that we probably want getPrimary in that class rather than in DurableObjectStorage. (If you have any ideas on how we might move the implementation of getPrimary out of DurableObjectStorage once we've removed the primary API, please put them in the TODO.)
| DurableObjectStorage: { | ||
| primary: `* @deprecated Use \`ctx.primaryStub\` instead. `, | ||
| }, |
There was a problem hiding this comment.
For my own education, why do we document this here? If someone doesn't have the experimental flag set, would they see this?
There was a problem hiding this comment.
so two things I have discovered:
- annotations added to this file do not do anything unless the field getting documented is actually visible somewhere, so, for the purposes of non-experimental annotations, this is a no-op.
- we actually do generate typescript annotations and docstrings for experimental-only flag fields, but they are in an experimental output folder (types/generated-snapshot/experimental).
i don't honestly know what we do with stuff in types/generated-snapshot/experimental. i assume customers don't see it, but if it was not used at all would it be in here? anyway, at the advice of the Bonk bot earlier in the thread it seemed harmless enough to add.
| } | ||
|
|
||
| if (flags.getWorkerdExperimental()) { | ||
| JSG_LAZY_READONLY_INSTANCE_PROPERTY(primaryStub, getPrimary); |
There was a problem hiding this comment.
Typically, by convention, we try to keep the naming aligned.. e.g. primaryStub -> getPrimaryStub. Since the getPrimary method here is being added, is there a reason not to align the names?
There was a problem hiding this comment.
good point, will fix, if i can figure out how to reopen this PR
2684b54 to
6fd5e2b
Compare
0864429 to
857b457
Compare
|
Cool, Github! That's exactly what I wanted to have happen.
|
|
Moved to #6663 |
The 'primary' field name is confusing, as it is easy to read code like
and assume it's a boolean, not a stub. you might think the above is a test to make sure we're not the primary, but because the primary stub is set only on replicas, it's the opposite!
is easier to read and understand that this is an 'if' checking if we're the primary.
This change also moves the registration location to be outside of .ctx.storage.
Once #6649 is merged, this PR should be retargeted to main.