Commit 78eb1bd1 authored by Robert Knight's avatar Robert Knight

Get WebSocket URL from /api/links endpoint

Get the WebSocket URL from the /api/links endpoint instead of the `websocketUrl`
configuration in app.html. This ensures that the client uses the correct
WebSocket endpoint for the h API service it is currently talking to, which may
be different than the default when  `services` configuration is specified.

When updating the tests, several had to be reworked to be less sensitive
to the number of microtask ticks in between certain events.

This depends on the h change in https://github.com/hypothesis/h/pull/7253.
parent b3075bd7
...@@ -23,17 +23,17 @@ import { watch } from '../util/watch'; ...@@ -23,17 +23,17 @@ import { watch } from '../util/watch';
export class StreamerService { export class StreamerService {
/** /**
* @param {import('../store').SidebarStore} store * @param {import('../store').SidebarStore} store
* @param {import('./api-routes').APIRoutesService} apiRoutes
* @param {import('./auth').AuthService} auth * @param {import('./auth').AuthService} auth
* @param {import('./groups').GroupsService} groups * @param {import('./groups').GroupsService} groups
* @param {import('./session').SessionService} session * @param {import('./session').SessionService} session
* @param {Record<string, any>} settings
*/ */
constructor(store, auth, groups, session, settings) { constructor(store, apiRoutes, auth, groups, session) {
this._auth = auth; this._auth = auth;
this._groups = groups; this._groups = groups;
this._session = session; this._session = session;
this._store = store; this._store = store;
this._websocketURL = settings.websocketUrl; this._websocketURL = apiRoutes.links().then(links => links.websocket);
/** The randomly generated session ID */ /** The randomly generated session ID */
this.clientId = generateHexString(32); this.clientId = generateHexString(32);
...@@ -84,8 +84,11 @@ export class StreamerService { ...@@ -84,8 +84,11 @@ export class StreamerService {
this._store.clearPendingUpdates(); this._store.clearPendingUpdates();
} }
/** @param {ErrorEvent} event */ /**
_handleSocketError(event) { * @param {string} websocketURL
* @param {ErrorEvent} event
*/
_handleSocketError(websocketURL, event) {
warnOnce('Error connecting to H push notification service:', event); warnOnce('Error connecting to H push notification service:', event);
// In development, warn if the connection failure might be due to // In development, warn if the connection failure might be due to
...@@ -93,7 +96,7 @@ export class StreamerService { ...@@ -93,7 +96,7 @@ export class StreamerService {
// //
// Unfortunately the error event does not provide a way to get at the // Unfortunately the error event does not provide a way to get at the
// HTTP status code for HTTP -> WS upgrade requests. // HTTP status code for HTTP -> WS upgrade requests.
const websocketHost = new URL(this._websocketURL).hostname; const websocketHost = new URL(websocketURL).hostname;
if (['localhost', '127.0.0.1'].indexOf(websocketHost) !== -1) { if (['localhost', '127.0.0.1'].indexOf(websocketHost) !== -1) {
/* istanbul ignore next */ /* istanbul ignore next */
warnOnce( warnOnce(
...@@ -173,8 +176,8 @@ export class StreamerService { ...@@ -173,8 +176,8 @@ export class StreamerService {
} }
async _reconnect() { async _reconnect() {
// If we have no URL configured, don't do anything. const websocketURL = await this._websocketURL;
if (!this._websocketURL) { if (!websocketURL) {
return; return;
} }
this._socket?.close(); this._socket?.close();
...@@ -193,11 +196,11 @@ export class StreamerService { ...@@ -193,11 +196,11 @@ export class StreamerService {
// is used to send credentials because the `WebSocket` constructor does // is used to send credentials because the `WebSocket` constructor does
// not support setting the `Authorization` header directly as we do for // not support setting the `Authorization` header directly as we do for
// other API requests. // other API requests.
const parsedURL = new URL(this._websocketURL); const parsedURL = new URL(websocketURL);
parsedURL.searchParams.set('access_token', token); parsedURL.searchParams.set('access_token', token);
url = parsedURL.toString(); url = parsedURL.toString();
} else { } else {
url = this._websocketURL; url = websocketURL;
} }
const newSocket = new Socket(url); const newSocket = new Socket(url);
...@@ -219,7 +222,7 @@ export class StreamerService { ...@@ -219,7 +222,7 @@ export class StreamerService {
); );
} }
}); });
newSocket.on('error', err => this._handleSocketError(err)); newSocket.on('error', err => this._handleSocketError(websocketURL, err));
newSocket.on('message', event => this._handleSocketMessage(event)); newSocket.on('message', event => this._handleSocketMessage(event));
this._socket = newSocket; this._socket = newSocket;
......
...@@ -74,25 +74,29 @@ class FakeSocket extends EventEmitter { ...@@ -74,25 +74,29 @@ class FakeSocket extends EventEmitter {
} }
describe('StreamerService', () => { describe('StreamerService', () => {
let fakeAPIRoutes;
let fakeStore; let fakeStore;
let fakeAuth; let fakeAuth;
let fakeGroups; let fakeGroups;
let fakeSession; let fakeSession;
let fakeSettings;
let fakeWarnOnce; let fakeWarnOnce;
let activeStreamer; let activeStreamer;
function createDefaultStreamer() { function createDefaultStreamer() {
activeStreamer = new StreamerService( activeStreamer = new StreamerService(
fakeStore, fakeStore,
fakeAPIRoutes,
fakeAuth, fakeAuth,
fakeGroups, fakeGroups,
fakeSession, fakeSession
fakeSettings
); );
} }
beforeEach(() => { beforeEach(() => {
fakeAPIRoutes = {
links: sinon.stub().resolves({ websocket: 'ws://example.com/ws' }),
};
fakeAuth = { fakeAuth = {
getAccessToken: sinon.stub().resolves('dummy-access-token'), getAccessToken: sinon.stub().resolves('dummy-access-token'),
}; };
...@@ -122,10 +126,6 @@ describe('StreamerService', () => { ...@@ -122,10 +126,6 @@ describe('StreamerService', () => {
update: sinon.stub(), update: sinon.stub(),
}; };
fakeSettings = {
websocketUrl: 'ws://example.com/ws',
};
fakeWarnOnce = sinon.stub(); fakeWarnOnce = sinon.stub();
$imports.$mock({ $imports.$mock({
...@@ -140,8 +140,9 @@ describe('StreamerService', () => { ...@@ -140,8 +140,9 @@ describe('StreamerService', () => {
fakeWebSockets = []; fakeWebSockets = [];
}); });
it('should not create a websocket connection if websocketUrl is not provided', () => { it('should not create a websocket connection if WebSocket URL is not available', () => {
fakeSettings = {}; fakeAPIRoutes.links.resolves({});
createDefaultStreamer(); createDefaultStreamer();
return activeStreamer.connect().then(() => { return activeStreamer.connect().then(() => {
...@@ -207,7 +208,9 @@ describe('StreamerService', () => { ...@@ -207,7 +208,9 @@ describe('StreamerService', () => {
}); });
it('should preserve query params when adding access token to URL', () => { it('should preserve query params when adding access token to URL', () => {
fakeSettings.websocketUrl = 'ws://example.com/ws?foo=bar'; fakeAPIRoutes.links.resolves({
websocket: 'ws://example.com/ws?foo=bar',
});
createDefaultStreamer(); createDefaultStreamer();
return activeStreamer.connect().then(() => { return activeStreamer.connect().then(() => {
assert.equal( assert.equal(
...@@ -264,21 +267,19 @@ describe('StreamerService', () => { ...@@ -264,21 +267,19 @@ describe('StreamerService', () => {
console.error.restore(); console.error.restore();
}); });
it('should reconnect when user changes', () => { it('should reconnect when user changes', async () => {
let oldWebSocket;
createDefaultStreamer(); createDefaultStreamer();
return activeStreamer await activeStreamer.connect();
.connect()
.then(() => { const oldWebSocket = fakeWebSocket;
oldWebSocket = fakeWebSocket; fakeStore.profile.returns({ userid: 'somebody' });
fakeStore.profile.returns({ userid: 'somebody' }); fakeStore.setState({});
return fakeStore.setState({});
}) await delay(0);
.then(() => {
assert.ok(oldWebSocket.didClose); assert.ok(oldWebSocket.didClose);
assert.ok(!fakeWebSocket.didClose); assert.ok(!fakeWebSocket.didClose);
});
}); });
it('should reconnect after unexpected disconnection', async () => { it('should reconnect after unexpected disconnection', async () => {
...@@ -290,7 +291,9 @@ describe('StreamerService', () => { ...@@ -290,7 +291,9 @@ describe('StreamerService', () => {
// Wait for reconnection to happen. // Wait for reconnection to happen.
clock.tick(3000); clock.tick(3000);
await Promise.resolve(); clock.restore();
await delay(0);
assert.lengthOf(fakeWebSockets, 2); assert.lengthOf(fakeWebSockets, 2);
}); });
......
...@@ -50,7 +50,6 @@ ...@@ -50,7 +50,6 @@
* @prop {string} oauthClientId * @prop {string} oauthClientId
* @prop {string[]} rpcAllowedOrigins * @prop {string[]} rpcAllowedOrigins
* @prop {SentryConfig} [sentry] * @prop {SentryConfig} [sentry]
* @prop {string} [websocketUrl]
*/ */
/** /**
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment