Skip to content

Commit 128faf6

Browse files
minggangwminggangw
authored
Align WS and HTTP transports on a default /capability path (#1515)
Both transports now apply the default path when (and only when) the caller didn't supply one. Before this commit WS required the caller to spell `/capability` out (`connect('ws://host:9000')` silently failed against a default server) and HTTP did the opposite (it hardcoded `/capability/<kind>/<name>` so `connect('http://host:9001/capability')` silently double-prefixed and 404'd). Pure addition: existing `connect('ws://host:9000/capability')` and `connect('http://host:9001')` callers see byte-identical request URLs. - `web/client.js`: new `_normaliseWsPath()` wired into `_resolveUrls()` for both URL forms; `_HttpLink.constructor` only appends `/capability` when the URL doesn't already end with it; `_fetch()` builds URLs from the normalised baseUrl with no hardcoded literal. `RosClient` JSDoc + matching `web/index.d.ts` updated to drop the spell-it-out examples and document the "pass an explicit non-default path" escape hatch for path-rewriting proxies. - `test/test-web-ws.js`: +2 cases (`ws://host:port` and `{ ws: 'ws://host:port' }` against a live runtime). - `test/test-web-http.js`: +2 cases (`httpBase + '/capability'` under both single-string and `{ http }` forms). Out of scope: custom server `basePath` / `path` configurations behind a reverse proxy. Same "user URL is authoritative when non-default" model could extend to support them, but that deserves its own PR with proxy-deployment docs and tests. Fix: #1510
1 parent 837cd85 commit 128faf6

5 files changed

Lines changed: 164 additions & 66 deletions

File tree

‎test/test-type-description-service.js‎

Lines changed: 65 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -89,59 +89,77 @@ describe('type description service test suite', function () {
8989
this.skip();
9090
}
9191

92-
setTimeout(() => {
93-
exec(
94-
'ros2 param list /test_type_description_service',
95-
(error, stdout, stderr) => {
96-
if (error || stderr) {
97-
done(
98-
new Error(
99-
`Test type description service configured by parameter failed. Error: ${error}, Stderr: ${stderr}`
100-
)
101-
);
102-
return;
103-
}
104-
if (stdout.includes('start_type_description_service')) {
105-
done();
106-
} else {
107-
done(
108-
new Error("'start_type_description_service' not found in stdout.")
109-
);
110-
}
111-
}
112-
);
113-
}, 1000);
92+
// ROS 2 graph discovery is asynchronous: there is no guarantee
93+
// that an external `ros2` CLI process will see this node
94+
// immediately after rclnodejs.spin() returns. A fixed setTimeout
95+
// is racy on slower runners (notably the Rolling lane). Poll
96+
// instead, with a generous overall budget.
97+
waitForRos2Cli(
98+
'ros2 param list /test_type_description_service',
99+
(stdout) => stdout.includes('start_type_description_service'),
100+
done,
101+
"'start_type_description_service' not found in stdout."
102+
);
114103
});
115104

116105
it('Test start_type_description_service parameter value', function (done) {
117106
if (process.platform === 'win32') {
118107
this.skip();
119108
}
120109

121-
setTimeout(() => {
122-
exec(
123-
'ros2 param get /test_type_description_service start_type_description_service',
124-
(error, stdout, stderr) => {
125-
if (error || stderr) {
126-
done(
127-
new Error(
128-
`Test type description service configured by parameter failed. Error: ${error}, Stderr: ${stderr}`
129-
)
130-
);
131-
return;
132-
}
133-
if (stdout.includes('Boolean value is: True')) {
134-
done();
135-
} else {
136-
console.log(stdout);
137-
done(
138-
new Error(
139-
"'start_type_description_service param value' not found in stdout."
140-
)
141-
);
142-
}
143-
}
144-
);
145-
}, 1000);
110+
waitForRos2Cli(
111+
'ros2 param get /test_type_description_service start_type_description_service',
112+
(stdout) => stdout.includes('Boolean value is: True'),
113+
done,
114+
"'start_type_description_service param value' not found in stdout."
115+
);
146116
});
147117
});
118+
119+
// Run a `ros2 ...` CLI command repeatedly until either `predicate(stdout)`
120+
// returns true (success) or the overall budget runs out. Treats
121+
// `Node not found` and other transient failures as retryable while the
122+
// graph is still propagating; surfaces the last error otherwise.
123+
function waitForRos2Cli(
124+
cmd,
125+
predicate,
126+
done,
127+
notFoundMessage,
128+
{ timeoutMs = 15000, intervalMs = 500 } = {}
129+
) {
130+
const deadline = Date.now() + timeoutMs;
131+
let lastErr = null;
132+
let lastStdout = '';
133+
let lastStderr = '';
134+
135+
const tick = () => {
136+
exec(cmd, (error, stdout, stderr) => {
137+
lastErr = error;
138+
lastStdout = stdout || '';
139+
lastStderr = stderr || '';
140+
141+
if (!error && !stderr && predicate(lastStdout)) {
142+
return done();
143+
}
144+
145+
if (Date.now() < deadline) {
146+
return setTimeout(tick, intervalMs);
147+
}
148+
149+
// Timed out. Prefer the most informative failure: the predicate
150+
// mismatch (we got a CLI response but the wrong content) wins
151+
// over a transient discovery error.
152+
if (!error && !stderr) {
153+
return done(new Error(`${notFoundMessage}\nstdout: ${lastStdout}`));
154+
}
155+
done(
156+
new Error(
157+
`\`${cmd}\` did not succeed within ${timeoutMs}ms. ` +
158+
`Last error: ${lastErr}, last stderr: ${lastStderr}`
159+
)
160+
);
161+
});
162+
};
163+
164+
tick();
165+
}

