- 
                Notifications
    
You must be signed in to change notification settings  - Fork 10
 
Prototype protobuf-es usage #70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 
           The latest updates on your projects. Learn more about Vercel for Git ↗︎ 
  | 
    
96cdfc2    to
    759cfcc      
    Compare
  
    | 
           There's something funky with check watches. Wasm isn't working in development, which would be necessary to debug it. I'll work on that later.  | 
    
759cfcc    to
    b9577de      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment
| version: "v2" | ||
| plugins: | ||
| - remote: "buf.build/community/timostamm-protobuf-ts:v2.9.1" | ||
| - remote: "buf.build/bufbuild/es:v2.4.0" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This swaps in the new codegen.
| import { | ||
| Struct, | ||
| Value, | ||
| } from "../spicedb-common/protodefs/google/protobuf/struct"; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's lots of places where we were using Struct that we no longer need to, because protobuf-es automatically converts a normal JS object to Structs and Values where needed. This is one of the big ergonomic benefits.
| if (t.resolution.case === "subProblems") { | ||
| t.resolution.value.traces.forEach(appendExpanded); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the difference in how protobuf-es represents oneOfKind fields.
| } | ||
| 
               | 
          ||
| function ContextTreeView(context: Struct | undefined) { | ||
| function ContextTreeView(context: JsonObject | undefined) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of a Struct on a message is just a JsonObject as far as client code is concerned, so we convert these functions to handle them.
| }), | ||
| true, | ||
| ]; | ||
| function ContextTreeValue(value: JsonValue) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function needed to change a bit - rather than switching on a string field, we switch on the type of the value itself.
| let checkResult: CheckOperationsResult = create( | ||
| CheckOperationsResultSchema, | ||
| { | ||
| membership: CheckOperationsResult_Membership.UNKNOWN, | ||
| }, | ||
| ); | ||
| request?.check( | ||
| create(CheckOperationParametersSchema, { | ||
| resource: relationship.resourceAndRelation!, | ||
| subject: relationship.subject!, | ||
| }, | ||
| }), | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than the message types having their own constructors, you have a Schema object that you hand to a create function. It's a little more verbose in some ways, but the create function lets you pass in normal JS objects and it Just Works, which is nifty.
| if ( | ||
| status.code === | ||
| LookupShareResponse_LookupStatus.FAILED_TO_LOOKUP.toString() | ||
| response.status === LookupShareResponse_LookupStatus.FAILED_TO_LOOKUP | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shape change - status is on the object.
| : "", | ||
| expiration: rel.optionalExpirationTime | ||
| ? new Date(parseFloat(rel.optionalExpirationTime.seconds) * 1000) | ||
| ? timestampDate(rel.optionalExpirationTime) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another nicety - there are some utility functions around well-known types, so we don't have to manually parse out the representations of Timestamps.
| expect(parsed?.caveat?.caveatName).toBe("somecaveat"); | ||
| expect(parsed?.caveat?.context).toHaveProperty("fields"); | ||
| expect(parsed?.caveat?.context).toEqual({ | ||
| fields: { | ||
| hi: { | ||
| kind: { | ||
| oneofKind: "stringValue", | ||
| stringValue: "there", | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| assert(parsed?.caveat?.context); | ||
| expect(Struct.toJson(parsed?.caveat?.context)).toEqual({ hi: "there" }); | ||
| expect(parsed?.caveat?.context).toEqual({ hi: "there" }); | ||
| }); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a concrete illustration of how Structs changed.
| const encodedResponse: string = window[ENTRYPOINT_FUNCTION]( | ||
| DeveloperRequest.toJsonString(request), | ||
| const developerRequest = JSON.stringify( | ||
| toJson(DeveloperRequestSchema, request), | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat confusingly, toJson returns a JS object, not a JSON string.
b9577de    to
    0fbf524      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool
Fixes #38
Description
protobuf-esis a more modern take on a TS-based implementation of protobuf and clients. I'm considering using it for authzed-node, but I wanted to test it out in the real world; this library seems like a good place to do that.Notes
This adds about 100kb of bundle size, which isn't ideal, but we're also already at 5mb.
Changes
Will annotate
Testing
Review. See that tests pass and that playground functionality still works as expected.