Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

bug 739735, 739737: Template libraries and includes #5

Merged
merged 1 commit into from

2 participants

@lmorchard
  • template() - fixes bug 739735, executes another template, returns
    the output. Useful for wrapping other templates in additional logic.

  • require() - fixes bug 739737, executes another template, captures
    exported data & methods to be cached and returned. Useful for building
    reusable code libraries.

  • Auto-require feature, allows server to be configured with a set of
    template libraries that get automatically loaded for every request.
    Useful for building out DekiScript API shims that will be made
    available to all migrated templates.

  • Expose node-fibers Future and HTTP request access to templates for
    building libraries.

  • Additional notes on the setCaseVariantAliases hack

  • Tests and fixtures

@lmorchard lmorchard bug 739735, 739737: Template libraries and includes
* template() - fixes bug 739735, executes another template, returns
  the output. Useful for wrapping other templates in additional logic.

* require() - fixes bug 739737, executes another template, captures
  exported data & methods to be cached and returned. Useful for building
  reusable code libraries.

* Auto-require feature, allows server to be configured with a set of
  template libraries that get automatically loaded for every request.
  Useful for building out DekiScript API shims that will be made
  available to all migrated templates.

* Expose node-fibers Future and HTTP request access to templates for
  building libraries.

* Additional notes on the setCaseVariantAliases hack

* Tests and fixtures
ceab707
@lmorchard

Haven't started building out any Deki API shims with this yet, but I think this makes for a good start.

@groovecoder groovecoder commented on the diff
lib/kumascript/api.js
((12 lines not shown))
},
+
+ // #### Future
+ //
+ // Allow access to node-fiber Future from templates.
+ Future: Future,
+
+ // #### request
+ //
+ // Allow access to mikeal/request in templates and libraries, so they can
+ // easily make HTTP requests.
+ //
+ // TODO: Very permissive. Should there be more restrictions on net access?
@groovecoder Owner

I think it's okay to err on the side of permissiveness until we have a problem. Templates are only editable by a subset of wiki users anyway(?) so I'd rather empower the template writers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@groovecoder groovecoder commented on the diff
lib/kumascript/api.js
@@ -237,6 +285,88 @@ var APIContext = ks_utils.Class({
_.each(args, function (v, i) {
$this['$'+i] = v;
});
+ },
+
+ // #### template(name, arguments)
+ //
+ // Attempt to load and execute a template with the given name and
+ // arguments. The output, if any, is returned. Errors, if any, are pushed
+ // up to the macro processor
+ template: function (name, args) {
+
+ // TODO: Implement caching here by building a key out of hashed args?
@groovecoder Owner

yeah let's not worry about it until we do a post-Kuma performance pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@groovecoder groovecoder commented on the diff
tests/test-api.js
((18 lines not shown))
// Main test case starts here
module.exports = {
+ setUp: function (next) {
+ this.test_server = ks_test_utils.createTestServer();
@groovecoder Owner

just noticed this while looking for this class - should test-utils.js live under tests/ ?

No. nodeunit runs anything it finds under tests/ as a test script. Don't want that, since test-utils.js is a library.

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

Looks great! My only nit: the autorequired, the library, and the template fixtures all use a generic 'result' object. Makes it a bit confusing to follow the test code when looking at it all at once. Still need to spot-check.

@groovecoder
Owner

And I forget - did we decide to start using jshint on all this code? 8872 errors right now :(

@lmorchard

Not sure what you mean about a generic result object. Can you point to an example?

Also, how are you running jshint? This gets me no errors:

./node_modules/.bin/jshint lib tests
@lmorchard

When you see something like "8872 errors", can you pastebin or gist things like that? My first questions will always be, how did you get that result and what exactly did you see?

@groovecoder
Owner

autorequire-lib1.ejs, library1.ejs, and template-exec-template.ejs all output nearly identical "The result is a" messages. It makes all the document and template fixtures so similar it's hard to tell which is for which.

@groovecoder
Owner

Awesome. spot-checked everything. Is it possible to have the same library syntax between the autorequired libraries and the standard libraries? Auto-required libraries require module.exports = buildAPI() while standard libraries just use exports.functionName ?

@lmorchard

We should probably just use module.exports for everything. It's more verbose, but works. (ie. module.exports.functionName instead of exports.functionName)

Assigning to one of the members of exports or module worked (eg. exports.functionName or module.exports), but not assigning to exports itself. The result doesn't get carried up into require

I think it has to do with the way pass-by-reference works in JS, and I am not entirely sure how node.js itself handles it in implementing CommonJS modules.

@lmorchard

I could rework all the fixtures, but I was trying to keep them simple and consistent so I could hammer them out fast. Many of them, even beyond this PR, have very similar text. They should all work, though.

@groovecoder groovecoder merged commit 00948c5 into mozilla:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 30, 2012
  1. @lmorchard

    bug 739735, 739737: Template libraries and includes

    lmorchard authored
    * template() - fixes bug 739735, executes another template, returns
      the output. Useful for wrapping other templates in additional logic.
    
    * require() - fixes bug 739737, executes another template, captures
      exported data & methods to be cached and returned. Useful for building
      reusable code libraries.
    
    * Auto-require feature, allows server to be configured with a set of
      template libraries that get automatically loaded for every request.
      Useful for building out DekiScript API shims that will be made
      available to all migrated templates.
    
    * Expose node-fibers Future and HTTP request access to templates for
      building libraries.
    
    * Additional notes on the setCaseVariantAliases hack
    
    * Tests and fixtures
This page is out of date. Refresh to see the latest.
View
150 lib/kumascript/api.js
@@ -28,6 +28,7 @@ var util = require('util'),
wait = Future.wait,
request = require('request'),
+ ks_macros = require(__dirname + '/macros'),
ks_utils = require(__dirname + '/utils');
// ### BaseAPI
@@ -213,16 +214,63 @@ var APIContext = ks_utils.Class({
}
},
+ // #### initialize
+ //
// Initialize the API context.
initialize: function (options) {
_.each(this.options.apis, _.bind(this.installAPI, this));
+
+ // Create a new cache for required templates.
+ this._require_cache = {};
},
+
+ // #### Future
+ //
+ // Allow access to node-fiber Future from templates.
+ Future: Future,
+
+ // #### request
+ //
+ // Allow access to mikeal/request in templates and libraries, so they can
+ // easily make HTTP requests.
+ //
+ // TODO: Very permissive. Should there be more restrictions on net access?
@groovecoder Owner

I think it's okay to err on the side of permissiveness until we have a problem. Templates are only editable by a subset of wiki users anyway(?) so I'd rather empower the template writers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ request: request,
// Install a new instance of the given API class, with the given name.
installAPI: function (cls, name) {
setCaseVariantAliases(this, name, new cls({parent: this}));
},
+ // #### buildAPI(prototype)
+ //
+ // Utility method for building a new BaseAPI-based API, useful for building
+ // APIs in templates. Handy in conjuction with autorequire. See tests for
+ // details.
+ buildAPI: function (proto) {
+ var cls = ks_utils.Class(BaseAPI, proto);
+ return new cls({ parent: this });
+ },
+
+ // #### performAutoRequire
+ //
+ // Auto-require some templates and install the exports as APIs. This is
+ // kind of a hack, but I wanted to use the require method.
+ performAutoRequire: function () {
+ if (this.options.autorequire) {
+ var $this = this;
+ // require() expects to run inside a Fiber
+ Fiber(function () {
+ _.each($this.options.autorequire,
+ function (tmpl_name, install_name) {
+ var exports = $this.require(tmpl_name);
+ setCaseVariantAliases($this, install_name, exports);
+ }
+ );
+ }).run();
+ }
+ },
+
// Given a list of arguments, make them available to a template as $0..$n
// variables.
setArguments: function (args) {
@@ -237,6 +285,88 @@ var APIContext = ks_utils.Class({
_.each(args, function (v, i) {
$this['$'+i] = v;
});
+ },
+
+ // #### template(name, arguments)
+ //
+ // Attempt to load and execute a template with the given name and
+ // arguments. The output, if any, is returned. Errors, if any, are pushed
+ // up to the macro processor
+ template: function (name, args) {
+
+ // TODO: Implement caching here by building a key out of hashed args?
@groovecoder Owner

yeah let's not worry about it until we do a post-Kuma performance pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ // Probably not a big win, since the result of the template calling
+ // this template will itself be cached during macro evaluation.
+
+ // Try loading the template, using a node-fibers Future to avoid
+ // imposing async on templates.
+ var $this = this,
+ future = new Future(),
+ loader = $this.loader,
+ errors = $this.errors,
+ output = '';
+
+ try {
+ // Try loading the template...
+ loader.get(name, function (err, tmpl) {
+ if (!err) {
+ // Try executing the template...
+ tmpl.execute(args, $this, function (err, result) {
+ if (err) {
+ // There was an error executing the template. :(
+ var tok = {type: 'none', name: 'none'};
+ errors.push(new ks_macros.TemplateExecutionError(
+ {token: tok, error: err}));
+ }
+ output = result;
+ future['return']();
+ });
+ } else {
+ // There was an error loading the template. :(
+ errors.push(new ks_macros.TemplateLoadingError(
+ {name: name, error: err}));
+ future['return']();
+ }
+ });
+ } catch (e) {
+ // There was an error executing the template. :(
+ errors.push(new ks_macros.TemplateLoadingError(
+ {name: name, error: e}));
+ future['return']();
+ }
+
+ // Wait here for the async magic to complete.
+ future.wait();
+
+ return output;
+ },
+
+ // #### require(path)
+ //
+ // Attempts to load and execute a template which, as a side effect, can
+ // populate an exports object in quasi-CommonJS style. The template output
+ // is ignored.
+ require: function (name) {
+
+ // Use an internal cache, so that repeated require() calls reuse the
+ // previously loaded results.
+ if (!(name in this._require_cache)) {
+
+ // Let's pretend we're following CommonJS module conventions
+ this.module = { exports: {} };
+ this.exports = this.module.exports;
+
+ // This is just like calling a template, only we ignore the output
+ // and return the side effect of populating exports.
+ var output = this.template(name, []);
+ this._require_cache[name] = this.module.exports;
+
+ // Clean out the exports
+ delete this.module;
+ delete this.exports;
+ }
+
+ return this._require_cache[name];
}
});
@@ -254,17 +384,28 @@ function htmlEscape (s) {
//
// THIS IS A BIG FAT HAIRY HACK. And, it has a long name, so no one forgets it.
//
-// Set a property on an object with aliases of various mixed cases.
-// (ie. wiki -> Wiki, string -> String, Page.Location -> page.location)
+// Set a property on an object with aliases of various mixed cases. For example:
+//
+// page.location, Page.location, Page.Location, page.Location
//
// There's no such thing as case-insensitive object keys in JS, but
// apparently there are in DekiScript. This hack just covers the most
// common slack in known MDN templates.
//
-// Harmony proxies might be a solution, but requires a compiled C++ component
-// and seems like overkill.
+// Harmony proxies might be a solution, in that an attribute access proxy could
+// intercept and squash case. But, it requires a compiled C++ component and
+// seems like overkill.
//
// <https://github.com/samshull/node-proxy>
+//
+// Running with the V8 option --harmony_proxies seems like a lead, especially
+// when combined with this (thanks, David Bruant):
+//
+// <https://github.com/Benvie/Direct-Proxies-Shim-Shim-for-Node-and-Chrome>
+//
+// But, `--harmony_proxies` is off by default, which makes me hesitant to rely
+// on it for a production service.
+//
function setCaseVariantAliases($this, name, val) {
// As-is from the source.
@@ -283,5 +424,6 @@ function setCaseVariantAliases($this, name, val) {
module.exports = {
APIContext: APIContext,
BaseAPI: BaseAPI,
+ KumaAPI: KumaAPI,
WikiAPI: WikiAPI
};
View
11 lib/kumascript/macros.js
@@ -51,6 +51,13 @@ var MacroProcessor = ks_utils.Class({
var loader_options = $this.options.loader_options,
loader = new $this.options.loader_class(loader_options);
+ // Give the API context access to the loader and errors
+ api_ctx.loader = loader;
+ api_ctx.errors = errors;
+
+ // Kick off loading any autorequire templates.
+ api_ctx.performAutoRequire();
+
// Attempt to parse the document, trap errors
var tokens = [];
try { tokens = ks_parser.parse(src); }
@@ -127,7 +134,7 @@ var MacroProcessor = ks_utils.Class({
}, $this.options.queue_concurrency);
names.forEach(function (name) { template_q.push(name); });
- template_q.drain = function (err) { next_cb(errors); }
+ template_q.drain = function (err) { next_cb(errors); };
},
// #### Evaluate macros
@@ -173,7 +180,7 @@ var MacroProcessor = ks_utils.Class({
}, $this.options.queue_concurrency);
hashes.forEach(function (hash) { macro_q.push(hash); });
- macro_q.drain = function (err) { next_cb(errors); }
+ macro_q.drain = function (err) { next_cb(errors); };
}
});
View
7 lib/kumascript/server.js
@@ -156,7 +156,8 @@ var Server = ks_utils.Class({
var api_ctx = new ks_api.APIContext({
request: req,
response: res,
- source: src
+ source: src,
+ autorequire: $this.options.autorequire
});
try {
@@ -166,7 +167,7 @@ var Server = ks_utils.Class({
res.log.error(error.message, {
name: 'kumascript',
template: '%s: %s',
- args: [ error.name, error.message ]
+ args: [ error.name, error.message, error.stack ]
});
});
}
@@ -176,7 +177,7 @@ var Server = ks_utils.Class({
res.log.error(error.message, {
name: 'kumascript',
template: '%s: %s',
- args: [ error.name, error.message ]
+ args: [ error.name, error.message, error.stack ]
});
// HACK: If all else fails, send back the source
res.send(src);
View
3  tests/fixtures/documents/autorequire-expected.txt
@@ -0,0 +1,3 @@
+This exercises autorequire:
+
+* The result is a SUCCESS!
View
3  tests/fixtures/documents/autorequire.txt
@@ -0,0 +1,3 @@
+This exercises autorequire:
+
+* {{ autorequire-used('SUCCESS') }}
View
3  tests/fixtures/documents/library-test-expected.txt
@@ -0,0 +1,3 @@
+Testing a library:
+
+* The result was a SUCCESS!
View
3  tests/fixtures/documents/library-test.txt
@@ -0,0 +1,3 @@
+Testing a library:
+
+* {{ library1-used("SUCCESS!") }}
View
3  tests/fixtures/documents/template-exec-expected.txt
@@ -0,0 +1,3 @@
+Testing a library:
+
+* The result was a SUCCESS!
View
3  tests/fixtures/documents/template-exec.txt
@@ -0,0 +1,3 @@
+Testing a library:
+
+* {{ template-exec('SUCCESS') }}
View
7 tests/fixtures/templates/autorequire-lib1.ejs
@@ -0,0 +1,7 @@
+<%
+module.exports = buildAPI({
+ result: function (str) {
+ return str + '!';
+ }
+});
+%>
View
1  tests/fixtures/templates/autorequire-used.ejs
@@ -0,0 +1 @@
+The result is a <%= test_api.result($0) %>
View
2  tests/fixtures/templates/library1-used.ejs
@@ -0,0 +1,2 @@
+<% var library1 = require('library1'); %>
+<%= library1.result($0) %>
View
5 tests/fixtures/templates/library1.ejs
@@ -0,0 +1,5 @@
+<%
+exports.result = function (str) {
+ return "The result was a " + str;
+};
+%>
View
1  tests/fixtures/templates/template-exec-template.ejs
@@ -0,0 +1 @@
+The result was a <%= $0 %>!
View
1  tests/fixtures/templates/template-exec.ejs
@@ -0,0 +1 @@
+<%= template('template-exec-template', [$0]) %>
View
118 tests/test-api.js
@@ -19,6 +19,7 @@ var util = require('util'),
ks_loaders = kumascript.loaders,
ks_templates = kumascript.templates,
ks_api = kumascript.api,
+ ks_server = kumascript.server,
ks_macros = kumascript.macros,
ks_test_utils = kumascript.test_utils;
@@ -59,9 +60,63 @@ var DemoAPI = ks_utils.Class(ks_api.BaseAPI, {
});
+// Reusable fixture-based test runner
+function performTestRequest(test, expected_fn, result_url) {
+ fs.readFile(expected_fn, 'utf8', function (err, expected) {
+ var opts = {
+ url: result_url,
+ headers: { 'X-FireLogger': 'plaintext' }
+ };
+ request(opts, function (err, resp, result) {
+ test.equal(result.trim(), expected.trim());
+ test.done();
+ });
+ });
+}
+
// Main test case starts here
module.exports = {
+ setUp: function (next) {
+ this.test_server = ks_test_utils.createTestServer();
@groovecoder Owner

just noticed this while looking for this class - should test-utils.js live under tests/ ?

No. nodeunit runs anything it finds under tests/ as a test script. Don't want that, since test-utils.js is a library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ this.server = new ks_server.Server({
+ port: 9000,
+ document_url_template: "http://localhost:9001/documents/{path}.txt",
+ template_url_template: "http://localhost:9001/templates/{name}.ejs",
+ template_class: "EJSTemplate",
+ autorequire: {
+ "test_api": "autorequire-lib1"
+ }
+ });
+ this.server.listen();
+ next();
+ },
+
+ // Kill all the servers on teardown.
+ tearDown: function (next) {
+ this.server.close();
+ this.test_server.close();
+ next();
+ },
+
+ "A template can include the output of executing another template with kumascript.template()": function (test) {
+ var expected_fn = __dirname + '/fixtures/documents/template-exec-expected.txt',
+ result_url = 'http://localhost:9000/docs/template-exec';
+ performTestRequest(test, expected_fn, result_url);
+ },
+
+ "A template can export methods and data to another template with kumascript.require()": function (test) {
+ var expected_fn = __dirname + '/fixtures/documents/library-test-expected.txt',
+ result_url = 'http://localhost:9000/docs/library-test';
+ performTestRequest(test, expected_fn, result_url);
+ },
+
+ "The server can be configured to auto-require some templates": function (test) {
+ var expected_fn = __dirname + '/fixtures/documents/autorequire-expected.txt',
+ result_url = 'http://localhost:9000/docs/autorequire';
+ performTestRequest(test, expected_fn, result_url);
+ },
+
"A sub-API installed into APIContext should be usable in a template": function (test) {
var $this = this,
t_fn = 'api1.txt',
@@ -96,69 +151,6 @@ module.exports = {
});
- },
-
- "Exercise some popular MDN templates that have been transliterated": function (test) {
-
- // TBD: Disabled for now.
- return test.done();
-
- // [List of popular MDN templates][tmpl_list]
- // [tmpl_list]: https://bug714804.bugzilla.mozilla.org/attachment.cgi?id=588125
-
- /*
- var $this = this,
- t_fn = 'api2.txt',
- loader = new ks_loaders.FileLoader({
- filename_template: __dirname + '/fixtures/templates/{name}.ejs'
- }),
- mp = new ks_macros.MacroProcessor({ loader: loader }),
- api_ctx = new ks_api.APIContext({ });
-
- api_ctx.installAPI(DemoAPI, 'demo');
-
- _.extend(api_ctx.wiki, {
-
- // Mock out pageExists with pretend pages.
- pageExists: function (path) {
- var pretend_exists = [
- "en/CSS/position",
- "en/CSS/auto",
- "en/XUL/content",
- "en/XUL:member",
- "en/XPCOM_Interface_Reference/nsISupports",
- "en/nsIDocShell"
- ];
- return (pretend_exists.indexOf(path) !== -1);
- },
-
- // Mock out uri() until we have a better implementation.
- uri: function (path, query) {
- var out = 'http://example.com/' + path;
- if (query) { out += '?' + query; }
- return out;
- }
-
- });
-
- api_ctx.Page.uri = api_ctx.page.uri =
- 'http://example.com/en/HTML/FakePage';
-
- fs.readFile(__dirname + '/fixtures/' + t_fn, function (err, data) {
-
- var parts = (''+data).split('---'),
- src = parts.shift(),
- expected = parts.shift();
-
- mp.process(src, api_ctx, function (err, result) {
- if (err) { throw err; }
- util.debug("RESULT\n" + result.trim());
- test.equal(result.trim(), expected.trim());
- test.done();
- });
-
- });
- */
}
};
Something went wrong with that request. Please try again.