Copilot commented on code in PR #3763:
URL: https://github.com/apache/iggy/pull/3763#discussion_r3668716785
##########
foreign/node/src/client/client.socket.ts:
##########
@@ -104,54 +143,86 @@ export class CommandResponseStream extends EventEmitter {
*
* @param command - Command code to send
* @param payload - Command payload buffer
- * @param handleResponse - Whether to parse the response (default: true)
- * @param last - Whether to add to end of queue (default: true)
+ * @param options - Response and queue options
* @returns Promise resolving to the command response
*/
async sendCommand(
command: number,
payload: Buffer,
- handleResponse = true,
- last = true
+ options: SendCommandOptions = {}
): Promise<CommandResponse> {
+ this.pendingSubmissions += 1;
+ try {
+ const {
+ handleResponse = true,
+ last = true
+ } = options;
Review Comment:
`sendCommand` now destructures `options` as an object. This is a runtime
breaking change for any JS/TS callers still passing the old positional boolean
arguments (e.g., `sendCommand(code, payload, false)`), which will throw at
destructuring time. To preserve backwards compatibility, accept a union (e.g.,
`boolean | SendCommandOptions`) and normalize at runtime (and/or provide
overloads) so legacy call sites continue to work without throwing.
##########
foreign/node/src/client/client.type.ts:
##########
@@ -44,14 +44,25 @@ export type CommandResponse = {
data: Buffer
};
+export type SendCommandOptions = {
+ /** Whether the response uses the standard command response decoder */
+ handleResponse?: boolean,
+ /** Whether to append rather than prepend the command to the queue */
+ last?: boolean
+};
+
/**
* Low-level client interface for communicating with the Iggy server.
* Provides direct access to command sending and event handling.
*/
export type RawClient = {
+ /** Server wire protocol used by this connection */
+ readonly protocol: Protocol,
/** Sends a command to the server and returns the response */
sendCommand: (
- code: number, payload: Buffer, handleResponse?: boolean
+ code: number,
+ payload: Buffer,
+ options?: SendCommandOptions
) => Promise<CommandResponse>,
Review Comment:
`SendCommandOptions.handleResponse` is documented as “Whether the response
uses the standard command response decoder”, but in multiple places (and tests)
`handleResponse: false` returns a raw `Buffer` rather than a `CommandResponse`.
This makes the exported `RawClient.sendCommand` type unsound for consumers.
Consider either (a) always returning a `CommandResponse` (even for raw frames),
or (b) changing the type signature via overloads / a discriminated option so
`handleResponse: false` returns `Promise<Buffer>` and `handleResponse:
true|undefined` returns `Promise<CommandResponse>`.
##########
foreign/node/scripts/check-vsr-protocol.mjs:
##########
@@ -0,0 +1,301 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import assert from 'node:assert/strict';
+import { access, readFile } from 'node:fs/promises';
+import { resolve } from 'node:path';
+import { pathToFileURL } from 'node:url';
+
+const rootCandidates = [
+ resolve(import.meta.dirname, '../..'),
+ resolve(import.meta.dirname, '../../..')
Review Comment:
`import.meta.dirname` is not available in all supported Node.js ESM runtimes
(it depends on the Node version). If this repository supports Node versions
that don’t include it, this build-time protocol check will fail even when the
SDK code is correct. Using `fileURLToPath(import.meta.url)` + `dirname(...)` is
the most compatibility-safe way to compute the script directory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
