feat: Add promise support

[mgagliardo91]

The Problem

Current gRPC client v1 does not support promises, which the majority of NodeJs applications use over callbacks. In order to avoid consumers of this client having to wrap the methods themselves, it would be convenient to expose promise-wrapped methods of the client as an alternate capability.

Support for promises in grpc-js is still open: grpc/grpc-node#54

Implementation

This PR has two main commits, the first one is not a requirement, but added to the PR for the sake of keeping the client up-to-date with the latest tagged buf image:

Commit 8d56266

• Bumps protobuf-ts to ^2.8.1
• Re-runs the protobuf-ts generation with the latest tagged image: 97e6f8e5f9669b673a53a40658241609fb1a94e3

Commit febc165:

• Shifts to use docker compose for test env (supports --wait with health check to ensure SpiceDB container is ready)
• Creates a version of the grpc client that wraps all methods to support promises, including stream methods (using the utility function promisifyStream
• Updates the main client to expose all gRPC v1 methods (callback version) at the root via a Proxy and nests all promise-versions of the client at a key promises in the client.
• Adds separate *-promise.test.ts test files to test promise versions of the client alongside existing tests

Example

cons client = NewClient(
  'token',
  'localhost:50051',
  ClientSecurity.INSECURE_LOCALHOST_ALLOWED
);

// default method comes from gRPC generated method
client.readRelationships(request, (err, response) => {});

// promise version 
const response = await client.promises.readRelationships(request)

Additional Information

Typing support for promise usage was added dynamically; however, methods that returned a ClientReadableStream had to be called out specifically using a static set to indicate that they should be handled differently.

For promisifying readable streams, the logic will return a list of responses that are accrued while the stream was open.

const response = client.promises.readRelationships(request)
typeof response // array

Other Comments

A lot of the files changed are from the fact that js-dist is checked in - it may be worth ignoring that and only building it at the time of deploying to NPM (looks like it currently does that anyway).

Thanks for reviewing!

[mgagliardo91]

@samkim I noticed you had a draft PR out for promise support. I was having to wrap it locally to get it to work in our setup, and figured it may be worth trying to get it up in a PR. Hopefully this gives an option of how it can be tackled until grpc-js supports it natively.

[mgagliardo91]

I think it may be worthwhile to have the buf generation and js-dist build/publish purely happen in CI so that all generated files are not checked in, but that would likely be better in a separate PR to avoid bloating this one.

[mgagliardo91]

@samkim I have addressed your comments in the previous three commits:

1d03f1b

• Comments/renaming

974db2e:

• Add full consistency to the Lookup Resources tests to remove flakey failures noted above
• Re-organizes code to move proxy handlers into full classes for readability
• Adds a level of caching to the PromiseClient to avoid re-wrapping the original client methods at each request

8496f78

• Migrates the last non-unique token test to a unique token

Per your comment #47 (comment) , I have seen both locally and now in CI, failures due to trying to create relationships that already exist and I believe this is just from the fact of re-using the same token rather than making them unique for each test run.

Thanks for taking the time to give it another review - the tests appear to be passing in full.

[mgagliardo91]

@samkim in a follow-up PR/issue, I would be interested moving all of the generation (both buf and the javascript client) to CI completely, which should reduce the size of PRs/checked-in code that can be recreated on the fly. Thoughts on moving that direction?

[samkim]

@samkim in a follow-up PR/issue, I would be interested moving all of the generation (both buf and the javascript client) to CI completely, which should reduce the size of PRs/checked-in code that can be recreated on the fly. Thoughts on moving that direction?

Ideally we'd move the code gen to CI. However, we initially checked in both the grpc and js generated code to 1) ease local testing and 2) we initially encountered discrepancies across dev and CI with buf gen. We're open to trying to move the generated code out of the repo though so we can collaborate on that in a subsequent PR as you mentioned.

Thanks for the updates. I'll review this evening.

[samkim]

Just a few more minor suggestions!

[mgagliardo91]

@samkim should be ready for another look - I went ahead and created a helper function for test token generation, and went with a bit more uniqueness to minimize conflicts via the current timestamp (how we are doing it in our spicedb integration testing).

[samkim]

@mgagliardo91 Sorry 2 more minor changes that I missed, then it looks good to go

[mgagliardo91]

@samkim no problem, thanks for the quick review!

[mgagliardo91]

@samkim I went ahead and updated the README with content on the usage for the promise methods. If everything looks alright with you, I would love to get this merged today and get a new release version out so we can begin using it on our side.

Thanks!

[josephschorr]

Can we bind the generics here to the protocol buffer base interface(s)?

[mgagliardo91]

@josephschorr I may be misunderstanding this comment - the ZedPromiseClientInterface takes care of inferring the generics from the base interfaces based on the matching declarations (for both callbacks and streams). The usage of these methods will have the correct typings from the base methods.

I chose to go this route over manually recreating the interfaces as it should allow for automatic support of new client methods in the future, or until grpc-js supports promises directly: grpc/grpc-node#54

[josephschorr]

I was just curious if there was a way to further restrict the types of P1-P3 so that they only work with our specific response types; its not critical if the answer is "not easily"

[mgagliardo91]

Thanks for clarifying. Honestly, I don't think so because the generated types aren't inheriting any common base interfaces, so it would be moreso restricting P1-P3 to very specific cases manually, which doesn't buy much. This utility method is not exposed publicly, so the final methods (i.e. the generated promise lookupResources) will be fully-typed to that of the original base types via the inferred interface https://github.com/authzed/authzed-node/pull/47/files/742028b528add5c02040ce913e0ded43ab5877f9#diff-ac70eb13bcaa386d4139968851e423915ee41c96e70d29b1d78c7659e1aa0bd2R21

[samkim]

Just as a sanity check, do you see all the method signatures inferred correctly? I'm only seeing v1 methods that take a single request object.

[mgagliardo91]

@samkim you're right - I missed that as part of the type inference. I have a commit I'll push up in the next few hours that should set them correctly. Thanks!

[samkim]

Ah ok, can you include a call using one of the other signatures in a test eg https://github.com/authzed/authzed-node/pull/47/files#diff-20dc1a7112d38b8a2aa3c687aa523851d6f5b7c62acc2e08274d2be289ec86d4R73

After that, I'll merge and publish a release. Thanks!

[mgagliardo91]

@samkim see the latest commit 3093dbf

I restructured the typescript shape to be more specific to the grpc types and allow the full signature to be passed. You can see the full signatures in use in the v1-promises.test.ts test.

[samkim]

LGTM 🎉