NativeConnection powered Client #1699
Conversation
bergundy
force-pushed
the
native-connection-powered-client
branch
2 times, most recently
from
c8121c2 to
9accec3
Compare
|
I'm adding tests for using the client including the |
bergundy
force-pushed
the
native-connection-powered-client
branch
4 times, most recently
from
4c05e3a to
b36f850
Compare
| /** | ||
| * nativeClient is intentionally left private, framework code can access it with `extractNativeClient` (below) | ||
| */ | ||
| protected constructor(private nativeClient: Client) {} | ||
| protected constructor(private nativeClient: Client) { | ||
| this.workflowService = WorkflowService.create(this.sendRequest.bind(this), false, false); |
There was a problem hiding this comment.
Definitely not required for this PR, but any reason we should not also implement other services over NativeConnection at some later point?
There was a problem hiding this comment.
We can definitely do that later, I just wanted to limit the scope for this MVP. I will open an issue.
packages/worker/src/connection.ts
Outdated
| @@ -1,29 +1,208 @@ | |||
| import util from 'node:util'; | |||
| import { AsyncLocalStorage } from 'node:async_hooks'; | |||
| import * as grpc from '@grpc/grpc-js'; | |||
| import { getRelativeTimeout } from '@grpc/grpc-js/build/src/deadline'; | |||
There was a problem hiding this comment.
That function is pretty simple. I'd prefer we inline it here rather than have an import to their internal project structure.
packages/worker/src/connection.ts
Outdated
| import { NativeConnectionOptions } from './connection-options'; | ||
| import { Runtime } from './runtime'; | ||
|
|
||
| const updateHeaders = util.promisify(clientUpdateHeaders); | ||
| const updateApiKey = util.promisify(clientUpdateApiKey); | ||
|
|
||
| @SymbolBasedInstanceOfError('ServiceError') | ||
| export class ServiceError extends Error { |
There was a problem hiding this comment.
I don't think we should export this class, nor use the @SymbolBasedInstanceOfError decorator on it, as it is likely to be confused with the ServiceError class exported by the client package (yet, isn't exactly the same thing, as the later corresponds to errors rethrown by the Client, not those coming from the Connection).
In the client package, we simply use the isGrpcServiceError() type guard. So I suggest you keep this class here, just make it private and remove the decorator, and possibly add an implements GrpcServiceError if you feel like it.
There was a problem hiding this comment.
Hmm... I feel like any error that we throw should be exported but I'm not highly opinionated here.
FYI grpc-js ServiceError is not a concrete implementation, which is why I created this class.
WDYT?
There was a problem hiding this comment.
I feel like any error that we throw should be exported
That's normally what I would do, but in this case, NativeConnection is basically mocking the grpc-js API, which isn't exposing that class.
Exposing that class from the worker package means that a user (or our own code) could do err instanceof ServiceError if the error come from a NativeConnection, but would imperatively have to do isGrpcServiceError if the connection is a Connection.
There was a problem hiding this comment.
And indeed, grpc-js's ServiceError is just a type, not a class, so you would still want that class internally; just add an implement GrpcServiceError to the class signature to ensure we're not deviating from their API.
There was a problem hiding this comment.
I ended up not exporting this since it's easy to export later while we cannot remove an already exported class.
bergundy
force-pushed
the
native-connection-powered-client
branch
from
b36f850 to
158107c
Compare
This work enables both Nexus handlers and activities to get a
Clientpowered by theNativeConnectionof theWorkerthey are run in.I left a TODO to support request cancelation via
withAbortSignal, I may wait for @mjameswh's refactor before adding that.