Commit 179271b7 authored by Robert Knight's avatar Robert Knight

Convert `router` service to a native ES class

Services in `src/sidebar/services` are effectively classes. They are
currently implemented with a convention that was more common pre-ES6 where
the constructor is a function that creates the fields as local variables
and the methods using closures and returns an object that references the
closures.

This pattern has advantages, especially pre-ES6, as it avoids issues with
incorrect use of `this` and hides internal state from consumers. However
it also has downsides:

 - It is less obvious to readers that they are looking at something that
   is logically a class

 - This is not an idiom we use elsewhere in the codebase, where we use
   native classes instead

 - Static analysis tools don't support this pattern for creating a class
   as well as they support a native class. For example `TS` creates a
   named type for native classes, which is convenient to reference
   in JSDoc comments

This commit starts a process of refactoring service classes to ES
classes which are named `<Thing>Service`, using the router service as a
first example. Per recently agreed conventions, the classes are named
rather than default exports.
parent da4d67b5
...@@ -5,7 +5,7 @@ import SearchInput from './SearchInput'; ...@@ -5,7 +5,7 @@ import SearchInput from './SearchInput';
/** /**
* @typedef StreamSearchInputProps * @typedef StreamSearchInputProps
* @prop {Object} router - Injected service * @prop {import('../services/router').RouterService} router
*/ */
/** /**
......
...@@ -44,6 +44,10 @@ function setupApi(api, streamer) { ...@@ -44,6 +44,10 @@ function setupApi(api, streamer) {
/** /**
* Perform the initial fetch of groups and user profile and then set the initial * Perform the initial fetch of groups and user profile and then set the initial
* route to match the current URL. * route to match the current URL.
*
* @param {Object} groups
* @param {Object} session
* @param {import('./services/router').RouterService} router
*/ */
// @inject // @inject
function setupRoute(groups, session, router) { function setupRoute(groups, session, router) {
...@@ -102,7 +106,7 @@ import groupsService from './services/groups'; ...@@ -102,7 +106,7 @@ import groupsService from './services/groups';
import loadAnnotationsService from './services/load-annotations'; import loadAnnotationsService from './services/load-annotations';
import localStorageService from './services/local-storage'; import localStorageService from './services/local-storage';
import persistedDefaultsService from './services/persisted-defaults'; import persistedDefaultsService from './services/persisted-defaults';
import routerService from './services/router'; import { RouterService } from './services/router';
import serviceUrlService from './services/service-url'; import serviceUrlService from './services/service-url';
import sessionService from './services/session'; import sessionService from './services/session';
import streamFilterService from './services/stream-filter'; import streamFilterService from './services/stream-filter';
...@@ -140,7 +144,7 @@ function startApp(config, appEl) { ...@@ -140,7 +144,7 @@ function startApp(config, appEl) {
.register('loadAnnotationsService', loadAnnotationsService) .register('loadAnnotationsService', loadAnnotationsService)
.register('localStorage', localStorageService) .register('localStorage', localStorageService)
.register('persistedDefaults', persistedDefaultsService) .register('persistedDefaults', persistedDefaultsService)
.register('router', routerService) .register('router', RouterService)
.register('serviceUrl', serviceUrlService) .register('serviceUrl', serviceUrlService)
.register('session', sessionService) .register('session', sessionService)
.register('streamer', streamerService) .register('streamer', streamerService)
......
...@@ -5,14 +5,24 @@ import * as queryString from 'query-string'; ...@@ -5,14 +5,24 @@ import * as queryString from 'query-string';
* implied by the URL and the corresponding route state in the store. * implied by the URL and the corresponding route state in the store.
*/ */
// @inject // @inject
export default function router($window, store) { export class RouterService {
/**
* @param {Window} $window
* @param {import('../store').SidebarStore} store
*/
constructor($window, store) {
this._window = $window;
this._store = store;
this._didRegisterPopstateListener = false;
}
/** /**
* Return the name and parameters of the current route. * Return the name and parameters of the current route.
*/ */
function currentRoute() { currentRoute() {
const path = $window.location.pathname; const path = this._window.location.pathname;
const pathSegments = path.slice(1).split('/'); const pathSegments = path.slice(1).split('/');
const params = queryString.parse($window.location.search); const params = queryString.parse(this._window.location.search);
// The extension puts client resources under `/client/` to separate them // The extension puts client resources under `/client/` to separate them
// from extension-specific resources. Ignore this part. // from extension-specific resources. Ignore this part.
...@@ -49,8 +59,11 @@ export default function router($window, store) { ...@@ -49,8 +59,11 @@ export default function router($window, store) {
/** /**
* Generate a URL for a given route. * Generate a URL for a given route.
*
* @param {string} name
* @param {Object.<string,string>} params
*/ */
function routeUrl(name, params = {}) { routeUrl(name, params = {}) {
let url; let url;
const queryParams = { ...params }; const queryParams = { ...params };
...@@ -81,8 +94,6 @@ export default function router($window, store) { ...@@ -81,8 +94,6 @@ export default function router($window, store) {
return url; return url;
} }
let didRegisterPopstateListener = false;
/** /**
* Synchronize the route name and parameters in the store with the current * Synchronize the route name and parameters in the store with the current
* URL. * URL.
...@@ -90,21 +101,21 @@ export default function router($window, store) { ...@@ -90,21 +101,21 @@ export default function router($window, store) {
* The first call to this method also registers a listener for future back/forwards * The first call to this method also registers a listener for future back/forwards
* navigation in the browser. * navigation in the browser.
*/ */
function sync() { sync() {
const { route, params } = currentRoute(); const { route, params } = this.currentRoute();
store.changeRoute(route, params); this._store.changeRoute(route, params);
// Set up listener for back/forward navigation. We do this in `sync()` to // Set up listener for back/forward navigation. We do this in `sync()` to
// avoid the route being changed by a "popstate" emitted by the browser on // avoid the route being changed by a "popstate" emitted by the browser on
// document load (which Safari and Chrome do). // document load (which Safari and Chrome do).
if (!didRegisterPopstateListener) { if (!this._didRegisterPopstateListener) {
$window.addEventListener('popstate', () => { this._window.addEventListener('popstate', () => {
// All the state we need to update the route is contained in the URL, which // All the state we need to update the route is contained in the URL, which
// has already been updated at this point, so just sync the store route // has already been updated at this point, so just sync the store route
// to match the URL. // to match the URL.
sync(); this.sync();
}); });
didRegisterPopstateListener = true; this._didRegisterPopstateListener = true;
} }
} }
...@@ -112,12 +123,10 @@ export default function router($window, store) { ...@@ -112,12 +123,10 @@ export default function router($window, store) {
* Navigate to a given route. * Navigate to a given route.
* *
* @param {string} name * @param {string} name
* @param {Object} params * @param {Object.<string,string>} params
*/ */
function navigate(name, params) { navigate(name, params) {
$window.history.pushState({}, '', routeUrl(name, params)); this._window.history.pushState({}, '', this.routeUrl(name, params));
sync(); this.sync();
} }
return { sync, navigate };
} }
import EventEmitter from 'tiny-emitter'; import EventEmitter from 'tiny-emitter';
import router from '../router'; import { RouterService } from '../router';
const fixtures = [ const fixtures = [
// Sidebar for the embedded Hypothesis client. // Sidebar for the embedded Hypothesis client.
...@@ -40,12 +40,12 @@ const fixtures = [ ...@@ -40,12 +40,12 @@ const fixtures = [
}, },
]; ];
describe('router', () => { describe('RouterService', () => {
let fakeWindow; let fakeWindow;
let fakeStore; let fakeStore;
function createService() { function createService() {
return router(fakeWindow, fakeStore); return new RouterService(fakeWindow, fakeStore);
} }
function updateUrl(path, search) { function updateUrl(path, search) {
......
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