Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jbeard4
Copy link

@jbeard4 jbeard4 commented Jan 12, 2012

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.

    * 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.
@jrburke
Copy link
Member

jrburke commented Jan 18, 2012

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.

@jbeard4
Copy link
Author

jbeard4 commented Jan 18, 2012

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.

@jrburke
Copy link
Member

jrburke commented Jan 21, 2012

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.

@jbeard4
Copy link
Author

jbeard4 commented Feb 16, 2012

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.

@jrburke
Copy link
Member

jrburke commented Feb 21, 2012

  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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants