Conversation
…vice client connection
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Sushisource
left a comment
Sushisource
left a comment
There was a problem hiding this comment.
General notes:
- Not seeing a way to configure the replay worker explicitly. This exists in the other SDKs.
- The new interfaces should be marked experimental
| * them to set API keys, gRPC metadata, TLS context, and other | ||
| * connection-level options. | ||
| */ | ||
| interface ConnectionPluginInterface |
There was a problem hiding this comment.
Doesn't extend PluginInterface but probably should?
There was a problem hiding this comment.
Missed one, thanks!
| * Use this hook to configure connection-level settings such as | ||
| * API keys, gRPC metadata, auth tokens, or context options. | ||
| */ | ||
| public function configureServiceClient(ConnectionPluginContext $context): void; |
There was a problem hiding this comment.
This doesn't do chain-of-responsibility like in the other SDKs - as a general rule I think we want all of the plugins to be able to work that way, as it allows for true interception rather than just configuration.
There was a problem hiding this comment.
Some of changes were made on purpose. I've changed signature to public function configureServiceClient(ServiceClientInterface $serviceClient, callable $next): ServiceClientInterface. Is it what you wanted?
Any other methods?
| * | ||
| * @param callable(WorkerFactoryInterface): int $next Calls the next plugin or the actual run loop. | ||
| */ | ||
| public function run(WorkerFactoryInterface $factory, callable $next): int; |
There was a problem hiding this comment.
I think we probably need a runWorker or something which can wrap individual worker start calls? There are equivalents for this in the other SDKs.
| ); | ||
| $factory->newWorker(); | ||
|
|
||
| self::assertSame(['from-factory', 'from-client'], $order); |
There was a problem hiding this comment.
I believe we would want the client plugins to be applied first. The other SDKs are generally prepending the ones from the client.
| // Use only plugin interceptors - the base pipeline is lost in this edge case. | ||
| // Users should either use plugins OR a custom PipelineProvider, not both. |
There was a problem hiding this comment.
I know this is kind of already called out in the docstring, but, the fact that you could silently lose some config here probably deserves to be highlighted (ideally prevented if somehow possible).
…ace` in plugin execution
…lity within `Pipeline`
2 participants
What was changed
AbstractPlugininstead ofSimplePluginas from the rest implementationsWhy?
Resolves #644
Checklist
Closes
How was this tested: