Fix concurrent requests (#152)

* Inline scripts should always execute after scripts are downloaded

Fixes #151

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
 

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)
+  )

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 = ->

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

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
+  }

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: () -> {}
+  }

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()

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;

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)

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)