In LMS, load OVA JS only in four OVA templates with requireJS. [TNL-693-outdated] #5829

jmclaus · 2014-11-03T13:15:38Z

To avoid having OVA JS files loaded in every page of the LMS and therefore clash with edXNotes, remove their entries from PIPELINE_JS in common.py and load them instead, via requireJS, in the 4 OVA templates: imageannotation.html, notes.html, text annotation.html, and videoannotation.html.

@jimabramson, @explorerleslie Please review.

Alternatively, we can also load the OVA JS in the 4 templates by using PIPELINE_JS and defining a new group called ova_js:
https://github.com/edx/edx-platform/compare/jmclaus/load_ova_js_from_templates

This runs into a problem though. The following line for example will always be executed:
https://github.com/edx/edx-platform/blob/5cc0e023103475850c6ad1d77fa0a31605f69bb9/lms/templates/videoannotation.html#L203
But in Studio, we'd like to use requireJS:
https://github.com/edx/edx-platform/blob/5cc0e023103475850c6ad1d77fa0a31605f69bb9/lms/templates/videoannotation.html#L206

jimabramson · 2014-11-04T14:19:49Z

@andy-armstrong mind having a quick look at this?

On Nov 3, 2014, at 8:15 AM, jmclaus [email protected] wrote:

To avoid having OVA JS files loaded in every page of the LMS and therefore clash with edXNotes, remove their entries from PIPELINE_JS in common.py and load them instead, via requireJS, in the 4 OVA templates: imageannotation.html, notes.html, text annotation.html, and videoannotation.html.

@jimabramson, @explorerleslie Please review.

You can merge this Pull Request by running

git pull https://github.com/edx/edx-platform jmclaus/load_ova_js_from_templates_with_requirejs
Or view, comment on, or merge it at:

https://github.com/edx/edx-platform/pull/5829

Commit Summary

Load OVA JS with requireJS in LMS.
File Changes

M lms/envs/common.py (18)
A lms/static/require-config.js (107)
M lms/templates/imageannotation.html (37)
M lms/templates/main.html (10)
M lms/templates/notes.html (20)
M lms/templates/textannotation.html (30)
M lms/templates/videoannotation.html (32)
Patch Links:

https://github.com/edx/edx-platform/pull/5829.patch
https://github.com/edx/edx-platform/pull/5829.diff
—
Reply to this email directly or view it on GitHub.

jimabramson · 2014-11-04T14:51:31Z

lms/static/require-config.js

