From a37d2c1b93ab0e05612c1d54bf2911e3aa420b85 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Mon, 24 Mar 2014 19:19:17 -0400 Subject: [PATCH] Fix Studio's XBlock dependency loading issues STUD-1465 Changed the XBlock rendering logic to use promises to track asynchronous tasks. Each dependency is then only loaded once the previous one has completed, and the XBlock initialization only happens once all dependencies are loaded. This gives XBlocks the same semantics in Studio as they get when shown directly on a page. --- .../coffee/spec/views/module_edit_spec.coffee | 2 +- .../coffee/src/views/module_edit.coffee | 3 +- cms/static/js/spec/views/xblock_spec.js | 17 ++- cms/static/js/views/xblock.js | 108 +++++++++++++----- 4 files changed, 95 insertions(+), 35 deletions(-) diff --git a/cms/static/coffee/spec/views/module_edit_spec.coffee b/cms/static/coffee/spec/views/module_edit_spec.coffee index ccf22fab40..aaefb96dad 100644 --- a/cms/static/coffee/spec/views/module_edit_spec.coffee +++ b/cms/static/coffee/spec/views/module_edit_spec.coffee @@ -58,7 +58,7 @@ define ["jquery", "coffee/src/views/module_edit", "js/models/module_info", "xmod spyOn(@moduleEdit, 'loadEdit') spyOn(@moduleEdit, 'delegateEvents') spyOn($.fn, 'append') - spyOn($, 'getScript') + spyOn($, 'getScript').andReturn($.Deferred().resolve().promise()) window.loadedXBlockResources = undefined diff --git a/cms/static/coffee/src/views/module_edit.coffee b/cms/static/coffee/src/views/module_edit.coffee index e547a43c47..fa01cda265 100644 --- a/cms/static/coffee/src/views/module_edit.coffee +++ b/cms/static/coffee/src/views/module_edit.coffee @@ -81,8 +81,7 @@ define ["jquery", "underscore", "gettext", "xblock/runtime.v1", headers: Accept: 'application/json' success: (fragment) => - @renderXBlockFragment(fragment, target, viewName) - callback() + @renderXBlockFragment(fragment, target).done(callback) ) render: -> @loadView('student_view', @$el, => diff --git a/cms/static/js/spec/views/xblock_spec.js b/cms/static/js/spec/views/xblock_spec.js index b3e05c9ca5..4c38402e68 100644 --- a/cms/static/js/spec/views/xblock_spec.js +++ b/cms/static/js/spec/views/xblock_spec.js @@ -38,18 +38,22 @@ define([ "jquery", "js/spec/create_sinon", "URI", "js/views/xblock", "js/models/ var postXBlockRequest; postXBlockRequest = function(requests, resources) { + var promise; $.ajax({ url: "test_url", type: 'GET', success: function(fragment) { - xblockView.renderXBlockFragment(fragment, this.$el); + promise = xblockView.renderXBlockFragment(fragment, this.$el); } }); + // Note: this mock response will call the AJAX success function synchronously + // so the promise variable defined above will be available. respondWithMockXBlockFragment(requests, { html: mockXBlockHtml, resources: resources }); expect(xblockView.$el.select('.xblock-header')).toBeTruthy(); + return promise; }; it('can render an xblock with no CSS or JavaScript', function() { @@ -87,6 +91,17 @@ define([ "jquery", "js/spec/create_sinon", "URI", "js/views/xblock", "js/models/ ]); expect($('head').html()).toContain(mockHeadTag); }); + + it('aborts rendering when a dependent script fails to load', function() { + var requests = create_sinon.requests(this), + mockJavaScriptUrl = "mock.js", + promise; + spyOn($, 'getScript').andReturn($.Deferred().reject().promise()); + promise = postXBlockRequest(requests, [ + ["hash5", { mimetype: "application/javascript", kind: "url", data: mockJavaScriptUrl }] + ]); + expect(promise.isRejected()).toBe(true); + }); }); }); }); diff --git a/cms/static/js/views/xblock.js b/cms/static/js/views/xblock.js index 74fcf6464a..f13d9afc03 100644 --- a/cms/static/js/views/xblock.js +++ b/cms/static/js/views/xblock.js @@ -21,64 +21,110 @@ define(["jquery", "underscore", "js/views/baseview", "xblock/runtime.v1"], success: function(fragment) { var wrapper = self.$el, xblock; - self.renderXBlockFragment(fragment, wrapper); - xblock = self.$('.xblock').first(); - XBlock.initializeBlock(xblock); + self.renderXBlockFragment(fragment, wrapper).done(function() { + xblock = self.$('.xblock').first(); + XBlock.initializeBlock(xblock); + self.delegateEvents(); + }); } }); }, /** - * Renders an xblock fragment into the specifed element. The fragment has two attributes: + * Renders an xblock fragment into the specified element. The fragment has two attributes: * html: the HTML to be rendered * resources: any JavaScript or CSS resources that the HTML depends upon + * Note that the XBlock is rendered asynchronously, and so a promise is returned that + * represents this process. * @param fragment The fragment returned from the xblock_handler * @param element The element into which to render the fragment (defaults to this.$el) + * @returns {*} A promise representing the rendering process */ renderXBlockFragment: function(fragment, element) { - var applyResource, i, len, resources, resource; + var html = fragment.html, + resources = fragment.resources; if (!element) { element = this.$el; } + // First render the HTML as the scripts might depend upon it + element.html(html); + // Now asynchronously add the resources to the page + return this.addXBlockFragmentResources(resources); + }, - applyResource = function(value) { - var hash, resource, head; + /** + * Dynamically loads all of an XBlock's dependent resources. This is an asynchronous + * process so a promise is returned. + * @param resources The resources to be rendered + * @returns {*} A promise representing the rendering process + */ + addXBlockFragmentResources: function(resources) { + var self = this, + applyResource, + numResources, + deferred; + numResources = resources.length; + deferred = $.Deferred(); + applyResource = function(index) { + var hash, resource, head, value, promise; + if (index >= numResources) { + deferred.resolve(); + return; + } + value = resources[index]; hash = value[0]; if (!window.loadedXBlockResources) { window.loadedXBlockResources = []; } if (_.indexOf(window.loadedXBlockResources, hash) < 0) { resource = value[1]; - head = $('head'); - if (resource.mimetype === "text/css") { - if (resource.kind === "text") { - head.append(""); - } else if (resource.kind === "url") { - head.append(""); - } - } else if (resource.mimetype === "application/javascript") { - if (resource.kind === "text") { - head.append(""); - } else if (resource.kind === "url") { - $.getScript(resource.data); - } - } else if (resource.mimetype === "text/html") { - if (resource.placement === "head") { - head.append(resource.data); - } - } + promise = self.loadResource(resource); window.loadedXBlockResources.push(hash); + promise.done(function() { + applyResource(index + 1); + }).fail(function() { + deferred.reject(); + }); + } else { + applyResource(index + 1); } }; + applyResource(0); + return deferred.promise(); + }, - element.html(fragment.html); - resources = fragment.resources; - for (i = 0, len = resources.length; i < len; i++) { - resource = resources[i]; - applyResource(resource); + /** + * Loads the specified resource into the page. + * @param resource The resource to be loaded. + * @returns {*} A promise representing the loading of the resource. + */ + loadResource: function(resource) { + var head = $('head'), + mimetype = resource.mimetype, + kind = resource.kind, + placement = resource.placement, + data = resource.data; + if (mimetype === "text/css") { + if (kind === "text") { + head.append(""); + } else if (kind === "url") { + head.append(""); + } + } else if (mimetype === "application/javascript") { + if (kind === "text") { + head.append(""); + } else if (kind === "url") { + // Return a promise for the script resolution + return $.getScript(data); + } + } else if (mimetype === "text/html") { + if (placement === "head") { + head.append(data); + } } - return this.delegateEvents(); + // Return an already resolved promise for synchronous updates + return $.Deferred().resolve().promise(); } });