Refactor DO Bindings and extract them to utils #463
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mendess
requested review from
armfazh,
bhalleycf,
captain-mota and
cjpatton
as code owners
mendess
force-pushed
the
mendess/refactor-do-bindings
branch
from
0ab601a to
0d31a64
Compare
mendess
force-pushed
the
mendess/refactor-do-bindings
branch
11 times, most recently
from
fdd9d15 to
2b671ae
Compare
cjpatton
reviewed
Dec 20, 2023
There was a problem hiding this comment.
Beautiful work! I think the main thing that's left to do is to add a bit of versioning. I'm also not super happy with the custom serialization format for the storage proxy. Ideally we don't have a mixture of serialization formats unless we absolutely have to.
cjpatton
requested changes
Dec 20, 2023
mendess
force-pushed
the
mendess/refactor-do-bindings
branch
4 times, most recently
from
7448560 to
d724c41
Compare
mendess
force-pushed
the
mendess/refactor-do-bindings
branch
6 times, most recently
from
3b0d2bb to
653f0f6
Compare
cjpatton
approved these changes
Dec 21, 2023
There was a problem hiding this comment.
Looks good to me! Great work.
My one suggestion would be to add a version prefix to the path of the request from the edge service to the storage proxy. capnproto only solves the versioning problem if we decide to always use capnproto. It's still a good idea I think to version the path so that we can change to something else if we needed to. (Hopefully we don't.)
mendess
force-pushed
the
mendess/refactor-do-bindings
branch
10 times, most recently
from
ba5e7cc to
1fe5a90
Compare
Make a more intuitive and typesafe api for interacting with DOs
mendess
force-pushed
the
mendess/refactor-do-bindings
branch
2 times, most recently
from
c32f833 to
bac1448
Compare
mendess
force-pushed
the
mendess/refactor-do-bindings
branch
from
bac1448 to
e1c4e53
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of using global constants to encode the various bindings and methods, define a more intuitive and typesafe api for interacting with DOs.
Also add an abstraction that makes use of these types to implement the communication with the future storage proxy