‎test/test-web-http.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,33 @@ describe('rclnodejs/web — HTTP transport (call + publish)', function () {
238238
await ros.close();
239239
}
240240
});
241+
242+
// The HTTP transport's default basePath is `/capability`. Callers
243+
// should be able to spell that out explicitly without the SDK
244+
// double-prefixing it on every fetch — `'http://host:port'` and
245+
// `'http://host:port/capability'` must produce the same request
246+
// URLs.
247+
it('accepts URLs with explicit /capability suffix without double-prefixing', async function () {
248+
const ros = await connect(httpBase + '/capability');
249+
try {
250+
const reply = await ros.call('/http_add', { a: '5n', b: '7n' });
251+
assert.strictEqual(reply.sum, '12n');
252+
} finally {
253+
await ros.close();
254+
}
255+
});
256+
257+
it('idempotent under { http } form too', async function () {
258+
const ros = await connect({ http: httpBase + '/capability' });
259+
try {
260+
const ret = await ros.publish('/http_chatter', {
261+
data: 'explicit-base',
262+
});
263+
assert.strictEqual(ret, undefined);
264+
} finally {
265+
await ros.close();
266+
}
267+
});
241268
});
242269

243270
// -----------------------------------------------------------------

‎test/test-web-ws.js‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,31 @@ describe('rclnodejs/web (Browser SDK)', function () {
133133
await ros.close();
134134
}
135135
});
136+
137+
// The runtime listens on `/capability` by default. Callers should be
138+
// able to omit that path from the URL and still connect — the SDK
139+
// appends it automatically when the user-supplied URL has no path.
140+
it('appends the default /capability path when the URL has none', async function () {
141+
const port = runtime.transports[0].port;
142+
const ros = await connect(`ws://127.0.0.1:${port}`);
143+
try {
144+
const sub = await ros.subscribe('/rt_chatter', () => {});
145+
await sub.close();
146+
} finally {
147+
await ros.close();
148+
}
149+
});
150+
151+
it('appends the default /capability path inside { ws } form', async function () {
152+
const port = runtime.transports[0].port;
153+
const ros = await connect({ ws: `ws://127.0.0.1:${port}` });
154+
try {
155+
const sub = await ros.subscribe('/rt_chatter', () => {});
156+
await sub.close();
157+
} finally {
158+
await ros.close();
159+
}
160+
});
136161
});
137162

