Extract route handling into view model
This is a refactoring for just 1 of ~50 routes. Once this is reviewed
and submitted we will follow up with the 49 routes.
The core of all route handlers is to convert a URL into a view model
state. We think that it is vital to have the code for converting state
into URL and back in the same file. We already have the create...Url()
functions in view model classes. We would also like to have urlToState()
functions in the same file.
It is also beneficial to have the patterns for matching a URL and the
function for converting the URL into a state object very close to each
other. Let's create the notion of a `Route` for that. This is just
`pattern` and `createState()` in one object.
Most of the current handlers just use the `params` and the `querystring`
for creating a view state, and then they call `this.setState()` and
`viewModel.setState()`. Let's create a new method `mapRouteState()`
that is similar to what `mapRoute()` does, but caters for the newly
introduced concepts above.
There are tons of things that could be further refactored in the router,
but we are trying to be focused with this change, and only do one thing.
We are happy to add further TODOs and comments though, if reviewers have
ideas or questions.
One of the goals of this change is also to address the fragile handling
of repo routes. It will be very beneficial for that effort to have all
the handlers and tests in one place.
Release-Notes: skip
Change-Id: I537733c099660470019840ab4614a751dd5ec1d9
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index bcf6937..fac7a83 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -49,6 +49,7 @@
AdminChildView,
AdminViewModel,
AdminViewState,
+ PLUGIN_LIST_ROUTE,
} from '../../../models/views/admin';
import {
AgreementViewModel,
@@ -99,6 +100,8 @@
isInBaseOfPatchRange,
} from '../../../utils/comment-util';
import {isFileUnchanged} from '../../../embed/diff/gr-diff/gr-diff-utils';
+import {Route, ViewState} from '../../../models/views/base';
+import {Model} from '../../../models/model';
const RoutePattern = {
ROOT: '/',
@@ -184,8 +187,6 @@
PLUGINS: /^\/plugins\/(.+)$/,
- PLUGIN_LIST: /^\/admin\/plugins(\/)?$/,
-
// Matches /admin/plugins[,<offset>][/].
PLUGIN_LIST_OFFSET: /^\/admin\/plugins(,(\d+))?(\/)?$/,
PLUGIN_LIST_FILTER: '/admin/plugins/q/filter::filter',
@@ -369,6 +370,7 @@
}
setState(state: AppElementParams) {
+ // TODO: Move this logic into the change model.
if ('repo' in state && state.repo !== undefined && 'changeNum' in state)
this.restApiService.setInProjectLookup(state.changeNum, state.repo);
@@ -488,6 +490,9 @@
* route is matched, the handler will be executed with `this` referring
* to the component. Its return value will be discarded so that it does
* not interfere with page.js.
+ * TODO: Get rid of this parameter. This is really not something that the
+ * router wants to be concerned with. The reporting service and the view
+ * models should figure that out between themselves.
* @param authRedirect If true, then auth is checked before
* executing the handler. If the user is not logged in, it will redirect
* to the login flow and the handler will not be executed. The login
@@ -515,6 +520,32 @@
}
/**
+ * Convenience wrapper of `mapRoute()` for when you have a `Route` object that
+ * can deal with state creation. Takes care of setting the view model state,
+ * which is currently duplicated lots of times for direct callers of
+ * `mapRoute()`.
+ */
+ mapRouteState<T extends ViewState>(
+ route: Route<T>,
+ viewModel: Model<T | undefined>,
+ handlerName: string,
+ authRedirect?: boolean
+ ) {
+ const handler = (ctx: PageContext) => {
+ const state = route.createState(ctx);
+ // Note that order is important: `this.setState()` must be called before
+ // `viewModel.setState()`. Otherwise the chain of model subscriptions
+ // would be very different. Some views may want app element to swap the
+ // top level view first. Also, `this.setState()` has some special change
+ // view model resetting logic. Eventually the order might not be important
+ // anymore, but be careful! :-)
+ this.setState(state as AppElementParams);
+ viewModel.setState(state);
+ };
+ this.mapRoute(route.urlPattern, handlerName, handler, authRedirect);
+ }
+
+ /**
* This is similar to letting the browser navigate to this URL when the user
* clicks it, or to just setting `window.location.href` directly.
*
@@ -814,10 +845,10 @@
true
);
- this.mapRoute(
- RoutePattern.PLUGIN_LIST,
+ this.mapRouteState(
+ PLUGIN_LIST_ROUTE,
+ this.adminViewModel,
'handlePluginListRoute',
- ctx => this.handlePluginListRoute(ctx),
true
);
@@ -1399,16 +1430,6 @@
this.adminViewModel.setState(state);
}
- handlePluginListRoute(_: PageContext) {
- const state: AdminViewState = {
- view: GerritView.ADMIN,
- adminView: AdminChildView.PLUGINS,
- };
- // Note that router model view must be updated before view models.
- this.setState(state);
- this.adminViewModel.setState(state);
- }
-
handleQueryRoute(ctx: PageContext) {
const state: Partial<SearchViewState> = {
view: GerritView.SEARCH,
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
index d8761bf..112c776 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -1065,14 +1065,6 @@
filter: 'foo',
});
});
-
- test('handlePluginListRoute', () => {
- const ctx = createPageContext();
- assertctxToParams(ctx, 'handlePluginListRoute', {
- view: GerritView.ADMIN,
- adminView: AdminChildView.PLUGINS,
- });
- });
});
suite('change/diff routes', () => {
diff --git a/polygerrit-ui/app/elements/gr-app-types.ts b/polygerrit-ui/app/elements/gr-app-types.ts
index 0261992..68e7309 100644
--- a/polygerrit-ui/app/elements/gr-app-types.ts
+++ b/polygerrit-ui/app/elements/gr-app-types.ts
@@ -28,6 +28,7 @@
justRegistered: boolean;
}
+// TODO: Get rid of this type. <gr-app-element> needs to be refactored for that.
export type AppElementParams =
| DashboardViewState
| GroupViewState
diff --git a/polygerrit-ui/app/models/views/admin.ts b/polygerrit-ui/app/models/views/admin.ts
index 3380637..de9ec5d 100644
--- a/polygerrit-ui/app/models/views/admin.ts
+++ b/polygerrit-ui/app/models/views/admin.ts
@@ -7,7 +7,18 @@
import {getBaseUrl} from '../../utils/url-util';
import {define} from '../dependency';
import {Model} from '../model';
-import {ViewState} from './base';
+import {Route, ViewState} from './base';
+
+export const PLUGIN_LIST_ROUTE: Route<AdminViewState> = {
+ urlPattern: /^\/admin\/plugins(\/)?$/,
+ createState: () => {
+ const state: AdminViewState = {
+ view: GerritView.ADMIN,
+ adminView: AdminChildView.PLUGINS,
+ };
+ return state;
+ },
+};
export enum AdminChildView {
REPOS = 'gr-repo-list',
diff --git a/polygerrit-ui/app/models/views/admin_test.ts b/polygerrit-ui/app/models/views/admin_test.ts
new file mode 100644
index 0000000..c9b7801
--- /dev/null
+++ b/polygerrit-ui/app/models/views/admin_test.ts
@@ -0,0 +1,29 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import {assert} from '@open-wc/testing';
+import {GerritView} from '../../services/router/router-model';
+import '../../test/common-test-setup';
+import {AdminChildView, PLUGIN_LIST_ROUTE} from './admin';
+
+suite('admin view model', () => {
+ suite('routes', () => {
+ test('PLUGIN_LIST', () => {
+ const {urlPattern: pattern, createState} = PLUGIN_LIST_ROUTE;
+
+ assert.isTrue(pattern.test('/admin/plugins'));
+ assert.isTrue(pattern.test('/admin/plugins/'));
+ assert.isFalse(pattern.test('admin/plugins'));
+ assert.isFalse(pattern.test('//admin/plugins'));
+ assert.isFalse(pattern.test('//admin/plugins?'));
+ assert.isFalse(pattern.test('/admin/plugins//'));
+
+ assert.deepEqual(createState({}), {
+ view: GerritView.ADMIN,
+ adminView: AdminChildView.PLUGINS,
+ });
+ });
+ });
+});
diff --git a/polygerrit-ui/app/models/views/base.ts b/polygerrit-ui/app/models/views/base.ts
index 065495d..72bec33 100644
--- a/polygerrit-ui/app/models/views/base.ts
+++ b/polygerrit-ui/app/models/views/base.ts
@@ -8,3 +8,24 @@
export interface ViewState {
view: GerritView;
}
+
+/**
+ * While we are using page.js this interface will normally be implemented by
+ * PageContext, but it helps testing and independence to have our own type
+ * here.
+ */
+export interface UrlInfo {
+ querystring?: string;
+ hash?: string;
+ /** What the regular expression matching returns. */
+ params?: {[paramIndex: string]: string};
+}
+
+/**
+ * Based on `urlPattern` knows whether a URL matches and if so, then
+ * `createState()` can produce a `ViewState` from the matched URL.
+ */
+export interface Route<T extends ViewState> {
+ urlPattern: RegExp;
+ createState: (info: UrlInfo) => T;
+}
diff --git a/polygerrit-ui/app/services/router/router-model.ts b/polygerrit-ui/app/services/router/router-model.ts
index c3c1cb6..5e2cc10 100644
--- a/polygerrit-ui/app/services/router/router-model.ts
+++ b/polygerrit-ui/app/services/router/router-model.ts
@@ -21,12 +21,17 @@
SETTINGS = 'settings',
}
+// TODO: Consider renaming this to AppElementState or something similar.
+// Or maybe RootViewState. This class does *not* model the state of the router.
export interface RouterState {
// Note that this router model view must be updated before view model state.
view?: GerritView;
}
export const routerModelToken = define<RouterModel>('router-model');
+
+// TODO: Consider renaming this to AppElementViewModel or something similar.
+// Or maybe RootViewModel. This class is *not* a view model of the router.
export class RouterModel extends Model<RouterState> {
readonly routerView$: Observable<GerritView | undefined> = select(
this.state$,