+
+    config = {
+        // NOTE: baseUrl has been previously set in lms/templates/main.html
+        waitSeconds: 60,


what does this do?

@jimabramson It set the waiting time for loading modules before giving up. Same value as what is used in Studio.

http://requirejs.org/docs/api.html#config-waitSeconds

andy-armstrong · 2014-11-04T16:03:13Z

@jmclaus I don't think this can work in production as otherwise we'd already be using RequireJS in LMS. My understanding is that the problem is with the way we serve static files out of the CDN using file suffices that include a hash of the file's content. RequireJS is unable to deal with this as it doesn't understand the suffices.

Our eventual plan was to get around this problem by using RequireJS Optimizer, as then the files are not loaded dynamically at runtime, so the extra suffix doesn't cause a problem. Maybe you can do that here and see if that works. It would be fantastic to start using RequireJS in LMS.

I recommend creating a production-like sandbox and verifying that things works correctly there. @cahrens tells me that you'll need devops to set that up for you. If that works then maybe I'm concerned for no reason.

polesye · 2014-11-05T14:34:20Z

@andy-armstrong it looks like we have access to both files (with and without hash).
For example, link from production:
https://d37djvu3ytnwxt.cloudfront.net/static/js/lms-application.af5f2475e128.js - works
https://d37djvu3ytnwxt.cloudfront.net/static/js/lms-application.js - also works.

So, we can use the second link in RequireJS, but issues with browser's cache may appear for such kind of links.
We added urlArgs into config with git revision for cache-busting .

See d2202f5 .

andy-armstrong · 2014-11-05T15:02:08Z

@polesye Thanks for the explanation. I agree that the raw files are there, but I don't think we should use cache-busting in production even as a workaround. That will cause a lot of unnecessary fetches for each page load. @feanil would you agree?

Thinking about this more last night, I see this as a good opportunity to do RequireJS 'the right way' so that we can all start to benefit from it. Can you add RequireJS Optimizer to this PR so that we can have cached access to these files, or do you see an issue with that?

andy-armstrong · 2014-11-05T15:03:39Z

@polesye my mistake, I see now that you said that you use the git revision, so it will only bust the cache on each release. This is essentially what Studio is doing. I don't like it as every week/hotfix users will have to re-download all the files, but at least it is better than what I thought you were saying.

andy-armstrong · 2014-11-05T15:11:20Z

@cahrens What's your opinion about the approach here?

andy-armstrong · 2014-11-05T15:13:28Z

@polesye @cahrens I'd like us to pick one approach that we'll use going forward for both CMS and LMS that gives us the kind of performance we want to see. I believe that this should be RequireJS Optimizer but I'm willing to entertain the idea that a cache busting technique is preferable. If we do want to use cache busting then we should use the same approach in both CMS and LMS.

polesye · 2014-11-06T12:55:01Z

@andy-armstrong I'm totally agree with you about RequireJS Optimizer. We have appropriate story in our backlog and RequireJS Optimizer is not in a scope of this task. It's just a temporary solution to unblock us with Student Notes.

andy-armstrong · 2014-11-06T15:38:14Z

👍 Thanks for the discussion today. I agree with leaving the Optimizer piece out so that we can land Student Notes. I would recommend that you add a comment in the RequireJS cache busting code to say that this is a temporary solution that will be replaced by Optimizer.

@singingwolfboy FYI, you might want to see what @anton has done here for RequireJS usage in LMS. He's using a slightly different approach from what you did in Studio, but it has the same effect. This may also affect Open Source PRs if they start contributing LMS changes with RequireJS.

singingwolfboy · 2014-11-06T15:49:26Z

lms/templates/main.html

+        ## Using what amounts to a random number in the Development environment for cache-busting
+        var urlArgs = "bust=" + (new Date()).getTime();
+    % else:
+        var urlArgs = "v=${git.revision}";


Interesting! I like this idea.

there are some warnings against using this technique discussed in this SO topic, and an alternate suggestion to revision the filenames by directory instead of query string. Would that be a feasible alternative in this case?

there are some warnings against using this technique discussed in this SO topic, and an alternate suggestion to revision the filenames by directory instead of query string. Would that be a feasible alternative in this case?

@andy-armstrong I think, no. We should change STATIC_URL for LMS to use directory instead of query string. In this case, performance degradation occurs for all static (not only for loaded via requirejs).

Good point, OK thanks.

So if I understand correctly, under the present setup, performance/behavior improves a little for most pages since ova is not loaded all the time anymore, otherwise it's unchanged.

On the few pages using ova:
a) first load performance will degrade somewhat due to adding a number of new individual files to download
b) a user behind a proxy that refuses to cache urls with query strings will be downloading all these files on all loads, not just the first.

I'm concerned about the impact of this on low-bandwidth users, but if I've misunderstood or I'm missing how the solution addresses this issue, then I'll withdraw that concern.

So if I understand correctly, under the present setup, performance/behavior improves a little for most pages since ova is not loaded all the time anymore, otherwise it's unchanged.

On the few pages using ova:
a) first load performance will degrade somewhat due to adding a number of new individual files to download
b) a user behind a proxy that refuses to cache urls with query strings will be downloading all these files on all loads, not just the first.

@jimabramson

a. Correct, we are stripping 18 JS OVA files from from LMS main vendor. That should improve loading time a little for most pages. And yes, for the few pages using OVA, it will degrade things as the files will be loaded there by requireJS as 18 separate entities. But they will not block page rendering.

b. Indeed, some proxy servers may not cache these JavaScript files but we still have client-side caching by browser after first load.

cahrens · 2014-11-12T19:34:51Z

What is the status of this PR?

jmclaus · 2014-11-13T12:48:39Z

@cahrens @jimabramson @andy-armstrong @polesye @singingwolfboy

We were waiting to have access to a prod-like sandbox with a CDN connected to it. It's accessible now:

jmclaus.m.sandbox.edx.org

So I checked if everything was OK with the Harvard Annotation Test Course:

http://studio.jmclaus.m.sandbox.edx.org/course/ManTestX/ManTest3/2014

And found something interesting. All works fine in LMS and Studio, no script error or 404.

But things break when LMS is accessed via the View Live button.

Here are the Request Headers Status and for all three scenarios regarding 'require-config.js':

REQUEST HEADERS AND STATUS FOR 'require-config':

CMS
GET /static/d2202f5/require-config.b0e0a587f6bb.js HTTP/1.1
Host: d1rfybgqbxfvcx.cloudfront.net
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
Accept: /
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.122 Safari/537.36
Referer: http://studio.jmclaus.m.sandbox.edx.org/course/ManTestX/ManTest3/2014
Accept-Encoding: gzip,deflate,sdch
Accept-Language: en-US,en;q=0.8,fr;q=0.6

STATUS --> 200

LMS LOADED DIRECTLY
GET /static/require-config.0e68842c5f58.js HTTP/1.1
Host: d1rfybgqbxfvcx.cloudfront.net
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
Accept: /
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.122 Safari/537.36
Referer: http://jmclaus.m.sandbox.edx.org/courses/ManTestX/ManTest3/2014/courseware/46c9ebd233564d498447a9050ce50ab4/5589e7def70047229438c0cb3ced9dc9/
Accept-Encoding: gzip,deflate,sdch
Accept-Language: en-US,en;q=0.8,fr;q=0.6

STATUS --> 200

LMS LOADED FROM CMS
GET /static/require-config.b0e0a587f6bb.js HTTP/1.1
Host: d1rfybgqbxfvcx.cloudfront.net
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
Accept: /
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.122 Safari/537.36
Referer: http://jmclaus.m.sandbox.edx.org/courses/ManTestX/ManTest3/2014/courseware/46c9ebd233564d498447a9050ce50ab4/
Accept-Encoding: gzip,deflate,sdch
Accept-Language: en-US,en;q=0.8,fr;q=0.6

STATUS --> 404

Looks like we have an incorrect hash.

jmclaus · 2014-11-17T15:50:58Z

@lduarte1991 FYI.

jmclaus · 2014-11-18T10:13:30Z

@cahrens @jimabramson @andy-armstrong @lduarte1991@polesye @singingwolfboy

@polesye found an interesting paragraph in the Django docs regarding static file namespacing:
https://docs.djangoproject.com/en/1.6/howto/static-files/#configuring-static-files

So we basically renamed lms/static/require-config.js to lms/static/require-config-lms.js to namespace it and avoid clashing with studio/static/require-con.js.

It works, [View Live Version] and [Preview Changes] now function correctly, please have a look at the following CDN-enabled sandbox (due to expire, unless reconducted, this Friday):

http://studio.jmclaus.m.sandbox.edx.org/

andy-armstrong · 2014-11-18T14:52:50Z

Wow, nice catch @polesye! I would never have thought to look at that.

There's some strange permission failure on Bok Choy, but otherwise everything else looks good. It probably will go away with another test run.

👍 I can't wait to build upon this...

jmclaus · 2014-11-19T15:39:24Z

@andy-armstrong Rebased, the test run is now positive.

jmclaus · 2014-12-05T17:43:04Z

@cahrens @jimabramson @andy-armstrong @lduarte1991 @polesye @explorerleslie @singingwolfboy @tymofij @olmar

We recently got an AWS instance loading up the CATCH software (the backend storage we needed for the Harvard Annotation). We were then able to fully test OVA with the following test plan:

https://docs.google.com/a/edx.org/document/d/10BcGZRlOHTyBiBsk6OreUXzkU3m8Ju2K6hGic6Lw-9k

to see if the above would break anything. Very good news, to my knowledge, all functionality is intact and I didn't notice any console or network error. A sandbox has been built:

http://jmclaus.m.sandbox.edx.org/courses/ManTestX/ManTest3/2014/

@lduarte1991 I wouldn't mind if you could have a quick look, I may missed certain things that may be obvious to you.

We will not merge this into master but rather the following PR into our feature branch, feature/edxnotes:

https://github.com/edx/edx-platform/pull/6158

The feature branch will hopefully be merged into master mid-January.

lduarte1991 · 2014-12-05T19:07:38Z

@jmclaus Seems to be working as intended!

jmclaus · 2014-12-06T10:00:19Z

@lduarte1991 Good news, thanks.

jmclaus · 2014-12-09T15:58:08Z

@cahrens @jimabramson @andy-armstrong @lduarte1991 @polesye @explorerleslie @singingwolfboy @tymofij @olmar

Tested it with a CDN-enabled sandbox, it works nicely. I will merge this once the tests pass.

…with_requirejs In LMS, load OVA JS only in four OVA templates with requireJS. [TNL-693-outdated]

jmclaus force-pushed the jmclaus/load_ova_js_from_templates_with_requirejs branch 2 times, most recently from c63fcff to 0b0416d Compare November 4, 2014 08:05

jimabramson reviewed Nov 4, 2014
View reviewed changes

singingwolfboy reviewed Nov 6, 2014
View reviewed changes

jmclaus force-pushed the jmclaus/load_ova_js_from_templates_with_requirejs branch 2 times, most recently from 501895f to 42b9cd2 Compare November 19, 2014 09:55

cahrens mentioned this pull request Nov 24, 2014

Enable RequireJS optimizer for combined login/registration page #6036

Closed

jmclaus force-pushed the jmclaus/load_ova_js_from_templates_with_requirejs branch 3 times, most recently from be61283 to 1358676 Compare December 5, 2014 14:22

jmclaus mentioned this pull request Dec 5, 2014

Load OVA JS with RequireJS [TNL-693] #6158

Closed

jmclaus changed the title ~~In LMS, load OVA JS only in four OVA templates with requireJS. [TNL-693]~~ In LMS, load OVA JS only in four OVA templates with requireJS. [TNL-693-outdated] Dec 5, 2014

Load OVA JS with requireJS in LMS.

082cb3b

jmclaus force-pushed the jmclaus/load_ova_js_from_templates_with_requirejs branch from 1358676 to 082cb3b Compare December 9, 2014 14:50

jmclaus pushed a commit that referenced this pull request Dec 9, 2014

Merge pull request #5829 from edx/jmclaus/load_ova_js_from_templates_…

910466e

…with_requirejs In LMS, load OVA JS only in four OVA templates with requireJS. [TNL-693-outdated]

jmclaus merged commit 910466e into master Dec 9, 2014

jmclaus deleted the jmclaus/load_ova_js_from_templates_with_requirejs branch December 9, 2014 16:03

cahrens mentioned this pull request Dec 11, 2014

remove mathjax form main xmodule requirement and add it on problem addin... #6048

Closed

andy-armstrong mentioned this pull request Jan 26, 2015

Add RequireJS to the LMS #6765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In LMS, load OVA JS only in four OVA templates with requireJS. [TNL-693-outdated] #5829

In LMS, load OVA JS only in four OVA templates with requireJS. [TNL-693-outdated] #5829

jmclaus commented Nov 3, 2014

jimabramson commented Nov 4, 2014

jimabramson Nov 4, 2014

jmclaus Nov 5, 2014

andy-armstrong commented Nov 4, 2014

polesye commented Nov 5, 2014

andy-armstrong commented Nov 5, 2014

andy-armstrong commented Nov 5, 2014

andy-armstrong commented Nov 5, 2014

andy-armstrong commented Nov 5, 2014

polesye commented Nov 6, 2014

andy-armstrong commented Nov 6, 2014

singingwolfboy Nov 6, 2014

jimabramson Nov 6, 2014

polesye Nov 7, 2014

jimabramson Nov 7, 2014

jmclaus Nov 10, 2014

cahrens commented Nov 12, 2014

jmclaus commented Nov 13, 2014

jmclaus commented Nov 17, 2014

jmclaus commented Nov 18, 2014

andy-armstrong commented Nov 18, 2014

jmclaus commented Nov 19, 2014

jmclaus commented Dec 5, 2014

lduarte1991 commented Dec 5, 2014

jmclaus commented Dec 6, 2014

jmclaus commented Dec 9, 2014

In LMS, load OVA JS only in four OVA templates with requireJS. [TNL-693-outdated] #5829

In LMS, load OVA JS only in four OVA templates with requireJS. [TNL-693-outdated] #5829

Conversation

jmclaus commented Nov 3, 2014

jimabramson commented Nov 4, 2014

jimabramson Nov 4, 2014

Choose a reason for hiding this comment

jmclaus Nov 5, 2014

Choose a reason for hiding this comment

andy-armstrong commented Nov 4, 2014

polesye commented Nov 5, 2014

andy-armstrong commented Nov 5, 2014

andy-armstrong commented Nov 5, 2014

andy-armstrong commented Nov 5, 2014

andy-armstrong commented Nov 5, 2014

polesye commented Nov 6, 2014

andy-armstrong commented Nov 6, 2014

singingwolfboy Nov 6, 2014

Choose a reason for hiding this comment

jimabramson Nov 6, 2014

Choose a reason for hiding this comment

polesye Nov 7, 2014

Choose a reason for hiding this comment

jimabramson Nov 7, 2014

Choose a reason for hiding this comment

jmclaus Nov 10, 2014

Choose a reason for hiding this comment

cahrens commented Nov 12, 2014

jmclaus commented Nov 13, 2014

REQUEST HEADERS AND STATUS FOR 'require-config':

jmclaus commented Nov 17, 2014

jmclaus commented Nov 18, 2014

andy-armstrong commented Nov 18, 2014

jmclaus commented Nov 19, 2014

jmclaus commented Dec 5, 2014

lduarte1991 commented Dec 5, 2014

jmclaus commented Dec 6, 2014

jmclaus commented Dec 9, 2014