From db56a6f62c9acfc81abcdedea28fa0a6c9bb196b Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Mon, 1 Aug 2016 15:23:50 -0700 Subject: [PATCH] Lazy load HighlightJS library Previously, the HighlightJS library (HLJS) was being Vulcanized into the PolyGerrit build, even if the library was not being used (for example if the user does not navigate to a diff or has syntax highlighting disabled in preferences). Because HLJS is a substantial file -- ranging in size from 47KB to more than 455KB (ungizpped) depending on the build environment, this is not ideal. In order to lazy load a JS library: 1) It needs to be copied into the built WAR file to be addressable, and 2) It should not be Vulcanized into the built gr-app.js file. Previously, the PolyGerrit build system supported copying and non-vulcanizing exactly one JS library: namely webcomponents-lite.js. This change generalizes the mechanism by which webcomponents is copied and adds HLJS to that list. This satisfies **1**. Furthermore, the GR-SYNTAX-LAYER is rewritten to dynamically import HLJS in the `process` step by crafting a SCRIPT element and attaching it to the local DOM. (Syntax processing is invoked only after entire diff is rendered.) Code that depends on the library awaits this load before using it. Thus, the conventional JS import can be removed from the element's HTML and will not be recognized by the Vulcanize phase. This satisfies **2**. Tests are updated accordingly. Bug: Issue 4298 Change-Id: I9a95d6d4c211cd8f1ca1bc4daec770b64b22b3d1 --- polygerrit-ui/app/BUCK | 21 +++- .../diff/gr-syntax-layer/gr-syntax-layer.html | 1 - .../diff/gr-syntax-layer/gr-syntax-layer.js | 77 +++++++----- .../gr-syntax-layer/gr-syntax-layer_test.html | 116 ++++++++++-------- 4 files changed, 127 insertions(+), 88 deletions(-) diff --git a/polygerrit-ui/app/BUCK b/polygerrit-ui/app/BUCK index dfff22f939..d03acf24b1 100644 --- a/polygerrit-ui/app/BUCK +++ b/polygerrit-ui/app/BUCK @@ -14,7 +14,21 @@ APP_SRCS = glob( 'test/**', ] + WCT_TEST_PATTERNS + PY_TEST_PATTERNS) -WEBJS = 'bower_components/webcomponentsjs/webcomponents-lite.js' +# List libraries to be copied statically into the build. (i.e. Libraries not +# expected to be Vulcanized.) +WEB_JS_LIBS = [ + ('bower_components/webcomponentsjs', 'webcomponents-lite.js'), + ('bower_components/highlightjs', 'highlight.min.js'), +] + +# Map the static libraries to commands for the polygerrit_ui rule. +JS_LIBS_MKDIR_CMDS = [] +JS_LIBS_UNZIP_CMDS = [] +for lib in WEB_JS_LIBS: + JS_LIBS_MKDIR_CMDS.append('mkdir -p ' + lib[0]) + path = lib[0] + '/' + lib[1] + cmd = 'unzip -p $(location //polygerrit-ui:polygerrit_components) %s>%s' % (path, path) + JS_LIBS_UNZIP_CMDS.append(cmd) # TODO(dborowitz): Putting these rules in this package avoids having to handle # the app/ prefix like we would have to if this were in the parent directory. @@ -26,11 +40,12 @@ genrule( cmd = ' && '.join([ 'mkdir $TMP/polygerrit_ui', 'cd $TMP/polygerrit_ui', - 'mkdir -p {fonts,elements,bower_components/webcomponentsjs}', + 'mkdir -p {fonts,elements}', + ' && '.join(JS_LIBS_MKDIR_CMDS), 'unzip -qd fonts $(location //polygerrit-ui:fonts)', 'unzip -qd elements $(location :gr-app)', 'cp -rp $SRCDIR/* .', - 'unzip -p $(location //polygerrit-ui:polygerrit_components) %s>%s' % (WEBJS, WEBJS), + ' && '.join(JS_LIBS_UNZIP_CMDS), 'cd $TMP', 'zip -9qr $OUT .', ]), diff --git a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.html b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.html index d331ccdc02..c5c9377a75 100644 --- a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.html +++ b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.html @@ -14,7 +14,6 @@ See the License for the specific language governing permissions and limitations under the License. --> - diff --git a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js index 18650b55f0..97dd6ad473 100644 --- a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js +++ b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js @@ -31,8 +31,8 @@ 'text/x-sql': 'sql', 'text/x-scala': 'scala', }; - var ASYNC_DELAY = 10; + var HLJS_PATH = '../../../bower_components/highlightjs/highlight.min.js'; Polymer({ is: 'gr-syntax-layer', @@ -63,10 +63,6 @@ _processHandle: Number, }, - attached: function() { - hljs.configure({classPrefix: 'gr-diff gr-syntax gr-syntax-'}); - }, - addListener: function(fn) { this.push('_listeners', fn); }, @@ -139,36 +135,38 @@ lastNotify: {left: 1, right: 1}, }; - return new Promise(function(resolve) { - var nextStep = function() { - this._processHandle = null; - this._processNextLine(state); + return this._loadHLJS().then(function() { + return new Promise(function(resolve) { + var nextStep = function() { + this._processHandle = null; + this._processNextLine(state); - // Move to the next line in the section. - state.lineIndex++; + // Move to the next line in the section. + state.lineIndex++; - // If the section has been exhausted, move to the next one. - if (this._isSectionDone(state)) { - state.lineIndex = 0; - state.sectionIndex++; - } + // If the section has been exhausted, move to the next one. + if (this._isSectionDone(state)) { + state.lineIndex = 0; + state.sectionIndex++; + } - // If all sections have been exhausted, finish. - if (state.sectionIndex >= this.diff.content.length) { - resolve(); - this._notify(state); - return; - } + // If all sections have been exhausted, finish. + if (state.sectionIndex >= this.diff.content.length) { + resolve(); + this._notify(state); + return; + } - if (state.sectionIndex !== 0 && state.lineIndex % 100 === 0) { - this._notify(state); - this._processHandle = this.async(nextStep, ASYNC_DELAY); - } else { - nextStep.call(this); - } - }; + if (state.sectionIndex !== 0 && state.lineIndex % 100 === 0) { + this._notify(state); + this._processHandle = this.async(nextStep, ASYNC_DELAY); + } else { + nextStep.call(this); + } + }; - this._processHandle = this.async(nextStep, 1); + this._processHandle = this.async(nextStep, 1); + }.bind(this)); }.bind(this)); }, @@ -309,5 +307,24 @@ fn(start, end, side); }); }, + + /** + * Load and configure the HighlightJS library. If the library is already + * loaded, then do nothing and resolve. + * @return {Promise} + */ + _loadHLJS: function() { + if (window.hljs) { return Promise.resolve(); } + + return new Promise(function(resolve) { + var script = document.createElement('script'); + script.src = HLJS_PATH; + script.onload = function() { + hljs.configure({classPrefix: 'gr-diff gr-syntax gr-syntax-'}); + resolve(); + }; + Polymer.dom(this.root).appendChild(script); + }.bind(this)); + } }); })(); diff --git a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer_test.html b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer_test.html index ef81d8ce7e..f91bd1e89c 100644 --- a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer_test.html +++ b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer_test.html @@ -131,6 +131,7 @@ limitations under the License. test('process while disabled does nothing', function(done) { var processNextSpy = sandbox.spy(element, '_processNextLine'); element.enabled = false; + var loadHLJSSpy = sandbox.spy(element, '_loadHLJS'); var processPromise = element.process(); @@ -138,76 +139,83 @@ limitations under the License. assert.isFalse(processNextSpy.called); assert.equal(element._baseRanges.length, 0); assert.equal(element._revisionRanges.length, 0); + assert.isFalse(loadHLJSSpy.called); done(); }); }); - test('process highlight ipsum', function(done) { - element.diff.meta_a.content_type = 'application/json'; - element.diff.meta_b.content_type = 'application/json'; - var processNextSpy = sandbox.spy(element, '_processNextLine'); - var highlightStub = sandbox.stub(hljs, 'highlight', function( - lang, line, ignore, state) { - return { - value: line.replace(/ipsum/, 'ipsum'), - top: state === undefined ? 1 : state + 1, - }; + suite('after hljs load', function() { + setup(function(done) { + element._loadHLJS().then(done); }); - var processPromise = element.process(); + test('process highlight ipsum', function(done) { + element.diff.meta_a.content_type = 'application/json'; + element.diff.meta_b.content_type = 'application/json'; + var processNextSpy = sandbox.spy(element, '_processNextLine'); + var highlightStub = sandbox.stub(hljs, 'highlight', function( + lang, line, ignore, state) { + return { + value: line.replace(/ipsum/, 'ipsum'), + top: state === undefined ? 1 : state + 1, + }; + }); - processPromise.then(function() { - var linesA = diff.meta_a.lines; - var linesB = diff.meta_b.lines; + var processPromise = element.process(); - assert.isTrue(processNextSpy.called); - assert.equal(element._baseRanges.length, linesA); - assert.equal(element._revisionRanges.length, linesB); + processPromise.then(function() { + var linesA = diff.meta_a.lines; + var linesB = diff.meta_b.lines; - assert.equal(highlightStub.callCount, linesA + linesB); + assert.isTrue(processNextSpy.called); + assert.equal(element._baseRanges.length, linesA); + assert.equal(element._revisionRanges.length, linesB); - // The first line of both sides have a range. - [element._baseRanges[0], element._revisionRanges[0]] - .forEach(function(range) { - assert.equal(range.length, 1); - assert.equal(range[0].className, 'foobar'); - assert.equal(range[0].start, 'lorem '.length); - assert.equal(range[0].length, 'ipsum'.length); - }); + assert.equal(highlightStub.callCount, linesA + linesB); - // There are no ranges from ll.1-12 on the left and ll.1-11 on the - // right. - element._baseRanges.slice(1, 12) - .concat(element._revisionRanges.slice(1, 11)) - .forEach(function(range) { - assert.equal(range.length, 0); - }); + // The first line of both sides have a range. + [element._baseRanges[0], element._revisionRanges[0]] + .forEach(function(range) { + assert.equal(range.length, 1); + assert.equal(range[0].className, 'foobar'); + assert.equal(range[0].start, 'lorem '.length); + assert.equal(range[0].length, 'ipsum'.length); + }); - // There should be another pair of ranges on l.13 for the left and l.12 - // for the right. - [element._baseRanges[13], element._revisionRanges[12]] - .forEach(function(range) { - assert.equal(range.length, 1); - assert.equal(range[0].className, 'foobar'); - assert.equal(range[0].start, 32); - assert.equal(range[0].length, 'ipsum'.length); - }); + // There are no ranges from ll.1-12 on the left and ll.1-11 on the + // right. + element._baseRanges.slice(1, 12) + .concat(element._revisionRanges.slice(1, 11)) + .forEach(function(range) { + assert.equal(range.length, 0); + }); - // The next group should have a similar instance on either side. + // There should be another pair of ranges on l.13 for the left and + // l.12 for the right. + [element._baseRanges[13], element._revisionRanges[12]] + .forEach(function(range) { + assert.equal(range.length, 1); + assert.equal(range[0].className, 'foobar'); + assert.equal(range[0].start, 32); + assert.equal(range[0].length, 'ipsum'.length); + }); - var range = element._baseRanges[15]; - assert.equal(range.length, 1); - assert.equal(range[0].className, 'foobar'); - assert.equal(range[0].start, 34); - assert.equal(range[0].length, 'ipsum'.length); + // The next group should have a similar instance on either side. - range = element._revisionRanges[14]; - assert.equal(range.length, 1); - assert.equal(range[0].className, 'foobar'); - assert.equal(range[0].start, 35); - assert.equal(range[0].length, 'ipsum'.length); + var range = element._baseRanges[15]; + assert.equal(range.length, 1); + assert.equal(range[0].className, 'foobar'); + assert.equal(range[0].start, 34); + assert.equal(range[0].length, 'ipsum'.length); - done(); + range = element._revisionRanges[14]; + assert.equal(range.length, 1); + assert.equal(range[0].className, 'foobar'); + assert.equal(range[0].start, 35); + assert.equal(range[0].length, 'ipsum'.length); + + done(); + }); }); });