From 0c6cff88f48cf0b87f54618938f3b63d060f9d34 Mon Sep 17 00:00:00 2001 From: Gord P Date: Thu, 22 Sep 2016 09:47:52 -0400 Subject: [PATCH] Fix concurrent requests (#152) * Inline scripts should always execute after scripts are downloaded Fixes #151 --- README.md | 3 + .../javascripts/turbograft/turbohead.coffee | 71 ++++-- .../javascripts/turbograft/turbolinks.coffee | 5 +- test/example/config/initializers/assets.rb | 4 + test/javascripts/fake_document.coffee | 21 ++ test/javascripts/fake_script.coffee | 28 +++ test/javascripts/remote_test.coffee | 5 +- test/javascripts/test_helper.js | 3 + test/javascripts/turbohead_test.coffee | 122 +++++++++ test/javascripts/turbolinks_test.coffee | 25 ++ .../vendor/promise-polyfill/promise.js | 233 ++++++++++++++++++ 11 files changed, 492 insertions(+), 28 deletions(-) create mode 100644 test/javascripts/fake_document.coffee create mode 100644 test/javascripts/fake_script.coffee create mode 100644 test/javascripts/turbohead_test.coffee create mode 100644 test/javascripts/vendor/promise-polyfill/promise.js diff --git a/README.md b/README.md index 323b14f5..887ae802 100644 --- a/README.md +++ b/README.md @@ -28,6 +28,9 @@ Turbograft was built with simplicity in mind. It intends to offer the smallest a * Replace `//= require turbolinks` with `//= require turbograft` in _app/assets/javascripts/application.js_ * Run `bundle install` +## Dependencies + +Turbograft requires a browser that supports [`Promise`](https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Promise), or a polyfill (e.g., [core-js](https://github.com/zloirock/core-js).) ## Usage diff --git a/lib/assets/javascripts/turbograft/turbohead.coffee b/lib/assets/javascripts/turbograft/turbohead.coffee index 2ed81117..b00a9eb7 100644 --- a/lib/assets/javascripts/turbograft/turbohead.coffee +++ b/lib/assets/javascripts/turbograft/turbohead.coffee @@ -2,6 +2,14 @@ TRACKED_ASSET_SELECTOR = '[data-turbolinks-track]' TRACKED_ATTRIBUTE_NAME = 'turbolinksTrack' ANONYMOUS_TRACK_VALUE = 'true' +scriptPromises = {} +resolvePreviousRequest = null + +waitForCompleteDownloads = -> + loadingPromises = Object.keys(scriptPromises).map (url) -> + scriptPromises[url] + Promise.all(loadingPromises) + class window.TurboHead constructor: (@activeDocument, @upstreamDocument) -> @activeAssets = extractTrackedAssets(@activeDocument) @@ -14,6 +22,12 @@ class window.TurboHead .filter(attributeMatches('nodeName', 'LINK')) .filter(noAttributeMatchesIn('href', @activeAssets)) + @_testAPI: { + reset: -> + scriptPromises = {} + resolvePreviousRequest = null + } + hasChangedAnonymousAssets: () -> anonymousUpstreamAssets = @upstreamAssets .filter(datasetMatches(TRACKED_ATTRIBUTE_NAME, ANONYMOUS_TRACK_VALUE)) @@ -39,9 +53,20 @@ class window.TurboHead hasAssetConflicts: () -> @hasNamedAssetConflicts() || @hasChangedAnonymousAssets() - insertNewAssets: (callback) -> + waitForAssets: () -> + resolvePreviousRequest?(isCanceled: true) + + new Promise((resolve) => + resolvePreviousRequest = resolve + waitForCompleteDownloads() + .then(@_insertNewAssets) + .then(waitForCompleteDownloads) + .then(resolve) + ) + + _insertNewAssets: () => updateLinkTags(@activeDocument, @newLinks) - updateScriptTags(@activeDocument, @newScripts, callback) + updateScriptTags(@activeDocument, @newScripts) extractTrackedAssets = (doc) -> [].slice.call(doc.querySelectorAll(TRACKED_ASSET_SELECTOR)) @@ -76,37 +101,37 @@ noDatasetMatchesIn = (attribute, collection) -> updateLinkTags = (activeDocument, newLinks) -> # style tag load events don't work in all browsers # as such we just hope they load ¯\_(ツ)_/¯ - newLinks.forEach((linkNode) -> insertLinkTask(activeDocument, linkNode)()) - -updateScriptTags = (activeDocument, newScripts, callback) -> - asyncSeries( - newScripts.map((scriptNode) -> insertScriptTask(activeDocument, scriptNode)), - callback + newLinks.forEach((linkNode) -> + newNode = linkNode.cloneNode() + activeDocument.head.appendChild(newNode) + triggerEvent("page:after-link-inserted", newNode) ) -asyncSeries = (tasks, callback) -> - return callback() if tasks.length == 0 - task = tasks.shift() - task(-> asyncSeries(tasks, callback)) +updateScriptTags = (activeDocument, newScripts) -> + promise = Promise.resolve() + newScripts.forEach (scriptNode) -> + promise = promise.then(-> insertScript(activeDocument, scriptNode)) + promise + +insertScript = (activeDocument, scriptNode) -> + url = scriptNode.src + if scriptPromises[url] + return scriptPromises[url] -insertScriptTask = (activeDocument, scriptNode) -> - # We need to clone script tags in order to ensure that the browser executes them. + # Clone script tags to guarantee browser execution. newNode = activeDocument.createElement('SCRIPT') newNode.setAttribute(attr.name, attr.value) for attr in scriptNode.attributes newNode.appendChild(activeDocument.createTextNode(scriptNode.innerHTML)) - insertAssetTask(activeDocument, newNode, 'script') -insertLinkTask = (activeDocument, node) -> - insertAssetTask(activeDocument, node.cloneNode(), 'link') - -insertAssetTask = (activeDocument, newNode, name) -> - (done) -> + scriptPromises[url] = new Promise((resolve) -> onAssetEvent = (event) -> - triggerEvent("page:#{name}-error", event) if event.type == 'error' + triggerEvent("page:#script-error", event) if event.type == 'error' newNode.removeEventListener('load', onAssetEvent) newNode.removeEventListener('error', onAssetEvent) - done() if typeof done == 'function' + resolve() + newNode.addEventListener('load', onAssetEvent) newNode.addEventListener('error', onAssetEvent) activeDocument.head.appendChild(newNode) - triggerEvent("page:after-#{name}-inserted", newNode) + triggerEvent("page:after-script-inserted", newNode) + ) diff --git a/lib/assets/javascripts/turbograft/turbolinks.coffee b/lib/assets/javascripts/turbograft/turbolinks.coffee index 45e13eb6..6b0db0c6 100644 --- a/lib/assets/javascripts/turbograft/turbolinks.coffee +++ b/lib/assets/javascripts/turbograft/turbolinks.coffee @@ -139,7 +139,9 @@ class window.Turbolinks if turbohead.hasAssetConflicts() return Turbolinks.fullPageNavigate(url.absolute) reflectNewUrl url if options.updatePushState - turbohead.insertNewAssets(-> updateBody(upstreamDocument, xhr, options)) + turbohead.waitForAssets().then((result) -> + updateBody(upstreamDocument, xhr, options) unless result?.isCanceled + ) else triggerEvent 'page:error', xhr Turbolinks.fullPageNavigate(url.absolute) if url? @@ -303,6 +305,7 @@ class window.Turbolinks location = new ComponentUrl location preservedHash = if location.hasNoHash() then document.location.hash else '' Turbolinks.replaceState currentState, '', location.href + preservedHash + return rememberReferer = -> diff --git a/test/example/config/initializers/assets.rb b/test/example/config/initializers/assets.rb index 6d72166e..cd4e52cf 100644 --- a/test/example/config/initializers/assets.rb +++ b/test/example/config/initializers/assets.rb @@ -1,4 +1,5 @@ Example::Application.config.assets.precompile += %w( + vendor/promise-polyfill/promise.self.js application.self.js component_url_test.self.js csrf_token_test.self.js @@ -11,6 +12,8 @@ support/sinon.self.js support/chai.self.js support/sinon-chai.self.js + fake_document.self.js + fake_script.self.js test_helper.self.js turbograft.self.js turbograft/click.self.js @@ -23,6 +26,7 @@ turbograft/turbolinks.self.js turbograft/turbohead.self.js turbolinks_test.self.js + turbohead_test.self.js fixtures/css/foo.css fixtures/css/bar.css fixtures/css/order_testing/a.css diff --git a/test/javascripts/fake_document.coffee b/test/javascripts/fake_document.coffee new file mode 100644 index 00000000..c35ab76c --- /dev/null +++ b/test/javascripts/fake_document.coffee @@ -0,0 +1,21 @@ +#= require ./fake_script + +window.fakeDocument = (scriptSources) -> + nodes = (fakeScript(src) for src in scriptSources) + newNodes = [] + + return { + createdScripts: newNodes + head: { + appendChild: () -> {} + } + + createElement: () -> + script = fakeScript() + newNodes.push(script) + script + + createTextNode: () -> {} + + querySelectorAll: -> nodes + } diff --git a/test/javascripts/fake_script.coffee b/test/javascripts/fake_script.coffee new file mode 100644 index 00000000..f3ad1171 --- /dev/null +++ b/test/javascripts/fake_script.coffee @@ -0,0 +1,28 @@ +window.fakeScript = (src) -> + listeners = [] + node = { + 'data-turbolinks-track': src + attributes: [{name: 'src', value: src}] + isLoaded: false + src: src + nodeName: 'SCRIPT' + + appendChild: () -> {} + + setAttribute: (name, value) -> + if name == 'src' + @src = value + @attributes.push({name: name, value: value}) + + addEventListener: (eventName, listener) -> + return if eventName != 'load' + listeners.push(listener) + + fireLoaded: () -> + listener({type: 'load'}) for listener in listeners + new Promise (resolve) -> + node.isLoaded = true + setTimeout -> resolve(node) + + removeEventListener: () -> {} + } diff --git a/test/javascripts/remote_test.coffee b/test/javascripts/remote_test.coffee index 7989539a..1127f39e 100644 --- a/test/javascripts/remote_test.coffee +++ b/test/javascripts/remote_test.coffee @@ -94,10 +94,7 @@ describe 'Remote', -> fail: done , @initiating_target - simulateError = -> - listener(target: remote.xhr) for listener in remote.xhr.eventListeners.error - - setTimeout(simulateError, 0) + remote.xhr.respond(404) it 'will call options.success() on success', (done) -> server = sinon.fakeServer.create() diff --git a/test/javascripts/test_helper.js b/test/javascripts/test_helper.js index 8564b257..6e8f2684 100644 --- a/test/javascripts/test_helper.js +++ b/test/javascripts/test_helper.js @@ -2,6 +2,7 @@ //= require support/chai //= require support/sinon-chai //= require fixtures/js/routes +//= require vendor/promise-polyfill/promise //= require application expect = chai.expect; @@ -11,3 +12,5 @@ mock = sinon.mock; stub = sinon.stub; mocha.setup('tdd') +sinon.assert.expose(chai.assert, {prefix: ''}); +chai.config.truncateThreshold = 9999; diff --git a/test/javascripts/turbohead_test.coffee b/test/javascripts/turbohead_test.coffee new file mode 100644 index 00000000..43490a9c --- /dev/null +++ b/test/javascripts/turbohead_test.coffee @@ -0,0 +1,122 @@ +#= require ./fake_document + +describe 'TurboHead', -> + activeDocument = null + promiseQueue = null + requests = [] + + assertScriptCount = (size, message) -> + promiseQueue = promiseQueue.then -> + assert.lengthOf(activeDocument.createdScripts, size, message) + + finishScriptDownload = -> + promiseQueue = promiseQueue.then -> + new Promise (resolve) -> + unloaded = activeDocument.createdScripts.filter (script) -> + !script.isLoaded + + unloaded[0].fireLoaded().then -> + setTimeout -> resolve(unloaded[0]) + + newRequest = (requestScripts) -> + promiseQueue = promiseQueue.then -> + new Promise (resolve) -> + head = new TurboHead( + activeDocument, + fakeDocument(requestScripts) + ) + + request = head.waitForAssets() + request.isInProgress = true + request.isFulfilled = false + request.isRejected = false + request + .then (result) -> + request.isInProgress = false + request.isCanceled = Boolean(result.isCanceled) + request.isFulfilled = !request.isCanceled + .catch -> request.isRejected = true + requests.push(request) + + setTimeout(resolve, 50) # Wait for beginning of first script download. + + beforeEach -> + activeDocument = fakeDocument([]) # Start with no scripts. + promiseQueue = Promise.resolve() + requests = [] + + afterEach -> + TurboHead._testAPI.reset() + + describe 'script download queue', -> + it 'downloads scripts in sequence', -> + newRequest(['a.js', 'b.js', 'c.js']) + assertScriptCount(1, 'first script download should be in progress') + finishScriptDownload() + .then (script) -> assert.equal(script.src, 'a.js') + + assertScriptCount(2, 'first download complete should trigger second') + finishScriptDownload() + .then (script) -> assert.equal(script.src, 'b.js') + + assertScriptCount(3, 'second download complete should trigger third') + finishScriptDownload() + .then (script) -> + assert.equal(script.src, 'c.js') + assert.isTrue( + requests[0].isFulfilled, + 'all downloads complete should resolve request' + ) + + describe 'superceded requests', -> + it 'cancels stale requests', -> + newRequest(['d.js']) + newRequest([]).then -> + assert.isTrue(requests[0].isCanceled) + + it 'waits for previously queued scripts before starting new request', -> + newRequest(['a.js', 'b.js']) + newRequest([]) + finishScriptDownload() + finishScriptDownload() + assertScriptCount(2, 'duplicate script elements should not be created') + .then -> + assert.isTrue(requests[0].isCanceled) + assert.isTrue(requests[1].isFulfilled) + + it 'does not add duplicate script tags for new requests', -> + newRequest(['a.js', 'b.js']) + newRequest(['a.js', 'b.js']) + assertScriptCount(1, 'first script should be downloading').then -> + assert.isTrue(requests[1].isInProgress) + finishScriptDownload() + finishScriptDownload() + .then -> + assertScriptCount(2, 'duplicate script elements were created') + assert.isTrue(requests[1].isFulfilled) + + it 'enqueues new scripts for new requests', -> + newRequest(['a.js', 'b.js']) + newRequest(['b.js', 'c.js', 'd.js']) + finishScriptDownload() + finishScriptDownload().then -> + assert.isTrue(requests[1].isInProgress) + finishScriptDownload().then -> + assert.isTrue(requests[1].isInProgress) + finishScriptDownload().then (script) -> + assertScriptCount(4, 'second request\'s assets should be downloaded') + assert.equal( + script.src, 'd.js', + 'last queued script should be last downloaded' + ) + assert.isTrue(requests[1].isFulfilled) + + it 'does not reload completed scripts for new requests', -> + newRequest(['a.js', 'b.js']) + finishScriptDownload() + finishScriptDownload().then -> + assertScriptCount(2) + assert.isTrue(requests[0].isFulfilled) + newRequest(['a.js', 'b.js']).then -> + assertScriptCount(2) + assert.isTrue(requests[1].isFulfilled) diff --git a/test/javascripts/turbolinks_test.coffee b/test/javascripts/turbolinks_test.coffee index 57e41f40..5762fac2 100644 --- a/test/javascripts/turbolinks_test.coffee +++ b/test/javascripts/turbolinks_test.coffee @@ -86,6 +86,7 @@ describe 'Turbolinks', -> $("script").attr("data-turbolinks-eval", false) $("#mocha").attr("refresh-never", true) + TurboHead._testAPI.reset() resetPage() afterEach -> @@ -387,3 +388,27 @@ describe 'Turbolinks', -> assert.equal(1, nodes.length) assert.equal('div1', nodes[0].id) done() + + describe 'asset loading finished', -> + SUCCESS_HTML_CONTENT = 'Hi there' + xhr = null + resolver = null + + beforeEach -> + promise = new Promise((resolve) -> resolver = resolve) + sandbox.stub(TurboHead.prototype, 'hasAssetConflicts').returns(false) + sandbox.stub(TurboHead.prototype, 'waitForAssets').returns(promise) + + xhr = new sinon.FakeXMLHttpRequest() + xhr.open('POST', '/my/endpoint', true) + xhr.respond(200, {'Content-Type':'text/html'}, SUCCESS_HTML_CONTENT) + + it 'does not update document if the request was canceled', -> + resolver({isCanceled: true}) + loadPromise = Turbolinks.loadPage('/foo', xhr) + .then -> assert.notInclude(document.body.innerHTML, SUCCESS_HTML_CONTENT) + + it 'updates the document if the request was not canceled', -> + resolver() + loadPromise = Turbolinks.loadPage('/foo', xhr) + .then -> assert.include(document.body.innerHTML, SUCCESS_HTML_CONTENT) diff --git a/test/javascripts/vendor/promise-polyfill/promise.js b/test/javascripts/vendor/promise-polyfill/promise.js new file mode 100644 index 00000000..cf0c81d6 --- /dev/null +++ b/test/javascripts/vendor/promise-polyfill/promise.js @@ -0,0 +1,233 @@ +(function (root) { + + // Store setTimeout reference so promise-polyfill will be unaffected by + // other code modifying setTimeout (like sinon.useFakeTimers()) + var setTimeoutFunc = setTimeout; + + function noop() {} + + // Polyfill for Function.prototype.bind + function bind(fn, thisArg) { + return function () { + fn.apply(thisArg, arguments); + }; + } + + function Promise(fn) { + if (typeof this !== 'object') throw new TypeError('Promises must be constructed via new'); + if (typeof fn !== 'function') throw new TypeError('not a function'); + this._state = 0; + this._handled = false; + this._value = undefined; + this._deferreds = []; + + doResolve(fn, this); + } + + function handle(self, deferred) { + while (self._state === 3) { + self = self._value; + } + if (self._state === 0) { + self._deferreds.push(deferred); + return; + } + self._handled = true; + Promise._immediateFn(function () { + var cb = self._state === 1 ? deferred.onFulfilled : deferred.onRejected; + if (cb === null) { + (self._state === 1 ? resolve : reject)(deferred.promise, self._value); + return; + } + var ret; + try { + ret = cb(self._value); + } catch (e) { + reject(deferred.promise, e); + return; + } + resolve(deferred.promise, ret); + }); + } + + function resolve(self, newValue) { + try { + // Promise Resolution Procedure: https://github.com/promises-aplus/promises-spec#the-promise-resolution-procedure + if (newValue === self) throw new TypeError('A promise cannot be resolved with itself.'); + if (newValue && (typeof newValue === 'object' || typeof newValue === 'function')) { + var then = newValue.then; + if (newValue instanceof Promise) { + self._state = 3; + self._value = newValue; + finale(self); + return; + } else if (typeof then === 'function') { + doResolve(bind(then, newValue), self); + return; + } + } + self._state = 1; + self._value = newValue; + finale(self); + } catch (e) { + reject(self, e); + } + } + + function reject(self, newValue) { + self._state = 2; + self._value = newValue; + finale(self); + } + + function finale(self) { + if (self._state === 2 && self._deferreds.length === 0) { + Promise._immediateFn(function() { + if (!self._handled) { + Promise._unhandledRejectionFn(self._value); + } + }); + } + + for (var i = 0, len = self._deferreds.length; i < len; i++) { + handle(self, self._deferreds[i]); + } + self._deferreds = null; + } + + function Handler(onFulfilled, onRejected, promise) { + this.onFulfilled = typeof onFulfilled === 'function' ? onFulfilled : null; + this.onRejected = typeof onRejected === 'function' ? onRejected : null; + this.promise = promise; + } + + /** + * Take a potentially misbehaving resolver function and make sure + * onFulfilled and onRejected are only called once. + * + * Makes no guarantees about asynchrony. + */ + function doResolve(fn, self) { + var done = false; + try { + fn(function (value) { + if (done) return; + done = true; + resolve(self, value); + }, function (reason) { + if (done) return; + done = true; + reject(self, reason); + }); + } catch (ex) { + if (done) return; + done = true; + reject(self, ex); + } + } + + Promise.prototype['catch'] = function (onRejected) { + return this.then(null, onRejected); + }; + + Promise.prototype.then = function (onFulfilled, onRejected) { + var prom = new (this.constructor)(noop); + + handle(this, new Handler(onFulfilled, onRejected, prom)); + return prom; + }; + + Promise.all = function (arr) { + var args = Array.prototype.slice.call(arr); + + return new Promise(function (resolve, reject) { + if (args.length === 0) return resolve([]); + var remaining = args.length; + + function res(i, val) { + try { + if (val && (typeof val === 'object' || typeof val === 'function')) { + var then = val.then; + if (typeof then === 'function') { + then.call(val, function (val) { + res(i, val); + }, reject); + return; + } + } + args[i] = val; + if (--remaining === 0) { + resolve(args); + } + } catch (ex) { + reject(ex); + } + } + + for (var i = 0; i < args.length; i++) { + res(i, args[i]); + } + }); + }; + + Promise.resolve = function (value) { + if (value && typeof value === 'object' && value.constructor === Promise) { + return value; + } + + return new Promise(function (resolve) { + resolve(value); + }); + }; + + Promise.reject = function (value) { + return new Promise(function (resolve, reject) { + reject(value); + }); + }; + + Promise.race = function (values) { + return new Promise(function (resolve, reject) { + for (var i = 0, len = values.length; i < len; i++) { + values[i].then(resolve, reject); + } + }); + }; + + // Use polyfill for setImmediate for performance gains + Promise._immediateFn = (typeof setImmediate === 'function' && function (fn) { setImmediate(fn); }) || + function (fn) { + setTimeoutFunc(fn, 0); + }; + + Promise._unhandledRejectionFn = function _unhandledRejectionFn(err) { + if (typeof console !== 'undefined' && console) { + console.warn('Possible Unhandled Promise Rejection:', err); // eslint-disable-line no-console + } + }; + + /** + * Set the immediate function to execute callbacks + * @param fn {function} Function to execute + * @deprecated + */ + Promise._setImmediateFn = function _setImmediateFn(fn) { + Promise._immediateFn = fn; + }; + + /** + * Change the function to execute on unhandled rejection + * @param {function} fn Function to execute on unhandled rejection + * @deprecated + */ + Promise._setUnhandledRejectionFn = function _setUnhandledRejectionFn(fn) { + Promise._unhandledRejectionFn = fn; + }; + + if (typeof module !== 'undefined' && module.exports) { + module.exports = Promise; + } else if (!root.Promise) { + root.Promise = Promise; + } + +})(this);