138163
function waitFor(predicate, timeoutMs) {

‎web/client.js‎

Lines changed: 41 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -238,8 +238,16 @@ class _WsLink {
238238
*/
239239
class _HttpLink {
240240
constructor(baseUrl) {
241-
// Normalise: strip trailing slash so we can append `/capability/<kind>/<name>`
242-
this.baseUrl = baseUrl.replace(/\/+$/, '');
241+
// Normalise: strip trailing slash so we can append the path. If
242+
// the user URL already ends with `/capability` (e.g. they spelled
243+
// it out explicitly, mirroring the WS form), don't double-prefix
244+
// it on every fetch. The default-runtime layout still works for
245+
// the bare-host form `'http://host:9001'` — we just append the
246+
// default path ourselves below.
247+
const trimmed = baseUrl.replace(/\/+$/, '');
248+
this.baseUrl = trimmed.endsWith('/capability')
249+
? trimmed
250+
: trimmed + '/capability';
243251
}
244252

245253
async connect() {
@@ -261,8 +269,7 @@ class _HttpLink {
261269
}
262270

263271
async _fetch(kind, capability, payload, expectBody) {
264-
const url =
265-
this.baseUrl + '/capability/' + kind + '/' + _encodeRosName(capability);
272+
const url = this.baseUrl + '/' + kind + '/' + _encodeRosName(capability);
266273
let res;
267274
try {
268275
res = await fetch(url, {
@@ -328,17 +335,18 @@ function _encodeRosName(name) {
328335
* same host with `/capability` appended.
329336
* - object `{http, ws}` → both URLs spelled out explicitly.
330337
*
331-
* **Path conventions.** The single-URL forms assume the server uses
332-
* the default `/capability` layout for both transports. If you change
333-
* `--path` / `--http-base-path` on the server (or sit it behind a
334-
* path-rewriting proxy), pass the full URLs via `{http, ws}` so the
335-
* SDK does not have to guess.
338+
* **Path conventions.** When a `ws://` / `wss://` URL is passed
339+
* without a path (or with just `/`), the SDK appends the runtime's
340+
* default `/capability` path automatically — `'ws://host:9000'` and
341+
* `'ws://host:9000/capability'` therefore behave identically. Pass
342+
* an explicit non-default path if your server changed `--path` /
343+
* `--http-base-path` or sits behind a path-rewriting proxy.
336344
*
337345
* @example
338346
* import { connect } from 'rclnodejs/web';
339347
*
340-
* // WebSocket-only
341-
* const ros = await connect('ws://robot.local:9000/capability');
348+
* // WebSocket-only (path defaults to /capability)
349+
* const ros = await connect('ws://robot.local:9000');
342350
*
343351
* // HTTP for call/publish, automatic WS sibling for subscribe
344352
* const ros = await connect('http://robot.local:9001');
@@ -473,7 +481,7 @@ function _resolveUrls(url) {
473481
// Form 1: explicit { http, ws } pair.
474482
if (url && typeof url === 'object' && !Array.isArray(url)) {
475483
const httpUrl = _validateEndpoint(url.http, 'http');
476-
const wsUrl = _validateEndpoint(url.ws, 'ws');
484+
const wsUrl = _normaliseWsPath(_validateEndpoint(url.ws, 'ws'));
477485
if (!httpUrl && !wsUrl) {
478486
throw new TypeError(
479487
'connect({http, ws}): at least one of http or ws must be provided'
@@ -489,7 +497,7 @@ function _resolveUrls(url) {
489497
);
490498
}
491499
if (/^wss?:\/\//i.test(url)) {
492-
return { httpUrl: null, wsUrl: url, wsExplicit: true };
500+
return { httpUrl: null, wsUrl: _normaliseWsPath(url), wsExplicit: true };
493501
}
494502
if (/^https?:\/\//i.test(url)) {
495503
// HTTP base URL: derive a sibling WS URL lazily — we don't open
@@ -501,6 +509,26 @@ function _resolveUrls(url) {
501509
);
502510
}
503511

512+
// Append the runtime's default `/capability` path when the caller
513+
// passed a bare host (`ws://host:9000` or `ws://host:9000/`). Leave
514+
// any explicit non-empty path untouched so users behind a
515+
// path-rewriting proxy or with a non-default `WebSocketTransport({
516+
// path })` keep working. Returns the input unchanged on parse error
517+
// (let the underlying WebSocket constructor surface the bad URL).
518+
function _normaliseWsPath(wsUrl) {
519+
if (!wsUrl) return wsUrl;
520+
try {
521+
const u = new URL(wsUrl);
522+
if (u.pathname === '' || u.pathname === '/') {
523+
u.pathname = '/capability';
524+
return u.toString();
525+
}
526+
return wsUrl;
527+
} catch (_) {
528+
return wsUrl;
529+
}
530+
}
531+
504532
// Swap http(s) → ws(s) and append the default `/capability` path.
505533
// Returns null if the URL can't be parsed; verbs that need WS will
506534
// then surface a clear `transport_unavailable` error themselves.

‎web/index.d.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -143,12 +143,12 @@ declare module 'rclnodejs/web' {
143143
* host with `/capability` appended.
144144
* - {@link ConnectEndpoints} — both URLs spelled out.
145145
*
146-
* **Path conventions.** The single-URL forms assume the server uses
147-
* the default `rclnodejs-web` path layout: `/capability` for both
148-
* transports. If you change `--http-base-path` or `--path` on the
149-
* server (or sit it behind a path-rewriting proxy), pass the full
150-
* URLs via {@link ConnectEndpoints} instead so the SDK does not
151-
* have to guess.
146+
* **Path conventions.** When a `ws://` / `wss://` URL is passed
147+
* without a path (or with just `/`), the SDK appends the runtime's
148+
* default `/capability` path automatically — so `'ws://host:9000'`
149+
* and `'ws://host:9000/capability'` behave identically. Pass an
150+
* explicit non-default path if your server changed `--path` /
151+
* `--http-base-path` or sits behind a path-rewriting proxy.
152152
*/
153153
export class RosClient {
154154
readonly url: string;

0 commit comments

Comments
 (0)