Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Moved paths and shim for require.config to a central location #161

Merged
merged 1 commit into from
Apr 20, 2014

Conversation

JGulbronson
Copy link
Member

require.config now looks in a central location to find paths and shim, so we only have to modify it in one location when adding new libraries.

}
});
require(['../config_settings'], function(config_settings) {
var test_paths = config_settings.paths;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be testPaths, not test_paths.

We have our own pretty nonstandard conventions for module imports having snake_case names, that should probably honestly be changed, but that's outside the scope of this change. Aside from module imports, everything should be camelCased

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, fixed to camelCase!

@JGulbronson
Copy link
Member Author

So I still needed jquery, but was able to eliminate require, not sure why that was in there actually.

@jlfwong
Copy link
Member

jlfwong commented Apr 20, 2014

Ahh, I see what's going on. Right now, the paths aren't consistent for two reasons:

  1. Static assets are being served under different paths in tests and in development
  2. You aren't requiring chai and mocha using ext/ prefixed paths, which is our convention. This diff should fix all those things.

This diff makes chai and mocha require'able with ext/ prefixes, and changes the path for the static test server to be consistent with development.

diff --git i/Makefile w/Makefile
index bb90595..441d988 100644
--- i/Makefile
+++ w/Makefile
@@ -108,8 +108,8 @@ test: require_virtualenv_in_dev
    PYTHONPATH=.. nosetests -a '!slow'

 js-test:
-   cd server/static/; \
-   python -c 'import webbrowser; webbrowser.open("http://127.0.0.1:8000/js/js_tests/test.html")'; \
+   cd server/; \
+   python -c 'import webbrowser; webbrowser.open("http://127.0.0.1:8000/static/js/js_tests/test.html")'; \
    python -mSimpleHTTPServer 8000

 clean:
diff --git i/server/static/js/js_tests/main.js w/server/static/js/js_tests/main.js
index 397680c..5cc12f7 100644
--- i/server/static/js/js_tests/main.js
+++ w/server/static/js/js_tests/main.js
@@ -2,15 +2,15 @@
 // testing libraries mocha and chai
 require(['../config_settings'], function(config_settings) {
   var testPaths = config_settings.paths;
-  testPaths['chai'] = 'js_tests/vendor/chai';
-  testPaths['mocha'] = 'js_tests/vendor/mocha';
+  testPaths['ext/chai'] = 'js_tests/vendor/chai';
+  testPaths['ext/mocha'] = 'js_tests/vendor/mocha';
   require.config({
     baseUrl: '../',
     paths: testPaths,
     shim: config_settings.shim
   });

-  require(['chai', 'mocha', 'ext/jquery'],
+  require(['ext/chai', 'ext/mocha'],
       function(chai){

     /*globals mocha */
diff --git i/server/static/js/js_tests/schedule_test.js w/server/static/js/js_tests/schedule_test.js
index b9394fe..cb9adef 100644
--- i/server/static/js/js_tests/schedule_test.js
+++ w/server/static/js/js_tests/schedule_test.js
@@ -1,11 +1,11 @@
 define(function(require) {
-  var expect = require('chai').expect;
-  var $ = require('jquery');
+  var expect = require('ext/chai').expect;
+  var $ = require('ext/jquery');
   var schedule_parser = require('schedule_parser');

   describe('Schedule parsing', function() {
     var parsedSchedulePromise = $.get(
-        '/sample_schedule.txt').then(function(r) {
+        '/static/sample_schedule.txt').then(function(r) {
       return schedule_parser.parseSchedule(r);
     });

diff --git i/server/static/js/js_tests/transcript_test.js w/server/static/js/js_tests/transcript_test.js
index fb66cb3..7d409b2 100644
--- i/server/static/js/js_tests/transcript_test.js
+++ w/server/static/js/js_tests/transcript_test.js
@@ -1,11 +1,11 @@
 define(function(require) {
-  var expect = require('chai').expect;
+  var expect = require('ext/chai').expect;
   var transcript = require('transcript');
-  var $ = require('jquery');
+  var $ = require('ext/jquery');

   describe('Transcript parsing', function() {
     var parsedTranscriptPromise = $.get(
-        '/sample_transcript.txt').then(function(r) {
+        '/static/sample_transcript.txt').then(function(r) {
       return transcript.parseTranscript(r);
     });

@JGulbronson
Copy link
Member Author

Awesome, thanks! I'll make those changes.

@JGulbronson
Copy link
Member Author

There we go, works fine on my machine now. So to be clear, the problem stemmed from the assets being served under two different locations, which resulted in the path's being different?

require(['require', 'chai', 'mocha', 'ext/jquery'],
function(require, chai){
require(['ext/chai', 'ext/mocha'],
function(chai){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, didn't catch this in a previous diff, but there should be a space between the (chai) and the { (so it should be (chai) {. Also this should fit on the previous line now, no?

@jlfwong
Copy link
Member

jlfwong commented Apr 20, 2014

Once you fix the tiny nit for (chai){, this LGTM. Feel free to merge after you fix it.

JGulbronson added a commit that referenced this pull request Apr 20, 2014
Moved paths and shim for require.config to a central location
@JGulbronson JGulbronson merged commit 7c714fe into UWFlow:master Apr 20, 2014
divad12 added a commit that referenced this pull request Apr 20, 2014
... because we don't even use the minified files on production anyway!

This change was motivated because #161 broke
the JS compile step in deploy.sh. Ideally we should fix that and re-enable
minification, but until we re-enable minification there is no point to compiling
JS on deploy.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants