Skip to content
This repository

improved logger, support for more js shells, and quit() env module #79

Open
wants to merge 2 commits into from

2 participants

Jacob Beard James Burke
Jacob Beard

Sorry this is a bit of a messy patch - I made a bunch of changes in an anonymous clone of r.js before forking and cloning my own, so it includes a number of small contributions.

This is my first contribution to r.js, but I think I sent a CLA in a few months ago.

Changes:

  • Added support for 'spartan' shell environments. These are defined to be environments that expose global print() and load() functions, and that expose command-line arguments on top-level Arguments array. This now works for spidermonkey (Mozilla), jsc (Webkit), and v8 (Chrome) implementations. v8 must be patched to expose command-line arguments as top-level Arguments array. Rhino can also be considered a spartan shell environment.
  • Improved logger module to accept multiple arguments, like console.log.
  • Added quit() module for node, rhino, and spartan shell environments.
jbeard4 added some commits
Jacob Beard jbeard4 * Added .gitignore to ignore vim swap files.
    * Added support for 'spartan' shell environments. These are defined to be environments that expose global print() and load() functions, and that expose command-line arguments on top-level Arguments array. This now works for spidermonkey (Mozilla), jsc (Webkit), and v8 (Chrome) implementations. v8 must be patched to expose command-line arguments as top-level Arguments array. Rhino can also be considered a spartan shell environment.
    * Improved logger module to accept multiple arguments, like console.log.
    * Added quit() module for node, rhino, and spartan shell environments.
c17c31e
Jacob Beard jbeard4 Added basic browser support for r.js. 1dd826d
James Burke
Owner

I'm open to supporting more environments, but I want to be sure I can run all the tests in those environments before calling them officially supported. So on that note:

  • We should identify exactly which environments will be supported.
  • We need real usable env/file.js files for each supportable environment.
  • I do not think the browser one makes sense, at least at this time because of the issues with having a usable file.js
  • We need a top level build driving script, similar to build/tests/alln.sh for each env.
  • the logger enhancement seems like good idea.

Hmm, the more I think about this, what is your motivation for these changes? Is it to do builds or something else? In particular, I'm not sure when the quit module would be used.

Jacob Beard

The motivation behind the patch for "spartan" shell environments is to be able to use requirejs to load modules in shell environments that only expose print(), load() and quit() functions on the global object, as well as optionally exposing command-line arguments on the top-level Arguments array, and not much else. Examples of such shells are the example shells that ship with the spidermonkey, jsc, and v8 interpreters.

I've used this to develop a performance testing harness for a library I've written. This harness is able to use the patched r.js to load requirejs modules, which are then able to run equivalently in: the browser, nodejs, rhino, spidermonkey, jsc and v8 JavaScript shell environments. This enables me to benchmark my code under all major js interpreters, without needing to script a browser instance, and to precisely measure memory usage and performance of my library under these shells, independent of the browser environment.

The downside to this approach is that these "spartan" shells are quite minimal out of the box, and, for example, jsc and v8 shells are unable to read files. spidermonkey has rudimentary support for reading from files. This means that r.js will not have the same capabilities under spartan environments as it does in node.js or rhino, which do expose file APIs. I think, right now, you only need file support for the optimizer, so this means that the requirejs optimizer would be unable to run under the spartan environments, thus leading to some fragmentation of r.js capabilities. For example, if the user ran "jsc r.js -o", r.js would need to warn the user that the optimizer is not supported under that environment, and to instead run with node.js or rhino.

As owner of r.js, it's up to you to decide whether support for "spartan" shells is something that should get merged into the r.js core. In its favor, I'll mention that I think JavaScript gets embedded in many different contexts, and the need to run requirejs in minimal or unexpected environments has come up a few times in my work, for example using RequireJS in Ant: https://groups.google.com/d/msg/requirejs/w7z9Rn0NpwQ/dYBglloMKTIJ

Essentially, getting requirejs to work in environments that only support print() and load() makes it more versatile, but makes it more complicated to support.

I don't yet have a detailed understanding of how this affects automated testing of r.js.

James Burke
Owner

OK, that helps explain the motivation. I like the idea, but I would like to see the patch done differently. In particular:

  • removal of the browser branch, it would not apply for this, that is what require.js is for.
  • removal of the quit module, I cannot see where it is actually used, it is not needed by r.js itself.
  • The file module for spartan should just be an empty file with a comment that it cannot be supported generically.
Jacob Beard

Hi,

Thanks for taking the time to respond, and sorry it took me so long to get back to you. All of the changes you request sound reasonable and easy to implement, but there are a few issues that I want to bring up.

  • I added the browser branch so that the env plugin could be used in the browser. This was primarily used to allow the logger module to also work in the browser, thus making modules which depend on logger portable. I know that neither env nor logger are ready for outside consumption, and how to make them portable to the browser may not have been fully thought out yet, so I understand if this is not a use case that can currently be supported, and thus should not get merged into master.

  • The quit module is used to explicitly terminate the current process, and return an exit code. All shell environments expose a function for this (process.exit() in node, and quit() in everything else), and it is thus fairly portable. At the moment, I believe there is no way to use env modules except to add them to the r.js core, which is why I included it in this patch. There are some use cases in my framework where I require this functionality, but I understand that it is not needed for the optimizer, and so I can take it out of the patch and maintain it in my own branch.

  • There is no problem with the third point ;)

Please let me know what you think. Thanks.

James Burke
Owner

1) I do like the idea of allowing the r.js modules to work in a browser, but I would want to see a more concrete use case, before merging that part. Do you use them in the browser now? I would at least like to have a test case in r.js/tests to show some of it working. We can pick that up in a separate pull request though, make this patch a bit more focused on "other" environments.

2) OK, quit can stay.

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

Showing 2 unique commits by 1 author.

Jan 09, 2012
Jacob Beard jbeard4 * Added .gitignore to ignore vim swap files.
    * Added support for 'spartan' shell environments. These are defined to be environments that expose global print() and load() functions, and that expose command-line arguments on top-level Arguments array. This now works for spidermonkey (Mozilla), jsc (Webkit), and v8 (Chrome) implementations. v8 must be patched to expose command-line arguments as top-level Arguments array. Rhino can also be considered a spartan shell environment.
    * Improved logger module to accept multiple arguments, like console.log.
    * Added quit() module for node, rhino, and spartan shell environments.
c17c31e
Jan 12, 2012
Jacob Beard jbeard4 Added basic browser support for r.js. 1dd826d
This page is out of date. Refresh to see the latest.
1  .gitignore
@@ -22,3 +22,4 @@ build/tests/override/node_modules
22 22 build/tests/override/one-built.js
23 23 tests/node/node_modules
24 24 tests/node/embedded/node_modules
  25 +*.sw*
1  build/jslib/browser/args.js
... ... @@ -0,0 +1 @@
  1 +define(function(){return function(){};});
1  build/jslib/browser/file.js
... ... @@ -0,0 +1 @@
  1 +define(function(){return function(){};});
2  build/jslib/browser/load.js
... ... @@ -0,0 +1,2 @@
  1 +//nothing to do here
  2 +define(function(){return function(){};});
1  build/jslib/browser/optimize.js
... ... @@ -0,0 +1 @@
  1 +define(function(){return function(){};});
6 build/jslib/browser/print.js
... ... @@ -0,0 +1,6 @@
  1 +define(function(){
  2 + return function(){
  3 + console.log.apply(console,Array.prototype.slice.apply(arguments));
  4 + };
  5 +});
  6 +
1  build/jslib/browser/quit.js
... ... @@ -0,0 +1 @@
  1 +define(function(){return function(){};});
4 build/jslib/env.js
@@ -22,6 +22,8 @@
22 22 env = 'node';
23 23 } else if (typeof window !== "undefined" && navigator && document) {
24 24 env = 'browser';
  25 + } else if (typeof load === 'function' && typeof print === 'function'){
  26 + env = 'spartan';
25 27 }
26 28
27 29 define({
@@ -44,4 +46,4 @@
44 46 });
45 47 }
46 48 });
47   -}());
  49 +}());
24 build/jslib/logger.js
@@ -21,36 +21,36 @@ define(['env!env/print'], function (print) {
21 21 this.level = level;
22 22 },
23 23
24   - trace: function (message) {
  24 + trace: function () {
25 25 if (this.level <= this.TRACE) {
26   - this._print(message);
  26 + this._print(arguments);
27 27 }
28 28 },
29 29
30   - info: function (message) {
  30 + info: function () {
31 31 if (this.level <= this.INFO) {
32   - this._print(message);
  32 + this._print(arguments);
33 33 }
34 34 },
35 35
36   - warn: function (message) {
  36 + warn: function () {
37 37 if (this.level <= this.WARN) {
38   - this._print(message);
  38 + this._print(arguments);
39 39 }
40 40 },
41 41
42   - error: function (message) {
  42 + error: function () {
43 43 if (this.level <= this.ERROR) {
44   - this._print(message);
  44 + this._print(arguments);
45 45 }
46 46 },
47 47
48   - _print: function (message) {
49   - this._sysPrint((this.logPrefix ? (this.logPrefix + " ") : "") + message);
  48 + _print: function (args) {
  49 + this._sysPrint(this.logPrefix ? (this.logPrefix + " ") : "", Array.prototype.slice.apply(args));
50 50 },
51 51
52   - _sysPrint: function (message) {
53   - print(message);
  52 + _sysPrint: function (logPrefix,args) {
  53 + print.apply(this,[logPrefix].concat(args));
54 54 }
55 55 };
56 56
6 build/jslib/node/print.js
@@ -8,9 +8,5 @@
8 8 /*global define: false, console: false */
9 9
10 10 define(function () {
11   - function print(msg) {
12   - console.log(msg);
13   - }
14   -
15   - return print;
  11 + return console.log;
16 12 });
4 build/jslib/node/quit.js
... ... @@ -0,0 +1,4 @@
  1 +define(function(){
  2 + return process.exit;
  3 +});
  4 +
3  build/jslib/rhino/quit.js
... ... @@ -0,0 +1,3 @@
  1 +define(function(){
  2 + return quit;
  3 +});
1  build/jslib/spartan.js
1  build/jslib/spartan/args.js
48 build/jslib/spartan/file.js
... ... @@ -0,0 +1,48 @@
  1 +//file API's may not be supported in spartan shell environments
  2 +define({
  3 + getLineSeparator: function () {
  4 + },
  5 +
  6 + exists: function (fileName) {
  7 + },
  8 +
  9 + parent: function (fileName) {
  10 + },
  11 +
  12 + normalize: function (fileName) {
  13 + },
  14 +
  15 + isFile: function (path) {
  16 + },
  17 +
  18 + isDirectory: function (path) {
  19 + },
  20 +
  21 + absPath: function (fileObj) {
  22 + },
  23 +
  24 + getFilteredFileList: function (/*String*/startDir, /*RegExp*/regExpFilters, /*boolean?*/makeUnixPaths, /*boolean?*/startDirIsJavaObject) {
  25 + },
  26 +
  27 + copyDir: function (/*String*/srcDir, /*String*/destDir, /*RegExp?*/regExpFilter, /*boolean?*/onlyCopyNew) {
  28 + },
  29 +
  30 + copyFile: function (/*String*/srcFileName, /*String*/destFileName, /*boolean?*/onlyCopyNew) {
  31 + },
  32 +
  33 + renameFile: function (from, to) {
  34 + },
  35 +
  36 + readFile: function (/*String*/path, /*String?*/encoding) {
  37 + },
  38 +
  39 + saveUtf8File: function (/*String*/fileName, /*String*/fileContents) {
  40 + },
  41 +
  42 + saveFile: function (/*String*/fileName, /*String*/fileContents, /*String?*/encoding) {
  43 + },
  44 +
  45 + deleteFile: function (/*String*/fileName) {
  46 + }
  47 +});
  48 +
1  build/jslib/spartan/load.js
1  build/jslib/spartan/optimize.js
1  build/jslib/spartan/print.js
1  build/jslib/spartan/quit.js
49 build/jslib/x.js
@@ -98,7 +98,30 @@ var requirejs, require, define;
98 98 commandOption = fileName.substring(1);
99 99 fileName = process.argv[3];
100 100 }
101   - }
  101 + } else if (typeof window !== "undefined" && navigator && document) {
  102 + env = 'browser';
  103 + } else if (typeof load === 'function' && typeof print === 'function'){
  104 + env = 'spartan';
  105 +
  106 + fileName = args[0];
  107 +
  108 + if (fileName && fileName.indexOf('-') === 0) {
  109 + commandOption = fileName.substring(1);
  110 + fileName = args[1];
  111 + }
  112 +
  113 + exec = eval;
  114 +
  115 + //Define a console.log for easier logging. Don't
  116 + //get fancy though.
  117 + if (typeof console === 'undefined') {
  118 + console = {
  119 + log: function () {
  120 + print.apply(undefined, arguments);
  121 + }
  122 + };
  123 + }
  124 + }
102 125
103 126 //INSERT require.js
104 127
@@ -115,6 +138,9 @@ var requirejs, require, define;
115 138
116 139 //INSERT build/jslib/node.js
117 140
  141 + } else if (env === 'spartan'){
  142 +
  143 + //INSERT build/jslib/spartan.js
118 144 }
119 145
120 146 //Support a default file name to execute. Useful for hosted envs
@@ -225,19 +251,24 @@ var requirejs, require, define;
225 251 //Just run an app
226 252
227 253 //Load the bundled libraries for use in the app.
228   - if (commandOption === 'lib') {
  254 + //browser loads libs by default
  255 + if (commandOption === 'lib' || env === 'browser') {
229 256 loadLib();
230 257 }
231 258
232 259 setBaseUrl(fileName);
233 260
234   - if (exists(fileName)) {
235   - exec(readFile(fileName), fileName);
236   - } else {
237   - showHelp();
  261 + if(env === 'spartan'){
  262 + load(fileName);
  263 + } else if (env !== 'browser'){
  264 + if (exists(fileName)) {
  265 + exec(readFile(fileName), fileName);
  266 + } else {
  267 + showHelp();
  268 + }
238 269 }
239 270 }
240 271
241   -}((typeof console !== 'undefined' ? console : undefined),
242   - (typeof Packages !== 'undefined' ? Array.prototype.slice.call(arguments, 0) : []),
243   - (typeof readFile !== 'undefined' ? readFile : undefined)));
  272 +})((typeof console !== 'undefined' ? console : undefined),
  273 + ((typeof Packages !== 'undefined' || (typeof load === 'function' && typeof print === 'function'))? Array.prototype.slice.call(arguments, 0) : []),
  274 + (typeof readFile !== 'undefined' ? readFile : undefined));
3  dist.js
@@ -22,7 +22,7 @@ var fs = require('fs'),
22 22 loadRegExp = /\/\/INSERT ([\w\/\.]+)/g,
23 23 moduleNameRegExp = /build\/jslib\/([\w\/\-]+)\.js$/,
24 24 defRegExp = /define\s*\(/,
25   - envs = ['node', 'rhino'],
  25 + envs = ['node', 'rhino', 'spartan', 'browser'],
26 26 //Update this list of files by running the optimizer against
27 27 //build/jslib/opto.build.js,
28 28 //but then remove any jslib/node entries and make sure there is
@@ -36,6 +36,7 @@ var fs = require('fs'),
36 36 'env!env/file',
37 37 'build/jslib/lang.js',
38 38 'env!env/print',
  39 + 'env!env/quit',
39 40 'build/jslib/logger.js',
40 41 'build/jslib/blank.js',
41 42 'build/jslib/blank.js',

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.