From 964455ae6f15cd4e2927d43f3a7a88c9633c6421 Mon Sep 17 00:00:00 2001 From: Ben Rohlfs Date: Wed, 27 Nov 2019 07:56:28 +0000 Subject: [PATCH] Revert "Support load from ASSETS_PATH for plugins if provided" This reverts commit c092c5b889ea44fdeac6f43f736973e7aae200d3. Reason for revert: Plugins (at least Android's) are relying on plugin._url to contain the actual host, e.g. android-review... Change-Id: I09ab73cbb1cc410b0f30fbe9fb1c465adaf5efd7 --- .../gr-js-api-interface/gr-api-utils.js | 6 +- .../gr-api-utils_test.html | 9 --- .../gr-js-api-interface/gr-plugin-loader.js | 74 ++++-------------- .../gr-plugin-loader_test.html | 78 ++----------------- .../gr-js-api-interface/gr-public-js-api.js | 9 +++ 5 files changed, 33 insertions(+), 143 deletions(-) diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils.js index 0ec3d6aeda..2d66cfae4e 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils.js +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils.js @@ -50,11 +50,7 @@ return url.pathname; } const base = Gerrit.BaseUrlBehavior.getBaseUrl(); - let pathname = url.pathname.replace(base, ''); - // Load from ASSETS_PATH - if (window.ASSETS_PATH && url.href.includes(window.ASSETS_PATH)) { - pathname = url.href.replace(window.ASSETS_PATH, ''); - } + const pathname = url.pathname.replace(base, ''); // Site theme is server from predefined path. if (pathname === '/static/gerrit-theme.html') { return 'gerrit-theme'; diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils_test.html b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils_test.html index b43796fe1d..128738d6bf 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils_test.html +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils_test.html @@ -72,15 +72,6 @@ limitations under the License. 'gerrit-theme' ); }); - - test('with ASSETS_PATH', () => { - window.ASSETS_PATH = 'http://cdn.com/2'; - assert.equal( - getPluginNameFromUrl(`${window.ASSETS_PATH}/plugins/a.html`), - 'a' - ); - window.ASSETS_PATH = undefined; - }); }); }); \ No newline at end of file diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.js index 96d84117dc..4be38b612a 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.js +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.js @@ -113,7 +113,7 @@ this._pluginListLoaded = true; plugins.forEach(path => { - const url = this._urlFor(path, window.ASSETS_PATH); + const url = this._urlFor(path); // Skip if preloaded, for bundling. if (this.isPluginPreloaded(url)) return; @@ -128,11 +128,11 @@ }); if (this._isPathEndsWith(url, '.html')) { - this._importHtmlPlugin(path, opts && opts[path]); + this._importHtmlPlugin(url, opts && opts[path]); } else if (this._isPathEndsWith(url, '.js')) { - this._loadJsPlugin(path); + this._loadJsPlugin(url); } else { - this._failToLoad(`Unrecognized plugin path ${path}`, path); + this._failToLoad(`Unrecognized plugin url ${url}`, url); } }); @@ -181,15 +181,14 @@ return; } - const url = this._urlFor(src); - const pluginObject = this.getPlugin(url); + const pluginObject = this.getPlugin(src); let plugin = pluginObject && pluginObject.plugin; if (!plugin) { - plugin = new Plugin(url); + plugin = new Plugin(src); } try { callback(plugin); - this._pluginInstalled(url, plugin); + this._pluginInstalled(src, plugin); } catch (e) { this._failToLoad(`${e.name}: ${e.message}`, src); } @@ -314,79 +313,38 @@ } _importHtmlPlugin(pluginUrl, opts = {}) { - const urlWithAP = this._urlFor(pluginUrl, window.ASSETS_PATH); - const urlWithoutAP = this._urlFor(pluginUrl); - let onerror = null; - if (urlWithAP !== urlWithoutAP) { - onerror = () => this._loadHtmlPlugin(urlWithoutAP, opts.sync); - } - this._loadHtmlPlugin(urlWithAP, opts.sync, onerror); - } - - _loadHtmlPlugin(url, sync, onerror) { - if (!onerror) { - onerror = () => { - this._failToLoad(`${pluginUrl} import error`, pluginUrl); - }; - } - + // onload (second param) needs to be a function. When null or undefined + // were passed, plugins were not loaded correctly. (Polymer.importHref || Polymer.Base.importHref)( - url, () => {}, - onerror, - !sync); + this._urlFor(pluginUrl), () => {}, + () => this._failToLoad(`${pluginUrl} import error`, pluginUrl), + !opts.sync); } _loadJsPlugin(pluginUrl) { - const urlWithAP = this._urlFor(pluginUrl, window.ASSETS_PATH); - const urlWithoutAP = this._urlFor(pluginUrl); - let onerror = null; - if (urlWithAP !== urlWithoutAP) { - onerror = () => this._createScriptTag(urlWithoutAP); - } - - this._createScriptTag(urlWithAP, onerror); + this._createScriptTag(this._urlFor(pluginUrl)); } - _createScriptTag(url, onerror) { - if (!onerror) { - onerror = () => this._failToLoad(`${url} load error`, url); - } - + _createScriptTag(url) { const el = document.createElement('script'); el.defer = true; el.setAttribute('src', url); - el.onerror = onerror; + el.onerror = () => this._failToLoad(`${url} load error`, url); return document.body.appendChild(el); } - _urlFor(pathOrUrl, assetsPath) { + _urlFor(pathOrUrl) { if (!pathOrUrl) { return pathOrUrl; } - - // theme is per host, should always load from assetsPath - const isThemeFile = pathOrUrl.endsWith('static/gerrit-theme.html'); - const shouldTryLoadFromAssetsPathFirst = !isThemeFile && assetsPath; if (pathOrUrl.startsWith(PRELOADED_PROTOCOL) || pathOrUrl.startsWith('http')) { // Plugins are loaded from another domain or preloaded. - if (pathOrUrl.includes(location.host) - && shouldTryLoadFromAssetsPathFirst) { - // if is loading from host server, try replace with cdn when assetsPath provided - return pathOrUrl - .replace(location.origin, assetsPath); - } return pathOrUrl; } - if (!pathOrUrl.startsWith('/')) { pathOrUrl = '/' + pathOrUrl; } - - if (shouldTryLoadFromAssetsPathFirst) { - return assetsPath + pathOrUrl; - } - return window.location.origin + getBaseUrl() + pathOrUrl; } diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader_test.html b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader_test.html index 08e7e18d71..8c1ec968b2 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader_test.html +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader_test.html @@ -325,11 +325,11 @@ limitations under the License. let loadJsPluginStub; setup(() => { importHtmlPluginStub = sandbox.stub(); - sandbox.stub(Gerrit._pluginLoader, '_loadHtmlPlugin', url => { + sandbox.stub(Gerrit._pluginLoader, '_importHtmlPlugin', url => { importHtmlPluginStub(url); }); loadJsPluginStub = sandbox.stub(); - sandbox.stub(Gerrit._pluginLoader, '_createScriptTag', url => { + sandbox.stub(Gerrit._pluginLoader, '_loadJsPlugin', url => { loadJsPluginStub(url); }); }); @@ -346,8 +346,8 @@ limitations under the License. assert.isTrue(failToLoadStub.calledOnce); assert.isTrue(failToLoadStub.calledWithExactly( - 'Unrecognized plugin path foo/bar', - 'foo/bar' + `Unrecognized plugin url ${url}/foo/bar`, + `${url}/foo/bar` )); }); @@ -407,72 +407,6 @@ limitations under the License. }); }); - suite('With ASSETS_PATH', () => { - let importHtmlPluginStub; - let loadJsPluginStub; - setup(() => { - window.ASSETS_PATH = 'https://cdn.com'; - importHtmlPluginStub = sandbox.stub(); - sandbox.stub(Gerrit._pluginLoader, '_loadHtmlPlugin', url => { - importHtmlPluginStub(url); - }); - loadJsPluginStub = sandbox.stub(); - sandbox.stub(Gerrit._pluginLoader, '_createScriptTag', url => { - loadJsPluginStub(url); - }); - }); - - teardown(() => { - window.ASSETS_PATH = ''; - }); - - test('Should try load plugins from assets path instead', () => { - Gerrit._loadPlugins([ - 'foo/bar.js', - 'foo/bar.html', - ]); - - assert.isTrue(importHtmlPluginStub.calledOnce); - assert.isTrue( - importHtmlPluginStub.calledWithExactly(`https://cdn.com/foo/bar.html`) - ); - assert.isTrue(loadJsPluginStub.calledOnce); - assert.isTrue( - loadJsPluginStub.calledWithExactly(`https://cdn.com/foo/bar.js`)); - }); - - test('Should honor original path if exists', () => { - Gerrit._loadPlugins([ - 'http://e.com/foo/bar.html', - 'http://e.com/foo/bar.js', - ]); - - assert.isTrue(importHtmlPluginStub.calledOnce); - assert.isTrue( - importHtmlPluginStub.calledWithExactly(`http://e.com/foo/bar.html`) - ); - assert.isTrue(loadJsPluginStub.calledOnce); - assert.isTrue( - loadJsPluginStub.calledWithExactly(`http://e.com/foo/bar.js`)); - }); - - test('Should try replace current host with assetsPath', () => { - const host = window.location.origin; - Gerrit._loadPlugins([ - `${host}/foo/bar.html`, - `${host}/foo/bar.js`, - ]); - - assert.isTrue(importHtmlPluginStub.calledOnce); - assert.isTrue( - importHtmlPluginStub.calledWithExactly(`https://cdn.com/foo/bar.html`) - ); - assert.isTrue(loadJsPluginStub.calledOnce); - assert.isTrue( - loadJsPluginStub.calledWithExactly(`https://cdn.com/foo/bar.js`)); - }); - }); - test('adds js plugins will call the body', () => { Gerrit._loadPlugins([ 'http://e.com/foo/bar.js', @@ -555,10 +489,12 @@ limitations under the License. test('installing preloaded plugin', () => { let plugin; + window.ASSETS_PATH = 'http://blips.com/chitz'; Gerrit.install(p => { plugin = p; }, '0.1', 'preloaded:foo'); assert.strictEqual(plugin.getPluginName(), 'foo'); assert.strictEqual(plugin.url('/some/thing.html'), - 'preloaded:foo/plugins/foo/some/thing.html'); + 'http://blips.com/chitz/plugins/foo/some/thing.html'); + delete window.ASSETS_PATH; }); }); }); diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.js index 0aaeaa1a0d..6c306d9b63 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.js +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.js @@ -17,6 +17,8 @@ (function(window) { 'use strict'; + const PRELOADED_PROTOCOL = 'preloaded:'; + const PANEL_ENDPOINTS_MAPPING = { CHANGE_SCREEN_BELOW_COMMIT_INFO_BLOCK: 'change-view-integration', CHANGE_SCREEN_BELOW_CHANGE_INFO_BLOCK: 'change-metadata-item', @@ -64,6 +66,13 @@ this._url = new URL(opt_url); this._name = getPluginNameFromUrl(this._url); + if (this._url.protocol === PRELOADED_PROTOCOL) { + // Original plugin URL is used in plugin assets URLs calculation. + const assetsBaseUrl = window.ASSETS_PATH || + (window.location.origin + Gerrit.BaseUrlBehavior.getBaseUrl()); + this._url = new URL(assetsBaseUrl + '/plugins/' + this._name + + '/static/' + this._name + '.js'); + } } Plugin._sharedAPIElement = document.createElement('gr-js-api-interface');