From e6ed295567b587c1561b3b779f2bc8223dc4173f Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Wed, 21 Nov 2018 17:55:18 -0800 Subject: [PATCH 01/10] Fixes path_prefix flag PR #1512 introduced a bug where it formed a complete path without using existing pathname. Previously without forming the complete path, it was treated as a relative path from current window.location.pathname. --- tensorboard/components/tf_backend/router.ts | 12 +++++++++--- .../components/tf_backend/test/backendTests.ts | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index 1619024565..9f21e4aea7 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -28,10 +28,10 @@ export interface Router { * Create a router for communicating with the TensorBoard backend. You * can pass this to `setRouter` to make it the global router. * - * @param pathPrefix {string=} The base prefix for data endpoints. + * @param pathPrefix {string} The base prefix for data endpoints. * @param demoMode {boolean=} Whether to modify urls for filesystem demo usage. */ -export function createRouter(pathPrefix = 'data', demoMode = false): Router { +export function createRouter(pathPrefix, demoMode = false): Router { if (pathPrefix[pathPrefix.length - 1] === '/') { pathPrefix = pathPrefix.slice(0, pathPrefix.length - 1); } @@ -62,7 +62,13 @@ export function createRouter(pathPrefix = 'data', demoMode = false): Router { }; }; -let _router: Router = createRouter(); +export function getDefaultRouter(): Router { + const sep = window.location.pathname.endsWith('/') ? '' : '/'; + const pathPrefix = window.location.pathname + sep + 'data'; + return createRouter(pathPrefix); +} + +let _router: Router = getDefaultRouter(); /** * @return {Router} the global router diff --git a/tensorboard/components/tf_backend/test/backendTests.ts b/tensorboard/components/tf_backend/test/backendTests.ts index 6dea12e15c..5171b50632 100644 --- a/tensorboard/components/tf_backend/test/backendTests.ts +++ b/tensorboard/components/tf_backend/test/backendTests.ts @@ -81,6 +81,23 @@ describe('backend tests', () => { assert.equal(r.runs(), '/foo/runs'); }); + describe('default router', () => { + beforeEach(function() { + this.router = getDefaultRouter(); + }); + + it('makes sure test is not degenerate', function() { + // Test becomes degenerate if location.pathname is empty. + assert.notEqual('/', window.location.pathname); + }); + + it('creates a URL including the pathname', function() { + assert.equal( + this.router.runs(), + `${window.location.pathname}/data/runs`); + }); + }); + describe('prod mode', () => { beforeEach(function() { this.router = createRouter(base, /*demoMode=*/false); From 7fc49bac587bf776349cf242ee885a08bda3441e Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Mon, 26 Nov 2018 20:14:40 -0800 Subject: [PATCH 02/10] Use two arg constructor for URL for relative path --- tensorboard/components/tf_backend/router.ts | 16 +--- .../tf_backend/test/backendTests.ts | 93 ++++++++----------- 2 files changed, 45 insertions(+), 64 deletions(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index 9f21e4aea7..1c73ac4d69 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -28,10 +28,10 @@ export interface Router { * Create a router for communicating with the TensorBoard backend. You * can pass this to `setRouter` to make it the global router. * - * @param pathPrefix {string} The base prefix for data endpoints. + * @param pathPrefix {string=} The base prefix for data endpoints. * @param demoMode {boolean=} Whether to modify urls for filesystem demo usage. */ -export function createRouter(pathPrefix, demoMode = false): Router { +export function createRouter(pathPrefix = 'data', demoMode = false): Router { if (pathPrefix[pathPrefix.length - 1] === '/') { pathPrefix = pathPrefix.slice(0, pathPrefix.length - 1); } @@ -62,13 +62,7 @@ export function createRouter(pathPrefix, demoMode = false): Router { }; }; -export function getDefaultRouter(): Router { - const sep = window.location.pathname.endsWith('/') ? '' : '/'; - const pathPrefix = window.location.pathname + sep + 'data'; - return createRouter(pathPrefix); -} - -let _router: Router = getDefaultRouter(); +let _router: Router = createRouter(); /** * @return {Router} the global router @@ -94,9 +88,9 @@ export function setRouter(router: Router): void { function createProdPath(pathPrefix: string, path: string, ext: string, params?: URLSearchParams): string { - const url = new URL(window.location.origin); // Use URL to normalize pathPrefix with leading slash and without. - url.pathname = pathPrefix + path; + const maybeRelativePath = pathPrefix + path; + const url = new URL(maybeRelativePath, window.location.href); if (params) url.search = params.toString(); return url.pathname + url.search; } diff --git a/tensorboard/components/tf_backend/test/backendTests.ts b/tensorboard/components/tf_backend/test/backendTests.ts index 5171b50632..9d98b03115 100644 --- a/tensorboard/components/tf_backend/test/backendTests.ts +++ b/tensorboard/components/tf_backend/test/backendTests.ts @@ -76,54 +76,39 @@ describe('backend tests', () => { }); describe('router', () => { - it('removes trailing slash from base route', () => { - const r = createRouter('foo/'); - assert.equal(r.runs(), '/foo/runs'); - }); - - describe('default router', () => { - beforeEach(function() { - this.router = getDefaultRouter(); - }); - - it('makes sure test is not degenerate', function() { - // Test becomes degenerate if location.pathname is empty. - assert.notEqual('/', window.location.pathname); - }); - - it('creates a URL including the pathname', function() { - assert.equal( - this.router.runs(), - `${window.location.pathname}/data/runs`); - }); - }); - describe('prod mode', () => { beforeEach(function() { this.router = createRouter(base, /*demoMode=*/false); }); - it('ignores leading slash in pathPrefix', () => { + function assertRelativePath(actual, expectedRelativePath) { + // Path prefix is /tf-backend/test should match path of the + // test_web_library in the test build target. + assert.equal(actual, `/tf-backend/test/${expectedRelativePath}`); + } + + it('leading slash in pathPrefix is an absolute path', () => { const router = createRouter('/data/', /*demoMode=*/false); assert.equal(router.runs(), '/data/runs'); }); - it('returns leading slash when pathPrefix omits one', () => { + it('returns complete pathname when pathPrefix omits slash', () => { const router = createRouter('data/', /*demoMode=*/false); - assert.equal(router.runs(), '/data/runs'); + assertRelativePath(router.runs(), 'data/runs'); }); - it('does not strip leading slash if pathPrefix has more than one', () => { - const router = createRouter('///data/', /*demoMode=*/false); - assert.equal(router.runs(), '///data/runs'); + it('treats more than two slashes as absolute url', () => { + const router = createRouter('///data/hello', /*demoMode=*/false); + // This becomes 'http://data/hello/runs' + assert.equal(router.runs(), '/hello/runs'); }); it('returns correct value for #environment', function() { - assert.equal(this.router.environment(), '/data/environment'); + assertRelativePath(this.router.environment(), 'data/environment'); }); it('returns correct value for #experiments', function() { - assert.equal(this.router.experiments(), '/data/experiments'); + assertRelativePath(this.router.experiments(), 'data/experiments'); }); it('returns correct value for #isDemoMode', function() { @@ -132,68 +117,69 @@ describe('backend tests', () => { describe('#pluginRoute', () => { it('encodes slash correctly', function() { - assert.equal( + assertRelativePath( this.router.pluginRoute('scalars', '/scalar'), - '/data/plugin/scalars/scalar'); + 'data/plugin/scalars/scalar'); }); it('encodes query param correctly', function() { - assert.equal( + assertRelativePath( this.router.pluginRoute( 'scalars', '/a', createSearchParam({b: 'c', d: ['1', '2']})), - '/data/plugin/scalars/a?b=c&d=1&d=2'); + 'data/plugin/scalars/a?b=c&d=1&d=2'); }); it('encodes parenthesis correctly', function() { - assert.equal( + assertRelativePath( this.router.pluginRoute('scalars', '/a', createSearchParam({foo: '()'})), - '/data/plugin/scalars/a?foo=%28%29'); + 'data/plugin/scalars/a?foo=%28%29'); }); it('encodes query param the same as #addParams', function() { - assert.equal( + assertRelativePath( this.router.pluginRoute( 'scalars', '/a', createSearchParam({b: 'c', d: ['1']})), - addParams('/data/plugin/scalars/a', {b: 'c', d: ['1']})); - assert.equal( + addParams('data/plugin/scalars/a', {b: 'c', d: ['1']})); + assertRelativePath( this.router.pluginRoute( 'scalars', '/a', createSearchParam({foo: '()'})), - addParams('/data/plugin/scalars/a', {foo: '()'})); + addParams('data/plugin/scalars/a', {foo: '()'})); }); it('ignores custom extension', function() { - assert.equal( + assertRelativePath( this.router.pluginRoute('scalars', '/a', undefined, 'meow'), - '/data/plugin/scalars/a'); + 'data/plugin/scalars/a'); }); }); it('returns correct value for #pluginsListing', function() { - assert.equal(this.router.pluginsListing(), '/data/plugins_listing'); + assertRelativePath( + this.router.pluginsListing(), 'data/plugins_listing'); }); it('returns correct value for #runs', function() { - assert.equal(this.router.runs(), '/data/runs'); + assertRelativePath(this.router.runs(), 'data/runs'); }); it('returns correct value for #runsForExperiment', function() { // No experiment id is passed. - assert.equal( + assertRelativePath( this.router.runsForExperiment(''), - '/data/experiment_runs'); - assert.equal( + 'data/experiment_runs'); + assertRelativePath( this.router.runsForExperiment('1'), - '/data/experiment_runs?experiment=1'); - assert.equal( + 'data/experiment_runs?experiment=1'); + assertRelativePath( this.router.runsForExperiment('1&foo=false'), - '/data/experiment_runs?experiment=1%26foo%3Dfalse'); + 'data/experiment_runs?experiment=1%26foo%3Dfalse'); }); }); @@ -202,18 +188,19 @@ describe('backend tests', () => { this.router = createRouter(base, /*demoMode=*/true); }); - it('ignores leading slash in pathPrefix', () => { + it('treats pathPrefix with leading slash as absolute path', () => { const router = createRouter('/data/', /*demoMode=*/true); assert.equal(router.runs(), '/data/runs.json'); }); - it('returns leading slash when pathPrefix omits one', () => { + it('treats pathPrefix without leading slash as absolute path', () => { const router = createRouter('data/', /*demoMode=*/true); assert.equal(router.runs(), '/data/runs.json'); }); - it('does not strip leading slash if pathPrefix has more than one', () => { + it('does not form absolute url with multiple leading slashes', () => { const router = createRouter('///data/', /*demoMode=*/true); + // For prod url, this would be http://data/runs assert.equal(router.runs(), '///data/runs.json'); }); From 8b3b3bfdba4c4ec1f08e78db96f41df78d299eaa Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Tue, 27 Nov 2018 10:33:57 -0800 Subject: [PATCH 03/10] fixed the router typing --- tensorboard/components/tf_backend/router.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index 1c73ac4d69..5cbd9d1b42 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -18,7 +18,8 @@ export interface Router { environment: () => string; experiments: () => string; isDemoMode: () => boolean; - pluginRoute: (pluginName: string, route: string) => string; + pluginRoute: (pluginName: string, route: string, + params?: URLSearchParams, demoCustomExt?: string) => string; pluginsListing: () => string; runs: () => string; runsForExperiment: (id: tf_backend.ExperimentId) => string; From 7b9ea26510c8f491a244c0c07614de431d26e8e1 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Tue, 27 Nov 2018 13:36:00 -0800 Subject: [PATCH 04/10] Not using mocha scope for TypeScript type inference --- .../tf_backend/test/backendTests.ts | 73 ++++++++----------- 1 file changed, 31 insertions(+), 42 deletions(-) diff --git a/tensorboard/components/tf_backend/test/backendTests.ts b/tensorboard/components/tf_backend/test/backendTests.ts index 9d98b03115..cd573e7bcf 100644 --- a/tensorboard/components/tf_backend/test/backendTests.ts +++ b/tensorboard/components/tf_backend/test/backendTests.ts @@ -77,8 +77,9 @@ describe('backend tests', () => { describe('router', () => { describe('prod mode', () => { - beforeEach(function() { - this.router = createRouter(base, /*demoMode=*/false); + let router: Router; + beforeEach(() => { + router = createRouter(base, /*demoMode=*/false); }); function assertRelativePath(actual, expectedRelativePath) { @@ -104,27 +105,27 @@ describe('backend tests', () => { }); it('returns correct value for #environment', function() { - assertRelativePath(this.router.environment(), 'data/environment'); + assertRelativePath(router.environment(), 'data/environment'); }); it('returns correct value for #experiments', function() { - assertRelativePath(this.router.experiments(), 'data/experiments'); + assertRelativePath(router.experiments(), 'data/experiments'); }); it('returns correct value for #isDemoMode', function() { - assert.equal(this.router.isDemoMode(), false); + assert.equal(router.isDemoMode(), false); }); describe('#pluginRoute', () => { it('encodes slash correctly', function() { assertRelativePath( - this.router.pluginRoute('scalars', '/scalar'), + router.pluginRoute('scalars', '/scalar'), 'data/plugin/scalars/scalar'); }); it('encodes query param correctly', function() { assertRelativePath( - this.router.pluginRoute( + router.pluginRoute( 'scalars', '/a', createSearchParam({b: 'c', d: ['1', '2']})), @@ -133,20 +134,20 @@ describe('backend tests', () => { it('encodes parenthesis correctly', function() { assertRelativePath( - this.router.pluginRoute('scalars', '/a', + router.pluginRoute('scalars', '/a', createSearchParam({foo: '()'})), 'data/plugin/scalars/a?foo=%28%29'); }); it('encodes query param the same as #addParams', function() { assertRelativePath( - this.router.pluginRoute( + router.pluginRoute( 'scalars', '/a', createSearchParam({b: 'c', d: ['1']})), addParams('data/plugin/scalars/a', {b: 'c', d: ['1']})); assertRelativePath( - this.router.pluginRoute( + router.pluginRoute( 'scalars', '/a', createSearchParam({foo: '()'})), @@ -155,37 +156,32 @@ describe('backend tests', () => { it('ignores custom extension', function() { assertRelativePath( - this.router.pluginRoute('scalars', '/a', undefined, 'meow'), + router.pluginRoute('scalars', '/a', undefined, 'meow'), 'data/plugin/scalars/a'); }); }); it('returns correct value for #pluginsListing', function() { assertRelativePath( - this.router.pluginsListing(), 'data/plugins_listing'); + router.pluginsListing(), 'data/plugins_listing'); }); it('returns correct value for #runs', function() { - assertRelativePath(this.router.runs(), 'data/runs'); + assertRelativePath(router.runs(), 'data/runs'); }); it('returns correct value for #runsForExperiment', function() { - // No experiment id is passed. - assertRelativePath( - this.router.runsForExperiment(''), - 'data/experiment_runs'); assertRelativePath( - this.router.runsForExperiment('1'), + router.runsForExperiment(1), 'data/experiment_runs?experiment=1'); - assertRelativePath( - this.router.runsForExperiment('1&foo=false'), - 'data/experiment_runs?experiment=1%26foo%3Dfalse'); }); }); describe('demoMode', () => { - beforeEach( function() { - this.router = createRouter(base, /*demoMode=*/true); + let router: Router; + + beforeEach(() => { + router = createRouter(base, /*demoMode=*/true); }); it('treats pathPrefix with leading slash as absolute path', () => { @@ -205,27 +201,27 @@ describe('backend tests', () => { }); it('returns correct value for #environment', function() { - assert.equal(this.router.environment(), '/data/environment.json'); + assert.equal(router.environment(), '/data/environment.json'); }); it('returns correct value for #experiments', function() { - assert.equal(this.router.experiments(), '/data/experiments.json'); + assert.equal(router.experiments(), '/data/experiments.json'); }); it('returns correct value for #isDemoMode', function() { - assert.equal(this.router.isDemoMode(), true); + assert.equal(router.isDemoMode(), true); }); describe('#pluginRoute', () => { it('encodes slash correctly', function() { assert.equal( - this.router.pluginRoute('scalars', '/scalar'), + router.pluginRoute('scalars', '/scalar'), '/data/scalars_scalar.json'); }); it('encodes query param correctly', function() { assert.equal( - this.router.pluginRoute( + router.pluginRoute( 'scalars', '/a', createSearchParam({b: 'c', d: ['1', '2']})), @@ -234,7 +230,7 @@ describe('backend tests', () => { it('encodes parenthesis correctly', function() { assert.equal( - this.router.pluginRoute( + router.pluginRoute( 'scalars', '/a', createSearchParam({foo: '()'})), @@ -243,38 +239,31 @@ describe('backend tests', () => { it('uses custom extension if provided', function() { assert.equal( - this.router.pluginRoute('scalars', '/a', undefined, ''), + router.pluginRoute('scalars', '/a', undefined, ''), '/data/scalars_a'); assert.equal( - this.router.pluginRoute('scalars', '/a', undefined, '.meow'), + router.pluginRoute('scalars', '/a', undefined, '.meow'), '/data/scalars_a.meow'); assert.equal( - this.router.pluginRoute('scalars', '/a'), + router.pluginRoute('scalars', '/a'), '/data/scalars_a.json'); }); }); it('returns correct value for #pluginsListing', function() { assert.equal( - this.router.pluginsListing(), + router.pluginsListing(), '/data/plugins_listing.json'); }); it('returns correct value for #runs', function() { - assert.equal(this.router.runs(), '/data/runs.json'); + assert.equal(router.runs(), '/data/runs.json'); }); it('returns correct value for #runsForExperiment', function() { - // No experiment id is passed. - assert.equal( - this.router.runsForExperiment(''), - '/data/experiment_runs.json'); assert.equal( - this.router.runsForExperiment('1'), + router.runsForExperiment(1), '/data/experiment_runs_experiment_1.json'); - assert.equal( - this.router.runsForExperiment('1&foo=false'), - '/data/experiment_runs_experiment_1_26foo_3Dfalse.json'); }); }); }); From 789f11b6eb39714fdd8527666ea416013ec406f5 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Tue, 27 Nov 2018 13:38:11 -0800 Subject: [PATCH 05/10] Change to arrow func to denote that we will not be using mocha scope --- .../tf_backend/test/backendTests.ts | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/tensorboard/components/tf_backend/test/backendTests.ts b/tensorboard/components/tf_backend/test/backendTests.ts index cd573e7bcf..7938eac8c6 100644 --- a/tensorboard/components/tf_backend/test/backendTests.ts +++ b/tensorboard/components/tf_backend/test/backendTests.ts @@ -104,26 +104,26 @@ describe('backend tests', () => { assert.equal(router.runs(), '/hello/runs'); }); - it('returns correct value for #environment', function() { + it('returns correct value for #environment', () => { assertRelativePath(router.environment(), 'data/environment'); }); - it('returns correct value for #experiments', function() { + it('returns correct value for #experiments', () => { assertRelativePath(router.experiments(), 'data/experiments'); }); - it('returns correct value for #isDemoMode', function() { + it('returns correct value for #isDemoMode', () => { assert.equal(router.isDemoMode(), false); }); describe('#pluginRoute', () => { - it('encodes slash correctly', function() { + it('encodes slash correctly', () => { assertRelativePath( router.pluginRoute('scalars', '/scalar'), 'data/plugin/scalars/scalar'); }); - it('encodes query param correctly', function() { + it('encodes query param correctly', () => { assertRelativePath( router.pluginRoute( 'scalars', @@ -132,14 +132,14 @@ describe('backend tests', () => { 'data/plugin/scalars/a?b=c&d=1&d=2'); }); - it('encodes parenthesis correctly', function() { + it('encodes parenthesis correctly', () => { assertRelativePath( router.pluginRoute('scalars', '/a', createSearchParam({foo: '()'})), 'data/plugin/scalars/a?foo=%28%29'); }); - it('encodes query param the same as #addParams', function() { + it('encodes query param the same as #addParams', () => { assertRelativePath( router.pluginRoute( 'scalars', @@ -154,23 +154,23 @@ describe('backend tests', () => { addParams('data/plugin/scalars/a', {foo: '()'})); }); - it('ignores custom extension', function() { + it('ignores custom extension', () => { assertRelativePath( router.pluginRoute('scalars', '/a', undefined, 'meow'), 'data/plugin/scalars/a'); }); }); - it('returns correct value for #pluginsListing', function() { + it('returns correct value for #pluginsListing', () => { assertRelativePath( router.pluginsListing(), 'data/plugins_listing'); }); - it('returns correct value for #runs', function() { + it('returns correct value for #runs', () => { assertRelativePath(router.runs(), 'data/runs'); }); - it('returns correct value for #runsForExperiment', function() { + it('returns correct value for #runsForExperiment', () => { assertRelativePath( router.runsForExperiment(1), 'data/experiment_runs?experiment=1'); @@ -200,26 +200,26 @@ describe('backend tests', () => { assert.equal(router.runs(), '///data/runs.json'); }); - it('returns correct value for #environment', function() { + it('returns correct value for #environment', () => { assert.equal(router.environment(), '/data/environment.json'); }); - it('returns correct value for #experiments', function() { + it('returns correct value for #experiments', () => { assert.equal(router.experiments(), '/data/experiments.json'); }); - it('returns correct value for #isDemoMode', function() { + it('returns correct value for #isDemoMode', () => { assert.equal(router.isDemoMode(), true); }); describe('#pluginRoute', () => { - it('encodes slash correctly', function() { + it('encodes slash correctly', () => { assert.equal( router.pluginRoute('scalars', '/scalar'), '/data/scalars_scalar.json'); }); - it('encodes query param correctly', function() { + it('encodes query param correctly', () => { assert.equal( router.pluginRoute( 'scalars', @@ -228,7 +228,7 @@ describe('backend tests', () => { '/data/scalars_a_b_c_d_1_d_2.json'); }); - it('encodes parenthesis correctly', function() { + it('encodes parenthesis correctly', () => { assert.equal( router.pluginRoute( 'scalars', @@ -237,7 +237,7 @@ describe('backend tests', () => { '/data/scalars_a_foo__28_29.json'); }); - it('uses custom extension if provided', function() { + it('uses custom extension if provided', () => { assert.equal( router.pluginRoute('scalars', '/a', undefined, ''), '/data/scalars_a'); @@ -250,17 +250,17 @@ describe('backend tests', () => { }); }); - it('returns correct value for #pluginsListing', function() { + it('returns correct value for #pluginsListing', () => { assert.equal( router.pluginsListing(), '/data/plugins_listing.json'); }); - it('returns correct value for #runs', function() { + it('returns correct value for #runs', () => { assert.equal(router.runs(), '/data/runs.json'); }); - it('returns correct value for #runsForExperiment', function() { + it('returns correct value for #runsForExperiment', () => { assert.equal( router.runsForExperiment(1), '/data/experiment_runs_experiment_1.json'); From 9f0a7b47175a99941c17c1e61a1be5582fba1bb9 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Tue, 27 Nov 2018 15:06:02 -0800 Subject: [PATCH 06/10] Revert back to old behavior and added more tests --- tensorboard/components/tf_backend/router.ts | 53 ++++++++++--------- .../tf_backend/test/backendTests.ts | 50 +++++++++-------- 2 files changed, 58 insertions(+), 45 deletions(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index 5cbd9d1b42..efa5c41b90 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -29,33 +29,33 @@ export interface Router { * Create a router for communicating with the TensorBoard backend. You * can pass this to `setRouter` to make it the global router. * - * @param pathPrefix {string=} The base prefix for data endpoints. + * @param dataDir {string=} The base prefix for data endpoints. * @param demoMode {boolean=} Whether to modify urls for filesystem demo usage. */ -export function createRouter(pathPrefix = 'data', demoMode = false): Router { - if (pathPrefix[pathPrefix.length - 1] === '/') { - pathPrefix = pathPrefix.slice(0, pathPrefix.length - 1); +export function createRouter(dataDir = 'data', demoMode = false): Router { + if (dataDir[dataDir.length - 1] === '/') { + dataDir = dataDir.slice(0, dataDir.length - 1); } const createPath = demoMode ? createDemoPath : createProdPath; const ext = demoMode ? '.json' : ''; return { - environment: () => createPath(pathPrefix, '/environment', ext), - experiments: () => createPath(pathPrefix, '/experiments', ext), + environment: () => createPath(dataDir, '/environment', ext), + experiments: () => createPath(dataDir, '/experiments', ext), isDemoMode: () => demoMode, pluginRoute: (pluginName: string, route: string, params?: URLSearchParams, demoCustomExt = ext): string => { return createPath( - demoMode ? pathPrefix : pathPrefix + '/plugin', + demoMode ? dataDir : dataDir + '/plugin', `/${pluginName}${route}`, demoCustomExt, params); }, - pluginsListing: () => createPath(pathPrefix, '/plugins_listing', ext), - runs: () => createPath(pathPrefix, '/runs', ext), + pluginsListing: () => createPath(dataDir, '/plugins_listing', ext), + runs: () => createPath(dataDir, '/runs', ext), runsForExperiment: id => { return createPath( - pathPrefix, + dataDir, '/experiment_runs', ext, createSearchParam({experiment: String(id)})); @@ -87,13 +87,14 @@ export function setRouter(router: Router): void { _router = router; } -function createProdPath(pathPrefix: string, path: string, +function createProdPath(dataDir: string, route: string, ext: string, params?: URLSearchParams): string { - // Use URL to normalize pathPrefix with leading slash and without. - const maybeRelativePath = pathPrefix + path; - const url = new URL(maybeRelativePath, window.location.href); - if (params) url.search = params.toString(); - return url.pathname + url.search; + let relativePath = dataDir + route; + if (params) { + const delimiter = route.includes('?') ? '&' : '?'; + relativePath += delimiter + params.toString(); + } + return relativePath; } /** @@ -102,15 +103,19 @@ function createProdPath(pathPrefix: string, path: string, * > createDemoPath('a', '/b', '.json', {a: 1}) * < '/a/b_a_1.json' */ -function createDemoPath(pathPrefix: string, path: string, - ext: string, params?: URLSearchParams): string { +function createDemoPath(dataDir: string, route: string, + ext: string, params: URLSearchParams = new URLSearchParams()): string { // First, parse the path in a safe manner by constructing a URL. We don't // trust the path supplied by consumer. - const prefixLessUrl = new URL(`${window.location.origin}/${path}`); - let {pathname: normalizedPath} = prefixLessUrl; - const encodedQueryParam = params ? - params.toString().replace(/[&=%]/g, '_') : ''; + const absRoute = route.startsWith('/') ? route : '/' + route; + const absUrl = new URL(route, window.location.href); + let {pathname: normalizedPath, searchParams: normalizedSearchParams} = absUrl; + const queryParam = [normalizedSearchParams, params] + .map(p => String(p)) + .filter(Boolean) + .join('&'); + const encodedQueryParam = queryParam.replace(/[&=%]/g, '_'); // Strip leading slashes. normalizedPath = normalizedPath.replace(/^\/+/g, ''); @@ -118,10 +123,10 @@ function createDemoPath(pathPrefix: string, path: string, normalizedPath = normalizedPath.replace(/\//g, '_'); // Add query parameter as path if it is present. if (encodedQueryParam) normalizedPath += `_${encodedQueryParam}`; - const url = new URL(window.location.origin); + const url = new URL(window.location.href); // All demo data are serialized in JSON format. - url.pathname = `${pathPrefix}/${normalizedPath}${ext}`; + url.pathname = `${dataDir}/${normalizedPath}${ext}`; return url.pathname + url.search; } diff --git a/tensorboard/components/tf_backend/test/backendTests.ts b/tensorboard/components/tf_backend/test/backendTests.ts index 7938eac8c6..e26348babc 100644 --- a/tensorboard/components/tf_backend/test/backendTests.ts +++ b/tensorboard/components/tf_backend/test/backendTests.ts @@ -82,12 +82,6 @@ describe('backend tests', () => { router = createRouter(base, /*demoMode=*/false); }); - function assertRelativePath(actual, expectedRelativePath) { - // Path prefix is /tf-backend/test should match path of the - // test_web_library in the test build target. - assert.equal(actual, `/tf-backend/test/${expectedRelativePath}`); - } - it('leading slash in pathPrefix is an absolute path', () => { const router = createRouter('/data/', /*demoMode=*/false); assert.equal(router.runs(), '/data/runs'); @@ -95,21 +89,21 @@ describe('backend tests', () => { it('returns complete pathname when pathPrefix omits slash', () => { const router = createRouter('data/', /*demoMode=*/false); - assertRelativePath(router.runs(), 'data/runs'); + assert.equal(router.runs(), 'data/runs'); }); - it('treats more than two slashes as absolute url', () => { + it('does not prune many leading slashes that forms full url', () => { const router = createRouter('///data/hello', /*demoMode=*/false); // This becomes 'http://data/hello/runs' - assert.equal(router.runs(), '/hello/runs'); + assert.equal(router.runs(), '///data/hello/runs'); }); it('returns correct value for #environment', () => { - assertRelativePath(router.environment(), 'data/environment'); + assert.equal(router.environment(), 'data/environment'); }); it('returns correct value for #experiments', () => { - assertRelativePath(router.experiments(), 'data/experiments'); + assert.equal(router.experiments(), 'data/experiments'); }); it('returns correct value for #isDemoMode', () => { @@ -118,13 +112,13 @@ describe('backend tests', () => { describe('#pluginRoute', () => { it('encodes slash correctly', () => { - assertRelativePath( + assert.equal( router.pluginRoute('scalars', '/scalar'), 'data/plugin/scalars/scalar'); }); it('encodes query param correctly', () => { - assertRelativePath( + assert.equal( router.pluginRoute( 'scalars', '/a', @@ -133,20 +127,27 @@ describe('backend tests', () => { }); it('encodes parenthesis correctly', () => { - assertRelativePath( + assert.equal( router.pluginRoute('scalars', '/a', - createSearchParam({foo: '()'})), + createSearchParam({foo: '()'})), 'data/plugin/scalars/a?foo=%28%29'); }); + it('deals with existing query param correctly', () => { + assert.equal( + router.pluginRoute('scalars', '/a?foo=bar', + createSearchParam({hello: 'world'})), + 'data/plugin/scalars/a?foo=bar&hello=world'); + }); + it('encodes query param the same as #addParams', () => { - assertRelativePath( + assert.equal( router.pluginRoute( 'scalars', '/a', createSearchParam({b: 'c', d: ['1']})), addParams('data/plugin/scalars/a', {b: 'c', d: ['1']})); - assertRelativePath( + assert.equal( router.pluginRoute( 'scalars', '/a', @@ -155,23 +156,23 @@ describe('backend tests', () => { }); it('ignores custom extension', () => { - assertRelativePath( + assert.equal( router.pluginRoute('scalars', '/a', undefined, 'meow'), 'data/plugin/scalars/a'); }); }); it('returns correct value for #pluginsListing', () => { - assertRelativePath( + assert.equal( router.pluginsListing(), 'data/plugins_listing'); }); it('returns correct value for #runs', () => { - assertRelativePath(router.runs(), 'data/runs'); + assert.equal(router.runs(), 'data/runs'); }); it('returns correct value for #runsForExperiment', () => { - assertRelativePath( + assert.equal( router.runsForExperiment(1), 'data/experiment_runs?experiment=1'); }); @@ -228,6 +229,13 @@ describe('backend tests', () => { '/data/scalars_a_b_c_d_1_d_2.json'); }); + it('deals with existing query param correctly', () => { + assert.equal( + router.pluginRoute('scalars', '/a?foo=bar', + createSearchParam({hello: 'world'})), + '/data/scalars_a_foo_bar_hello_world.json'); + }); + it('encodes parenthesis correctly', () => { assert.equal( router.pluginRoute( From 1b4b88cb2ea96463efd15c9244c9ca20d9a64902 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Tue, 27 Nov 2018 15:09:19 -0800 Subject: [PATCH 07/10] Remove unused search in demo path --- tensorboard/components/tf_backend/router.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index efa5c41b90..49c59f5eb6 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -123,11 +123,11 @@ function createDemoPath(dataDir: string, route: string, normalizedPath = normalizedPath.replace(/\//g, '_'); // Add query parameter as path if it is present. if (encodedQueryParam) normalizedPath += `_${encodedQueryParam}`; - const url = new URL(window.location.href); + const url = new URL(window.location.origin); // All demo data are serialized in JSON format. url.pathname = `${dataDir}/${normalizedPath}${ext}`; - return url.pathname + url.search; + return url.pathname; } export function createSearchParam(params: QueryParams = {}): URLSearchParams { From ec073668fa6cb9d4ac10d94dc482caf398c090f4 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Wed, 28 Nov 2018 14:16:56 -0800 Subject: [PATCH 08/10] Renamed methods and added comment for clarity --- tensorboard/components/tf_backend/router.ts | 38 +++++++++++++-------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index 49c59f5eb6..36cedd0222 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -36,25 +36,25 @@ export function createRouter(dataDir = 'data', demoMode = false): Router { if (dataDir[dataDir.length - 1] === '/') { dataDir = dataDir.slice(0, dataDir.length - 1); } - const createPath = demoMode ? createDemoPath : createProdPath; + const createDataPath = demoMode ? createDemoDataPath : createProdDataPath; const ext = demoMode ? '.json' : ''; return { - environment: () => createPath(dataDir, '/environment', ext), - experiments: () => createPath(dataDir, '/experiments', ext), + environment: () => createDataPath(dataDir, '/environment', ext), + experiments: () => createDataPath(dataDir, '/experiments', ext), isDemoMode: () => demoMode, pluginRoute: (pluginName: string, route: string, params?: URLSearchParams, demoCustomExt = ext): string => { - return createPath( + return createDataPath( demoMode ? dataDir : dataDir + '/plugin', `/${pluginName}${route}`, demoCustomExt, params); }, - pluginsListing: () => createPath(dataDir, '/plugins_listing', ext), - runs: () => createPath(dataDir, '/runs', ext), + pluginsListing: () => createDataPath(dataDir, '/plugins_listing', ext), + runs: () => createDataPath(dataDir, '/runs', ext), runsForExperiment: id => { - return createPath( + return createDataPath( dataDir, '/experiment_runs', ext, @@ -87,7 +87,7 @@ export function setRouter(router: Router): void { _router = router; } -function createProdPath(dataDir: string, route: string, +function createProdDataPath(dataDir: string, route: string, ext: string, params?: URLSearchParams): string { let relativePath = dataDir + route; if (params) { @@ -98,12 +98,20 @@ function createProdPath(dataDir: string, route: string, } /** - * Creates a URL for demo. + * Creates a URL for demo apps. + * + * [1]: Demo pages are served as files and data routes are served as JSON files. + * For shareability and ease of use, the data files are served at root[2], "/", + * thus, the demo data path should return the absolute path regardless of + * current pathname. + * + * [2]: See the path property of tensorboard/demo/BUILD:demo_data. + * * e.g., - * > createDemoPath('a', '/b', '.json', {a: 1}) + * > createDemoDataPath('a', '/b', '.json', {a: 1}) * < '/a/b_a_1.json' */ -function createDemoPath(dataDir: string, route: string, +function createDemoDataPath(dataDir: string, route: string, ext: string, params: URLSearchParams = new URLSearchParams()): string { // First, parse the path in a safe manner by constructing a URL. We don't @@ -123,11 +131,11 @@ function createDemoPath(dataDir: string, route: string, normalizedPath = normalizedPath.replace(/\//g, '_'); // Add query parameter as path if it is present. if (encodedQueryParam) normalizedPath += `_${encodedQueryParam}`; - const url = new URL(window.location.origin); - // All demo data are serialized in JSON format. - url.pathname = `${dataDir}/${normalizedPath}${ext}`; - return url.pathname; + const pathname = `${dataDir}/${normalizedPath}${ext}`; + // See [1] for the reason why we are forming an absolute path here. + const absPathname = pathname.startsWith('/') ? pathname : '/' + pathname; + return absPathname; } export function createSearchParam(params: QueryParams = {}): URLSearchParams { From 2009d0d223cad861c42d6a3569b5e0aca918ad76 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Thu, 29 Nov 2018 10:54:52 -0800 Subject: [PATCH 09/10] Handle the case when plugin route passes an empty search params --- tensorboard/components/tf_backend/router.ts | 4 ++-- tensorboard/components/tf_backend/test/backendTests.ts | 7 +++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index 36cedd0222..9aaf626441 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -90,9 +90,9 @@ export function setRouter(router: Router): void { function createProdDataPath(dataDir: string, route: string, ext: string, params?: URLSearchParams): string { let relativePath = dataDir + route; - if (params) { + if (params && String(params)) { const delimiter = route.includes('?') ? '&' : '?'; - relativePath += delimiter + params.toString(); + relativePath += delimiter + String(params); } return relativePath; } diff --git a/tensorboard/components/tf_backend/test/backendTests.ts b/tensorboard/components/tf_backend/test/backendTests.ts index e26348babc..1cd039c8fe 100644 --- a/tensorboard/components/tf_backend/test/backendTests.ts +++ b/tensorboard/components/tf_backend/test/backendTests.ts @@ -126,6 +126,13 @@ describe('backend tests', () => { 'data/plugin/scalars/a?b=c&d=1&d=2'); }); + it('does not put ? when passed an empty URLSearchParams', () => { + assert.equal( + router.pluginRoute('scalars', '/a', + new URLSearchParams()), + 'data/plugin/scalars/a'); + }); + it('encodes parenthesis correctly', () => { assert.equal( router.pluginRoute('scalars', '/a', From 242b866e693a2cf1d607c706c3fa445374ab0da7 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Thu, 29 Nov 2018 10:56:33 -0800 Subject: [PATCH 10/10] Minor code change --- tensorboard/components/tf_backend/router.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tensorboard/components/tf_backend/router.ts b/tensorboard/components/tf_backend/router.ts index 9aaf626441..f2f9466443 100644 --- a/tensorboard/components/tf_backend/router.ts +++ b/tensorboard/components/tf_backend/router.ts @@ -88,9 +88,9 @@ export function setRouter(router: Router): void { } function createProdDataPath(dataDir: string, route: string, - ext: string, params?: URLSearchParams): string { + ext: string, params: URLSearchParams = new URLSearchParams()): string { let relativePath = dataDir + route; - if (params && String(params)) { + if (String(params)) { const delimiter = route.includes('?') ? '&' : '?'; relativePath += delimiter + String(params); }