Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Extract packaged module loading #830

Merged
merged 7 commits into from

5 participants

@josh

At GitHub, we're using the "packaged" install, however our urls don't quite match up with what Ace tries to guess.

It'd be nice to override that module loading behavior and provide a custom function that maps modules to urls.

Should fix #732 and #655.

@josh josh commented on the diff
lib/ace/worker/worker_client.js
@@ -47,7 +47,7 @@ var WorkerClient = function(topLevelNamespaces, packagedJs, mod, classname) {
this.changeListener = this.changeListener.bind(this);
if (config.get("packaged")) {
- this.$worker = new Worker(config.get("workerPath") + "/" + packagedJs);
@josh
josh added a note

packagedJS is now an unused argument to this function now. Might be able to drop it from the WorkerClient initializer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/ace/config.js
@@ -71,6 +71,22 @@ exports.all = function() {
return lang.copyObject(options);
};
+exports.moduleUrl = function(name) {
+ var parts = name.split("/");
+ if (parts[0] !== "ace") return;
+
+ var component = parts[1];
+ var base = parts[2];
+
+ if (component == "mode") {

Triple ===, or a switch statement here please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@janjongboom

@nightwing or @fjakobs Please review

lib/ace/config.js
@@ -71,6 +71,22 @@ exports.all = function() {
return lang.copyObject(options);
};
+exports.moduleUrl = function(name) {
+ var parts = name.split("/");
+ if (parts[0] !== "ace") return;
+
+ var component = parts[1];
+ var base = parts[2];
+
+ if (component === "mode") {
+ return this.get("modePath") + "/mode-" + base + this.get("suffix");
+ } else if (component === "theme") {
+ return this.get("themePath") + "/theme-" + base + this.get("suffix");
+ } else if (component === "worker") {
+ return this.get("workerPath") + "/worker-" + base + this.get("suffix");
+ }
@josh
josh added a note

I guess this could be refactored to:

return this.get(component + "Path") + "/" + component + "-" + base + this.get("suffix");

Think thats better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nightwing
Owner

i like this (especially with one lined variant from the comment)
but i am a little bit worried about if (parts[0] !== "ace") return;, as it will change behaviour for custom modes.

also this breaks a little bit of code in cloud9 which uses packagedJS, but i'd say it's better to change that code and remove packagedJS

@josh

Should we just allow "c9/modes/foo" to map to just modes-foo.js?

@josh

Made the worker change as well.

Was there a reason to stick to the ace/mode/foo_worker convention instead of ace/worker/foo?

@fjakobs
Owner

@nightwing looks good to me. Feel free to pull. We do have a CLA from github.

@josh

@fjakobs I don't think I signed anything myself, but @defunkt and I have had patches pulled before.

@nightwing nightwing merged commit 167d7f8 into ajaxorg:master
@nightwing
Owner

pulled, thanks.

@gjtorikian
Collaborator

@nightwing @josh This pull just effectively killed Ace for Cloud9--it no longer loads due to worker path issues. @fjakobs is not familiar enough with the change to provide comment. It's also 6pm on a Friday.

@josh

@gjtorikian The WorkerClient constructor has changed. I bet that code just needs to be updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  Makefile.dryice.js
@@ -395,7 +395,7 @@ function buildAce(options) {
'ace/lib/fixoldbrowsers',
'ace/lib/event_emitter',
'ace/lib/oop',
- 'ace/mode/' + mode + '_worker'
+ 'ace/worker/' + mode
]
}],
filter: filters,
View
8 lib/ace/config.js
@@ -71,6 +71,14 @@ exports.all = function() {
return lang.copyObject(options);
};
+exports.moduleUrl = function(name) {
+ var parts = name.split("/");
+ var component = parts[1];
+ var base = parts[2];
+
+ return this.get(component + "Path") + "/" + component + "-" + base + this.get("suffix");
+};
+
exports.init = function() {
options.packaged = require.packaged || module.packaged || (global.define && define.packaged);
View
8 lib/ace/edit_session.js
@@ -835,7 +835,7 @@ var EditSession = function(text, mode) {
if (!this.$mode)
this.$setModePlaceholder();
- fetch(function() {
+ fetch(mode, function() {
require([mode], done);
});
@@ -852,13 +852,11 @@ var EditSession = function(text, mode) {
callback(_self.$modes[mode]);
}
- function fetch(callback) {
+ function fetch(name, callback) {
if (!config.get("packaged"))
return callback();
- var base = mode.split("/").pop();
- var filename = config.get("modePath") + "/mode-" + base + ".js";
- net.loadScript(filename, callback);
+ net.loadScript(config.moduleUrl(name), callback);
}
};
View
2  lib/ace/mode/coffee.js
@@ -100,7 +100,7 @@ oop.inherits(Mode, TextMode);
};
this.createWorker = function(session) {
- var worker = new WorkerClient(["ace"], "worker-coffee.js", "ace/mode/coffee_worker", "Worker");
+ var worker = new WorkerClient(["ace"], "ace/worker/coffee", "Worker");
worker.attachToDocument(session.getDocument());
worker.on("error", function(e) {
View
2  lib/ace/mode/css.js
@@ -83,7 +83,7 @@ oop.inherits(Mode, TextMode);
};
this.createWorker = function(session) {
- var worker = new WorkerClient(["ace"], "worker-css.js", "ace/mode/css_worker", "Worker");
+ var worker = new WorkerClient(["ace"], "ace/worker/css", "Worker");
worker.attachToDocument(session.getDocument());
worker.on("csslint", function(e) {
View
2  lib/ace/mode/css_worker_test.js
@@ -43,7 +43,7 @@ define(function(require, exports, module) {
"use strict";
var assert = require("../test/assertions");
-var Worker = require("./css_worker").Worker;
+var Worker = require("./worker/css").Worker;
module.exports = {
View
2  lib/ace/mode/javascript.js
@@ -128,7 +128,7 @@ oop.inherits(Mode, TextMode);
};
this.createWorker = function(session) {
- var worker = new WorkerClient(["ace"], "worker-javascript.js", "ace/mode/javascript_worker", "JavaScriptWorker");
+ var worker = new WorkerClient(["ace"], "ace/worker/javascript", "JavaScriptWorker");
worker.attachToDocument(session.getDocument());
worker.on("jslint", function(results) {
View
2  lib/ace/mode/javascript_worker_test.js
@@ -43,7 +43,7 @@ define(function(require, exports, module) {
"use strict";
var assert = require("../test/assertions");
-var JavaScriptWorker = require("./javascript_worker").JavaScriptWorker;
+var JavaScriptWorker = require("./worker/javascript").JavaScriptWorker;
module.exports = {
View
2  lib/ace/mode/json.js
@@ -79,7 +79,7 @@ oop.inherits(Mode, TextMode);
};
this.createWorker = function(session) {
- var worker = new WorkerClient(["ace"], "worker-json.js", "ace/mode/json_worker", "JsonWorker");
+ var worker = new WorkerClient(["ace"], "ace/worker/json", "JsonWorker");
worker.attachToDocument(session.getDocument());
worker.on("error", function(e) {
View
2  lib/ace/mode/json_worker_test.js
@@ -43,7 +43,7 @@ define(function(require, exports, module) {
"use strict";
var assert = require("../test/assertions");
-var Worker = require("./json_worker").JsonWorker;
+var Worker = require("./worker/json").JsonWorker;
module.exports = {
View
2  lib/ace/mode/xquery.js
@@ -120,7 +120,7 @@ oop.inherits(Mode, TextMode);
this.createWorker = function(session) {
this.$deltas = [];
- var worker = new WorkerClient(["ace"], "worker-xquery.js", "ace/mode/xquery_worker", "XQueryWorker");
+ var worker = new WorkerClient(["ace"], "ace/worker/xquery", "XQueryWorker");
var that = this;
session.getDocument().on('change', function(evt){
View
4 lib/ace/test/all_browser.js
@@ -27,12 +27,11 @@ var testNames = [
"ace/mode/coldfusion_test",
"ace/mode/css_test",
"ace/mode/css_highlight_rules_test",
- "ace/mode/css_worker",
+ "ace/worker/css",
"ace/mode/html_test",
"ace/mode/html_highlight_rules_test",
"ace/mode/javascript_test",
"ace/mode/javascript_highlight_rules_test",
- "ace/mode/javascript_worker_test",
"ace/mode/python_test",
"ace/mode/ruby_highlight_rules_test",
"ace/mode/text_test",
@@ -43,6 +42,7 @@ var testNames = [
"ace/mode/folding/pythonic_test",
"ace/mode/folding/xml_test",
"ace/mode/folding/coffee_test",
+ "ace/worker/javascript_test",
"ace/multi_select_test",
"ace/range_test",
"ace/range_list_test",
View
4 lib/ace/virtual_renderer.js
@@ -1288,9 +1288,7 @@ var VirtualRenderer = function(container, theme) {
if (!config.get("packaged"))
return callback();
- var base = name.split("/").pop();
- var filename = config.get("themePath") + "/theme-" + base + config.get("suffix");
- net.loadScript(filename, callback);
+ net.loadScript(config.moduleUrl(name), callback);
};
/**
View
4 lib/ace/worker/worker_client.js
@@ -42,12 +42,12 @@ var oop = require("../lib/oop");
var EventEmitter = require("../lib/event_emitter").EventEmitter;
var config = require("../config");
-var WorkerClient = function(topLevelNamespaces, packagedJs, mod, classname) {
+var WorkerClient = function(topLevelNamespaces, mod, classname) {
this.changeListener = this.changeListener.bind(this);
if (config.get("packaged")) {
- this.$worker = new Worker(config.get("workerPath") + "/" + packagedJs);
@josh
josh added a note

packagedJS is now an unused argument to this function now. Might be able to drop it from the WorkerClient initializer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ this.$worker = new Worker(config.moduleUrl(mod));
}
else {
var workerUrl;
Something went wrong with that request. Please try again.